Re: [PATCH v3 2/3] staging: r8188eu: Shorten calls chain of rtw_read8/16/32()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/6/21 5:07 PM, Greg Kroah-Hartman wrote:
On Sun, Sep 05, 2021 at 12:00:47AM +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>
---

v2->v3: No changes.

v1->v2: No changes.

 drivers/staging/r8188eu/hal/usb_ops_linux.c | 92 +++++++++++++++++----
 1 file changed, 78 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
index a87b0d2e87d0..f9c4fd5a2c53 100644
--- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
+++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
@@ -97,38 +97,102 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata,
 	return status;
 }
+static int usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 size)
+{
+	u16 value = (u16)(addr & 0x0000ffff);

Why not just pass in the address as a 16bit value?


+	struct adapter *adapt = intfhdl->padapter;
+	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
+	struct usb_device *udev = dvobjpriv->pusbdev;
+	int status;
+	u8 *io_buf;
+	int vendorreq_times = 0;
+
+	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx) {
+		status = -EPERM;
+		goto exit;

This is "interesting" to see if it's really even working as they think
it does, but let's leave it alone for now...

+	}
+
+	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
+
+	/*  Acquire IO memory for vendorreq */
+	io_buf = dvobjpriv->usb_vendor_req_buf;
+
+	if (!io_buf) {
+		DBG_88E("[%s] io_buf == NULL\n", __func__);

How can this buffer ever be NULL?

+		status = -ENOMEM;
+		goto release_mutex;
+	}

Why share a buffer at all anyway?

+	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
+		status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
+					      REALTEK_USB_VENQT_READ, value,
+					      REALTEK_USB_VENQT_CMD_IDX, io_buf,
+					      size, RTW_USB_CONTROL_MSG_TIMEOUT,
+					      GFP_KERNEL);
+		if (!status) {   /*  Success this control transfer. */

Comments go on the next line.

+			rtw_reset_continual_urb_error(dvobjpriv);
+			memcpy(data, io_buf, size);
+		} else { /*  error cases */

Again, next line for the comment.

+			DBG_88E("reg 0x%x, usb %s %u fail, status:%d vendorreq_times:%d\n",
+				value, "read", size, status, vendorreq_times);

These should be removed eventually...

+
+			if (status == (-ESHUTDOWN) || status == -ENODEV) {
+				adapt->bSurpriseRemoved = true;

Odd, but ok...

+			} else {
+				struct hal_data_8188e *haldata = GET_HAL_DATA(adapt);
+
+				haldata->srestpriv.wifi_error_status = USB_VEN_REQ_CMD_FAIL;

Why are we not saying the command failed even if the device was removed?

But if we do say an error happened, why are we trying to send this out
again?  What would happen to make it work the second time?

+			}
+
+			if (rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {
+				adapt->bSurpriseRemoved = true;

So we try to see if the device was removed again?

+				break;
+			}
+		}
+
+		/*  firmware download is checksummed, don't retry */
+		if ((value >= FW_8188E_START_ADDRESS && value <= FW_8188E_END_ADDRESS) || !status)
+			break;

Nothing like a special case for firmware magic.

Those calls should just use a different write function entirely,
eventually, to remove this...

Ok, I know you are just moving code around, this is fine, just pointing
out things that should be fixed up eventually...


I agree with all statements, and I've asked maintainers about some of them before. It's 100% should be fixed, but we (me and Fabio) want to fix them step by step to not rebase every time new clean up goes in.

Our plan is: (Fabio, please, correct me, if I am wrong here)

1. ops removal + shorten call chain (this patch series)
2. my error handling series [1]
3. Clean ups for rtw_* functions (old usb_*)
4. Remove dead proc code + introduce new sysfs interface, based on old proc code.


We can prepare them all at once, but, IMO:

1. It will be really hard to review
2. A lot of rebasing through new versions, since there are a lot
clean ups appear every day.


I believe, our step-by-step plan should be more comfortable for all of us :)


Thank you!


[1] https://lore.kernel.org/linux-staging/cover.1629479152.git.paskripkin@xxxxxxxxx/



With regards,
Pavel Skripkin




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux