Re: [PATCH] rtlwifi: usb: allocate URB control message setup_packet and data buffer separately

[Jussi Kivilinna <jussi.kivilinna@xxxxxxxx>]

Quoting Larry Finger <Larry.Finger@xxxxxxxxxxxx>:

> On 02/16/2013 05:00 PM, Jussi Kivilinna wrote:
> > rtlwifi allocates both setup_packet and data buffer of control message urb,
> > using shared kmalloc in _usbctrl_vendorreq_async_write. Structure used for
> > allocating is:
> > 	struct {
> > 		u8 data[254];
> > 		struct usb_ctrlrequest dr;
> > 	};
> >
> > Because 'struct usb_ctrlrequest' is __packed, setup packet is unaligned and
> > DMA mapping of both 'data' and 'dr' confuses ARM/sunxi, leading to memory
> > corruptions and freezes.
> >
> > Patch changes setup packet to be allocated separately.
> >
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jussi Kivilinna <jussi.kivilinna@xxxxxxxx>
>
> The only change I would make is to convert the WARN_ON to  
> WARN_ON_ONCE. In case something goes wrong, there is no need to spam  
> the logs.

Ok.

> I am not crazy about the overhead in allocating two different blocks  
> for each output packet, but if it is necessary, then it has to happen.

It already happens for '*_sync_read' calls, where setup_packet is  
alloced in 'usb_control_message'.

-Jussi

> Larry
>
> > ---
> >  drivers/net/wireless/rtlwifi/usb.c |   44  
> > +++++++++++++++++++++++-------------
> >  1 file changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/net/wireless/rtlwifi/usb.c  
> > b/drivers/net/wireless/rtlwifi/usb.c
> > index 476eaef..d98fcdb 100644
> > --- a/drivers/net/wireless/rtlwifi/usb.c
> > +++ b/drivers/net/wireless/rtlwifi/usb.c
> > @@ -42,8 +42,12 @@
> >
> >  static void usbctrl_async_callback(struct urb *urb)
> >  {
> > -	if (urb)
> > -		kfree(urb->context);
> > +	if (urb) {
> > +		/* free dr */
> > +		kfree(urb->setup_packet);
> > +		/* free databuf */
> > +		kfree(urb->transfer_buffer);
> > +	}
> >  }
> >
> >  static int _usbctrl_vendorreq_async_write(struct usb_device *udev,  
> > u8 request,
> > @@ -55,39 +59,47 @@ static int  
> > _usbctrl_vendorreq_async_write(struct usb_device *udev, u8 request,
> >  	u8 reqtype;
> >  	struct usb_ctrlrequest *dr;
> >  	struct urb *urb;
> > -	struct rtl819x_async_write_data {
> > -		u8 data[REALTEK_USB_VENQT_MAX_BUF_SIZE];
> > -		struct usb_ctrlrequest dr;
> > -	} *buf;
> > +	const u16 databuf_maxlen = REALTEK_USB_VENQT_MAX_BUF_SIZE;
> > +	u8 *databuf;
> > +
> > +	if (WARN_ON(len > databuf_maxlen))
> > +		len = databuf_maxlen;
> >
> >  	pipe = usb_sndctrlpipe(udev, 0); /* write_out */
> >  	reqtype =  REALTEK_USB_VENQT_WRITE;
> >
> > -	buf = kmalloc(sizeof(*buf), GFP_ATOMIC);
> > -	if (!buf)
> > +	dr = kmalloc(sizeof(*dr), GFP_ATOMIC);
> > +	if (!dr)
> >  		return -ENOMEM;
> >
> > +	databuf = kmalloc(databuf_maxlen, GFP_ATOMIC);
> > +	if (!databuf) {
> > +		kfree(dr);
> > +		return -ENOMEM;
> > +	}
> > +
> >  	urb = usb_alloc_urb(0, GFP_ATOMIC);
> >  	if (!urb) {
> > -		kfree(buf);
> > +		kfree(databuf);
> > +		kfree(dr);
> >  		return -ENOMEM;
> >  	}
> >
> > -	dr = &buf->dr;
> > -
> >  	dr->bRequestType = reqtype;
> >  	dr->bRequest = request;
> >  	dr->wValue = cpu_to_le16(value);
> >  	dr->wIndex = cpu_to_le16(index);
> >  	dr->wLength = cpu_to_le16(len);
> >  	/* data are already in little-endian order */
> > -	memcpy(buf, pdata, len);
> > +	memcpy(databuf, pdata, len);
> >  	usb_fill_control_urb(urb, udev, pipe,
> > -			     (unsigned char *)dr, buf, len,
> > -			     usbctrl_async_callback, buf);
> > +			     (unsigned char *)dr, databuf, len,
> > +			     usbctrl_async_callback, NULL);
> >  	rc = usb_submit_urb(urb, GFP_ATOMIC);
> > -	if (rc < 0)
> > -		kfree(buf);
> > +	if (rc < 0) {
> > +		kfree(databuf);
> > +		kfree(dr);
> > +	}
> >  	usb_free_urb(urb);
> >  	return rc;
> >  }
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html