On 9/17/21 17:50, Greg Kroah-Hartman wrote:
On Fri, Sep 17, 2021 at 09:18:35AM +0200, Fabio M. De Francesco wrote:
Shorten the calls chain of rtw_read8/16/32() down to the actual reads.
For this purpose unify the three usb_read8/16/32 into the new
usb_read(); make the latter parameterizable with 'size'; embed most of
the code of usbctrl_vendorreq() into usb_read() and use in it the new
usb_control_msg_recv() API of USB Core.
Suggested-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Co-developed-by: Pavel Skripkin <paskripkin@xxxxxxxxx>
Signed-off-by: Pavel Skripkin <paskripkin@xxxxxxxxx>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
---
drivers/staging/r8188eu/hal/usb_ops_linux.c | 59 +++++++++++++++++++--
1 file changed, 56 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index 2d5e9b3ba538..ef35358cf2d3 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -89,6 +89,59 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
return status;
}
+static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
+{
+ struct adapter *adapt = intfhdl->padapter;
+ struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
+ struct usb_device *udev = dvobjpriv->pusbdev;
+ int status;
+ u8 *io_buf; /* Pointer to I/O buffer */
As you "know" size is not going to be larger than 4 (hint, you should
prboably check it), just use bytes off of the stack here, and you can
ignore this buffer entirely. That will hopefully allow you in the
future to get rid of that buffer as odds are it will not be needed
anymore.
+
+ if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
+ return -EPERM;
How is it ok to check this outside of the lock? What happens if these
values change right _after_ you check them?
Why check them at all, is this something that we even care about?
I know you are trying to make this just the same logic at is there
today, but why not just do it right the first time?
+
+ mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
+
+ io_buf = dvobjpriv->usb_vendor_req_buf;
+
+ status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
+ REALTEK_USB_VENQT_READ, addr,
+ REALTEK_USB_VENQT_CMD_IDX, io_buf,
+ size, RTW_USB_CONTROL_MSG_TIMEOUT,
+ GFP_KERNEL);
+
+ if (status == -ESHUTDOWN ||
+ status == -ENODEV ||
+ status == -ENOENT) {
+ /*
+ * device or controller has been disabled due to
+ * some problem that could not be worked around,
+ * device or bus doesn’t exist, endpoint does not
+ * exist or is not enabled.
+ */
+ adapt->bSurpriseRemoved = true;
+ goto mutex_unlock;
+ }
+
+ if (status < 0) {
+ GET_HAL_DATA(adapt)->srestpriv.wifi_error_status =
+ USB_VEN_REQ_CMD_FAIL;
+
+ if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
+ adapt->bSurpriseRemoved = true;
+
+ goto mutex_unlock;
+ }
+
+ rtw_reset_continual_urb_error(dvobjpriv);
+ memcpy(data, io_buf, size);
+
+mutex_unlock:
+ mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
+
+ return status;
No one cares about this value, is that ok?
Hi, Greg
I haven't yet read your previous messages, I will do it right after I
send this email.
Ignoring return value is 100% bug and i've fixed it my RFC series, that
was posted like month ago. As you suggested, we firstly cleaning up
usb_ops_linux and then I will rebase my series on top of this series.
Goal of my series is to add proper error handling of USB i/o all across
the driver code.
Thank you for reviewing our patches :)
With regards,
Pavel Skripkin
