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:

quoted

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

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 
[off-list ref], remove 'pipe' and pass an explicit 0
to the new API. According to suggestions by Pavel Skripkin 
[off-list ref], 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 [off-list ref]
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