On Monday, September 6, 2021 4:07:26 PM CEST Greg Kroah-Hartman wrote:

On Sun, Sep 05, 2021 at 12:00:47AM +0200, Fabio M. De Francesco wrote:

quoted

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@linuxfoundation.org>
Co-developed-by: Pavel Skripkin <redacted>
Signed-off-by: Pavel Skripkin <redacted>
Signed-off-by: Fabio M. De Francesco <redacted>
---

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

quoted

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,

quoted

 	return status;
 }
 
+static int usb_read(struct intf_hdl *intfhdl, u32 addr, void *data, u8 

size)

quoted

+{
+	u16 value = (u16)(addr & 0x0000ffff);

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

Yes, it should be better. It will be done in v4.

quoted

+	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) 

{

quoted

+		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...

As you suggest, I also prefer to leave it alone for now.

quoted

+	}
+
+	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?

As you noticed a few lines below this, I moved some code around and ignored 
and left as was anything that wasn't functional for my purpose.. 

quoted

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

Why share a buffer at all anyway?

Same answer as above.

quoted

+	while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) {
+		status = usb_control_msg_recv(udev, 0, 

REALTEK_USB_VENQT_CMD_REQ,

quoted

+					      

REALTEK_USB_VENQT_READ, value,

quoted

+					      

REALTEK_USB_VENQT_CMD_IDX, io_buf,

quoted

+					      size, 

RTW_USB_CONTROL_MSG_TIMEOUT,

quoted

+					      GFP_KERNEL);
+		if (!status) {   /*  Success this control transfer. */

Comments go on the next line.

This will be fixed in v4, although I'm not sure if this comment and the next 
are really necessary. The code seems self-explanatory.

quoted

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

Again, next line for the comment.

Same as above.

quoted

+			DBG_88E("reg 0x%x, usb %s %u fail, status:

%d vendorreq_times:%d\n",

quoted

+				value, "read", size, status, 

vendorreq_times);

These should be removed eventually...

I could use pr_debug() for now or remove it immediately. I'd prefer the 
latter but I'm not sure if it is the most appropriate thing to do. Let me 
think about it...

quoted

+
+			if (status == (-ESHUTDOWN) || status == -

ENODEV) {

quoted

+				adapt->bSurpriseRemoved = true;

Odd, but ok...

quoted

+			} else {
+				struct hal_data_8188e *haldata = 

GET_HAL_DATA(adapt);

quoted

+
+				haldata-

srestpriv.wifi_error_status = USB_VEN_REQ_CMD_FAIL;

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

This question is not clear to me. Are you talking about -ENOENT?
I suppose it should be if (status == (-ESHUTDOWN || -ENODEV || -ENOENT)) ...

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?

There are some errors that are unrecoverable and the loop has no reason to 
re-iterate again and again. I'll break this loop on unrecoverable errors.

I see that usb_submit_urb() at the very end of the calls chain may fail for a 
lot of different reasons and some of them are a bit obscure to me 
(unfortunately, at the moment I have no time to go deeper into the 
architecture and the inner workings of the USB subsystem :) ) 

I hope that I won't overlook any of them - as far as it regards some of all 
possible errors I have doubts in telling whether or not they are 
unrecoverable and if some can actually happen in this code :(

quoted

+			}
+
+			if 

(rtw_inc_and_chk_continual_urb_error(dvobjpriv)) {

quoted

+				adapt->bSurpriseRemoved = true;

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

This must be changed, let me see if I can understand how. At the moment I 
don't have the whole picture.
 

quoted

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

FW_8188E_END_ADDRESS) || !status)

quoted

+			break;

Nothing like a special case for firmware magic.

This is something that I really cannot understand, so I think I'll leave it 
as-is unless I may get some more hints... :)

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...

Yes, I'm just moving the code around. Anyway I guess that I can fix/change  
most of the things you pointed out.

Thanks very much for your review,

Fabio  

thanks,

greg k-h