Re: [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests

Pavel Skripkin <paskripkin@xxxxxxxxx> · Fri, 17 Sep 2021 18:03:52 +0300

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@xxxxxxxxx>

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@xxxxxxxxx>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
Signed-off-by: Pavel Skripkin <paskripkin@xxxxxxxxx>
---
 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











