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