Re: [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()

[Fabio M. De Francesco]

On Friday, September 17, 2021 4:50:10 PM CEST 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
> probably 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.

Dear Greg,

Yes we know that in fact, for the rtw_read*() cases, we only have 1,2,4 bytes 
size embedded in the calls. It's different for rtw_write*() where we have a 
version, that is rtw_writeN(), that could pass larger sizes (checked by the 
caller to not be larger than  VENDOR_CMD_MAX_DATA_LEN).

Said that, we already get rid of the buffer in 19/19. As far as 17/19 and 
18/19 are regarded we prefer to leave the code as-is, because we have 
many other changes in this 17/19 and in the next 18/19.

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

Yes, this is a mistake that was in the original code and we didn't noticed. 
Anyway, we guess that moving the acquire of he mutex soon before this check 
is all that is required. I hope it is the correct fix. Isn't it?

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

Yes, we are trying to make the same logic that is there today: we already 
changed a lot of other logic that was in this code. I see that 
bSurpriseRemoved is set in error cases, so for the moment we don't feel like 
removing anything related to this variable. With a high degree of probability  
we'll do this task later in future patches. 

Do you agree with the argument above?

For instance, we got rid of that while loop around the control messages 
sending/receiving API calls and we got rid of some plainly useless 'if' tests 
(such as "if ((value >= FW_8188E_START_ADDRESS && value <= 
FW_8188E_END_ADDRESS) || status == len) break;"), and much more.

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

I'm aware of Pavel's work on changing all the callers up to the top of the 
chains in order to do proper error checking. That's why we return status 
here.

> 
> thanks,
> 
> greg k-h
>