On 05/27/2012 12:34 PM, Ben Hutchings wrote:
On Sat, 2012-05-26 at 22:59 -0500, Larry Finger wrote:
commit a7959c1394d4126a70a53b914ce4105f5173d0aa
Author: Larry Finger<[email protected]>
Date: Mon Mar 19 15:44:31 2012 -0500
rtlwifi: Preallocate USB read buffers and eliminate kalloc in read routine
The current version of rtlwifi for USB operations uses kmalloc to
acquire a 32-bit buffer for each read of the device. When
_usb_read_sync() is called with the rcu_lock held, the result is
a "sleeping function called from invalid context" BUG. This is
reported for two cases in
https://bugzilla.kernel.org/show_bug.cgi?id=42775.
The first case has the lock originating from within rtlwifi and could
be fixed by rearranging the locking; however, the second originates from
within mac80211. The kmalloc() call is removed from _usb_read_sync()
by creating a ring buffer pointer in the private area and
allocating the buffer data in the probe routine.
Signed-off-by: Larry Finger<[email protected]>
Signed-off-by: John W. Linville<[email protected]>
---
Ben,
This version will apply to 3.2 and earlier.
Thanks, I've queued this up.
But I'm rather suspicious of this logic:
[...]
-static u32 _usb_read_sync(struct usb_device *udev, u32 addr, u16 len)
+static u32 _usb_read_sync(struct rtl_priv *rtlpriv, u32 addr, u16 len)
{
+ struct device *dev = rtlpriv->io.dev;
+ struct usb_device *udev = to_usb_device(dev);
u8 request;
u16 wvalue;
u16 index;
- u32 *data;
- u32 ret;
+ __le32 *data =&rtlpriv->usb_data[rtlpriv->usb_data_index];
- data = kmalloc(sizeof(u32), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
request = REALTEK_USB_VENQT_CMD_REQ;
index = REALTEK_USB_VENQT_CMD_IDX; /* n/a */
wvalue = (u16)addr;
_usbctrl_vendorreq_sync_read(udev, request, wvalue, index, data, len);
- ret = *data;
- kfree(data);
- return ret;
+ if (++rtlpriv->usb_data_index>= RTL_USB_MAX_RX_COUNT)
+ rtlpriv->usb_data_index = 0;
+ return le32_to_cpu(*data);
}
[...]
This is synchronous, so why do we need a ring of 32-bit buffers rather
than just one?
If the reason is that there can be multiple concurrent calls to this
function, that doesn't seem to be handled properly at all: the index
isn't updated until after we use each buffer, nor atomically.
My thought was that this section could not have concurrent calls, and I
initially had only a single 32-bit buffer. That code worked fine for me;
however, one of the testers had a problem with that. By converting to a ring
buffer, both his and my tests were OK and showed no problems with the indexing.
I was never able to determine why his system behaved differently.
Larry
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
