On Sat, 2012-05-26 at 22:59 -0500, Larry Finger wrote: > commit a7959c1394d4126a70a53b914ce4105f5173d0aa > Author: Larry Finger <larry.fin...@lwfinger.net> > 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 <larry.fin...@lwfinger.net> > Signed-off-by: John W. Linville <linvi...@tuxdriver.com> > --- > > 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.
Ben.
--
Ben Hutchings
The obvious mathematical breakthrough [to break modern encryption] would be
development of an easy way to factor large prime numbers. - Bill Gates
signature.asc
Description: This is a digitally signed message part
