Please note: This project is no longer active. The website is kept online for historic purposes only.
If you´re looking for a Linux driver for your Atheros WLAN device, you should continue here .

Ticket #1370 (closed defect: fixed)

Opened 12 years ago

Last modified 11 years ago

[patch] Protect ath_sysctl_halparam with ATH_LOCK and make ATH_LOCK disable IRQ

Reported by: mtaylor Assigned to: mtaylor
Priority: critical Milestone: version 0.9.5
Component: madwifi: other Version: trunk
Keywords: Cc:
Patch is attached: 0 Pending:

Description

This patch makes ath_sysctl_halparam protected with the same lock as ioctl, init, reset, etc...

Further, it uses a spinlock and disable IRQs instead of using a mutex.

The only problem with this patch at the moment is that one of the ioctl operations invokes ieee80211_ioctl_create_vap and this requires interrupts to be enabled when it calls register_netdevice and enables the bottom half.

Anyone who has time to look at this, I'd appreciate any feedback or help in getting rid of the non _IRQ version of ATH_LOCK_IRQ.

Attachments

madwifi-0.9.3-athlock-irq.diff (6.9 kB) - added by mtaylor on 06/12/07 03:33:14.

Change History

06/12/07 03:33:14 changed by mtaylor

  • attachment madwifi-0.9.3-athlock-irq.diff added.

06/12/07 03:34:43 changed by mtaylor

  • owner set to mtaylor.

06/16/07 01:57:20 changed by mtaylor

  • status changed from new to closed.
  • resolution set to fixed.

Added in trunk - r2466.

06/17/07 00:20:05 changed by dyqith

  • priority changed from minor to critical.
  • status changed from closed to reopened.
  • resolution deleted.

Ran into an error with this patch in the trunk. (from r2466 upwards) Shows up when I modprobe ath_pci

BUG: sleeping function called from invalid context at include/asm/uaccess.h:500
in_atomic():0, irqs_disabled():1
 [<c04ed24b>] copy_from_user+0x35/0x66
 [<e10e28a1>] ath_ioctl+0x236/0x268 [ath_pci]
 [<c042b0b6>] current_fs_time+0x4f/0x59
 [<c048363c>] touch_atime+0xa2/0xd6
 [<e10e266b>] ath_ioctl+0x0/0x268 [ath_pci]
 [<c05c3a45>] dev_ethtool+0x9f2/0xa0d
 [<c04ea497>] prio_tree_insert+0x161/0x1ef
 [<c045a72a>] get_page_from_freelist+0x23d/0x2a7
 [<c04ec7e5>] vsnprintf+0x459/0x495
 [<c06207a4>] _read_unlock_irq+0x5/0x7
 [<c045630c>] find_get_page+0x36/0x3b
 [<c0458968>] filemap_nopage+0x18b/0x319
 [<c05b77f2>] sock_ioctl+0x0/0x1c1
 [<c05c23c0>] dev_ioctl+0x2fd/0x46b
 [<c04619ad>] __handle_mm_fault+0x898/0x8ba
 [<c05b7994>] sock_ioctl+0x1a2/0x1c1
 [<c05b77f2>] sock_ioctl+0x0/0x1c1
 [<c047c683>] do_ioctl+0x1f/0x62
 [<c047c90a>] vfs_ioctl+0x244/0x256
 [<c047c968>] sys_ioctl+0x4c/0x64
 [<c0403f64>] syscall_call+0x7/0xb
 =======================

From what I understand, ATH_LOCK (the old usage) was to consistently make sure user requests are not racy. User contexts allow IRQ's to be enabled. Why do we need to turn them off ? ath_ioctl() needs to sleep (from copy_to_user())

Was there a problem with the mutex and causing known race conditions? Right now, I'm consistenty getting the above bug message with the newest revisions.

06/17/07 00:40:40 changed by mtaylor

Working on a fix for this, and yes I could consistently obtain race conditions caused by concurrent access of the driver. The HAL locking patch also helped quite a lot and may have obviated the need for some of the locking here, but the safe solution is to avoid having the driver and device management commands running in parallel.

The critical sections are being narrowed. Will submit a cleaner version to trunk shortly.

06/17/07 02:52:09 changed by mtaylor

  • status changed from reopened to closed.
  • resolution set to fixed.

Adjusted the scope of critical sections to exclude copy_to_user calls and reduce the scope of critical sections.

06/17/07 03:49:33 changed by mtaylor

  • status changed from closed to reopened.
  • resolution deleted.

Stupid logic in attach. Reopening ticket.

06/17/07 03:52:12 changed by mtaylor

Changes reverted from r2466 and r2472 related to ATH_LOCK. Restored to original condition. Not sure how to fix this one properly. r2473 brings us back where we started.

06/17/07 04:14:55 changed by mtaylor

  • status changed from reopened to closed.
  • resolution set to fixed.

r2474 adds ATH_LOCK to sysctl, which was the problem I originally tried to solve with this ticket. The HAL API is synchronized now, so if more locking is required - it will be to protect against concurrent updates of driver data structures that can crash the running driver. I'm going to close this one again now.

06/27/07 07:24:25 changed by mrenzmann

  • version set to trunk.
  • milestone set to version 0.9.4.

02/11/08 06:19:52 changed by mrenzmann

  • milestone changed from version 0.9.4 to version 0.9.5.