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

[Fabio M. De Francesco]

On Thursday, August 26, 2021 4:45:33 PM CEST Greg Kroah-Hartman wrote:
> On Thu, Aug 26, 2021 at 04:24:35PM +0200, Fabio M. De Francesco wrote:
> > 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@xxxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> > > > ---
> > > > 
> > > > 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@xxxxxxxxxx>, remove 'pipe' and pass an explicit 0
> > > > to the new API. According to suggestions by Pavel Skripkin 
> > > > <paskripkin@xxxxxxxxx>, 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@xxxxxxxxxxxxxxxx>
> > > > 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.
> 
> When you do that, please let me know somehow that this is the case,
> otherwise how am I supposed to guess that?

Correct, my fault :( 

To my defense I can only say that I really had forgot that there were the 
above-mentioned previous patch still in your queue. So I didn't immediately 
realized that I had to inform you somehow of this kind of dependence.
I knew that only yesterday, when Pavel wanted to apply this patch to 
his local copy of your then current tree and he couldn't. After some thoughts
I understood that the latter depended on the former, but I guess it was too late 
to inform you. Furthermore, yesterday I thought that you would have applied 
in a FIFO order and that you wouldn't notice any conflict.

Actually I was wrong, because you didn't apply the former and instead asked
me to test it (we talked about that patch some minutes ago in another message).
 
> > > 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?
> 
> If you unwind the mess, you will find that the code will be much easier
> to understand.
> 
> As an example, look at usb_write8().  Where is it ever called?  Why do
> we have it at all?  It's only used in 1 place, and then that function
> unwinds into rtw_write8(), which is used in a lot of places, and never
> checked at all, making the majority of the logic in this function
> totally unneeded and useless.
> 
> Same for rtw_write16() and rtw_write32().  After unwinding the mess you
> see that the logic you are working to try to clean up in this patch
> series is pretty much not used / needed at all, right?  So why do it?
> 
> Unwind the mess into a useful set of functions the driver can actually
> call that is not 2-4 function pointers deep and then we can talk about
> unifying things, if they are really needed.  But right now, it's
> impossible to tell.

Yeah, I know how is the call chain from top (rtw_read/write8/16/32()) to bottom
(usbctrl_vendorreq()) and then to the new USB core API. 

Pavel and I have been talking about this topic while he was working on his 
series ("r8188eu: avoid uninit value bugs").

Aside from this, I re-thought about what you write above and I too find that
having 2-4 function pointers deep is a bad design. Anyway I'm stuck in 
waiting to see what Pavel will submit with his reworking, because I don't 
desire to make patches that conflict with his.

As you often say to all us: there is no hurry! So, I'll wait to see Pavel's final
work before changing whatever could conflict with him.

> good luck!

Thanks, I'll need it :-)

And thanks for the time you spent to clarify your thoughts about these topics.

Fabio
 
> greg k-h