Re: [PATCH v3 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq()

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

On Thursday, August 26, 2021 12:48:37 PM CEST Greg Kroah-Hartman wrote:
> On Wed, Aug 25, 2021 at 05:53:10AM +0200, Fabio M. De Francesco wrote:
> > Replace usb_control_msg() with the new usb_control_msg_recv() and
> > usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
> > Remove no more needed variables. Move out of an if-else block
> > some code that it is no more dependent on status < 0. Remove
> > redundant code depending on status > 0 or status == len.
> > 
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> > v2->v3: Restore the test for success of usb_control_message_recv/send
> > that was inadvertently removed. Issue reported by Pavel Skripkin.
> > 
> > v1->v2: According to suggestions by Christophe JAILLET 
> > <christophe.jaillet@wanadoo.fr>, remove 'pipe' and pass an explicit 0
> > to the new API. According to suggestions by Pavel Skripkin 
> > <paskripkin@gmail.com>, remove an extra if-else that is no more needed, 
> > since status can be 0 and < 0 and there is no 3rd state, like it was before.
> > Many thanks to them and also to Phillip Potter <phil@philpotter.co.uk>
> > who kindly offered his time for the purpose of testing v1.
> > 
> >  drivers/staging/r8188eu/hal/usb_ops_linux.c | 45 ++++++++-------------
> >  1 file changed, 17 insertions(+), 28 deletions(-)
> 
> This doesn't apply to my tree at all.  Please rebase and resend.

This series cannot apply to your tree until another one of mine is applied 
("staging: r8188eu: Remove _enter/_exit_critical_mutex()"). This series builds
on the previous patch.

> But first, are you sure you want to use these new functions here?  This
> is a "common" function that is called from different places for
> different things.  How about unwinding the callers of this function
> first, to see if they really need all of the complexity in this function
> at all, and if not, then call the real USB function in those locations
> instead.

I think it could be fine to simply refactor usbctrl_vendorreq() to use the newer
API with no necessity to directly use them at least in six different places in
hal/usb_ops_linux.c. The only users of this helper are usb_read8/16/32() and
usb_write8/16/32(). Why do you prefer using usb_control_msg_recv/send() 
directly in the callers? I guess it would lead to redundant code, more or less
the same code repeated again and again within the above-mentioned six callers.
What do we improve by doing as you suggest? What am I missing?
 
> It's only used in this single file, so it shouldn't be that hard to
> unwind (after seeing where those calls are made from, and if they even
> need to be present at all.  Hint, look at the mess of where _write16 and
> friends are set to realize that structure is not needed at all, right?
> It's a long chain, the more you pull on it, the messier you realize it
> is...)

I've already exposed my POV above. However, I know that Pavel is working on
usb_read*() and usb_write*() and I wouldn't avoid to change those functions
while he is changing them. Shouldn't I better avoid further changes until 
my "Remove _enter/_exit_critical_mutexes()" get accepted (or definitely rejected)
and also wait for Pavel's series to be merged? Since usb_control_msg_recv/send()
don't return the length of the messages, my patch would break his checks of
ret == len and lead to serious bugs. I'd wait for his patches and then remove 
the ret == len check when we get rid of usb_control_msg() and use the new API.

What about my idea?

Thanks,

Fabio

> thanks,
> 
> greg k-h
>