Messages in this thread | | | Date | Fri, 17 Sep 2021 18:03:52 +0300 | Subject | Re: [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests | From | Pavel Skripkin <> |
| |
On 9/17/21 17:55, Greg Kroah-Hartman wrote: > On Fri, Sep 17, 2021 at 09:18:37AM +0200, Fabio M. De Francesco wrote: >> From: Pavel Skripkin <paskripkin@gmail.com> >> >> This driver used shared buffer for usb requests. It led to using >> mutexes, i.e no usb requests can be done in parallel. >> >> USB requests can be fired in parallel since USB Core allows it. In >> order to allow them, remove usb_vendor_req_buf from dvobj_priv (since >> USB I/O is the only user of it) and remove also usb_vendor_req_mutex >> (since there is nothing to protect). > > Ah, you are removing this buffer, nice! > > But, just because the USB core allows multiple messages to be sent to a > device at the same time, does NOT mean that the device itself can handle > that sort of a thing. > > Keeping that lock might be a good idea, until you can prove otherwise. > You never know, maybe there's never any contention at all for it because > these accesses are all done in a serial fashion and the lock > grab/release is instant. But if that is not the case, you might really > get a device confused here by throwing multiple control messages at it > in ways that it is not set up to handle at all. > > So please do not drop the lock. > > More comments below. >
We have tested this change. I've tested it in qemu with TP-Link TL-WN722N v2 / v3 [Realtek RTL8188EUS], and Fabio has tested it on his host for like a whole evening.
I agree, that our testing does not cover all possible cases and I can't say it was "good stress testing", so, I think, we need some comments from maintainers.
@Larry, @Phillip, does this change looks reasonable for this chip?
With regards, Pavel Skripkin
> >> >> Co-developed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> >> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> >> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> >> --- >> drivers/staging/r8188eu/hal/usb_ops_linux.c | 29 ++++++++------- >> drivers/staging/r8188eu/include/drv_types.h | 5 --- >> drivers/staging/r8188eu/os_dep/usb_intf.c | 40 ++------------------- >> 3 files changed, 16 insertions(+), 58 deletions(-) >> >> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c >> index 656f3a774e48..0ed4e6c8b1f5 100644 >> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c >> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c >> @@ -19,9 +19,9 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size) >> if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx) >> return -EPERM; >> >> - mutex_lock(&dvobjpriv->usb_vendor_req_mutex); >> - >> - io_buf = dvobjpriv->usb_vendor_req_buf; >> + io_buf = kmalloc(size, GFP_KERNEL); >> + if (!io_buf) >> + return -ENOMEM; > > Please read the docs for usb_control_msg_recv(). It can allow data off > of the stack, so no need to allocate/free the buffer like this all the > time. > > Note, the usb_control_msg() call does require the data to be allocated > dynamically, like the code does today. Which is why you probably got > confused here. > > Same for usb_control_msg_send(), it can take data off of the stack. > > >> >> status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ, >> REALTEK_USB_VENQT_READ, addr, >> @@ -39,7 +39,7 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size) >> * exist or is not enabled. >> */ >> adapt->bSurpriseRemoved = true; >> - goto mutex_unlock; >> + goto end; >> } >> >> if (status < 0) { >> @@ -49,15 +49,14 @@ static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size) >> if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) >> adapt->bSurpriseRemoved = true; >> >> - goto mutex_unlock; >> + goto end; >> } >> >> rtw_reset_continual_urb_error(dvobjpriv); >> memcpy(data, io_buf, size); >> >> -mutex_unlock: >> - mutex_unlock(&dvobjpriv->usb_vendor_req_mutex); >> - >> +end: >> + kfree(io_buf); >> return status; >> } >> >> @@ -72,9 +71,10 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size) >> if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx) >> return -EPERM; >> >> - mutex_lock(&dvobjpriv->usb_vendor_req_mutex); >> + io_buf = kmalloc(size, GFP_KERNEL); >> + if (!io_buf) >> + return -ENOMEM; >> >> - io_buf = dvobjpriv->usb_vendor_req_buf; >> memcpy(io_buf, data, size); >> >> status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ, >> @@ -93,7 +93,7 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size) >> * exist or is not enabled. >> */ >> adapt->bSurpriseRemoved = true; >> - goto mutex_unlock; >> + goto end; >> } >> >> if (status < 0) { >> @@ -103,14 +103,13 @@ static int usb_write(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size) >> if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) >> adapt->bSurpriseRemoved = true; >> >> - goto mutex_unlock; >> + goto end; >> } >> >> rtw_reset_continual_urb_error(dvobjpriv); >> >> -mutex_unlock: >> - mutex_unlock(&dvobjpriv->usb_vendor_req_mutex); >> - >> +end: >> + kfree(io_buf); >> return status; >> } >> >> diff --git a/drivers/staging/r8188eu/include/drv_types.h b/drivers/staging/r8188eu/include/drv_types.h >> index 626c6273be6f..499b2bce8cbe 100644 >> --- a/drivers/staging/r8188eu/include/drv_types.h >> +++ b/drivers/staging/r8188eu/include/drv_types.h >> @@ -168,11 +168,6 @@ struct dvobj_priv { >> int ep_num[5]; /* endpoint number */ >> int RegUsbSS; >> struct semaphore usb_suspend_sema; >> - struct mutex usb_vendor_req_mutex; >> - >> - u8 *usb_alloc_vendor_req_buf; >> - u8 *usb_vendor_req_buf; > > I do like removing these buffers, and I think that is all that this > change should be doing. > > thanks, > > greg k-h >
| |