* [PATCH 0/2] staging: r8188eu: Use new usb_control_msg_recv/send()
@ 2021-08-23 22:37 Fabio M. De Francesco
2021-08-23 22:37 ` [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() Fabio M. De Francesco
2021-08-23 22:37 ` [PATCH 2/2] staging: r8188eu: Make some clean-ups " Fabio M. De Francesco
0 siblings, 2 replies; 29+ messages in thread
From: Fabio M. De Francesco @ 2021-08-23 22:37 UTC (permalink / raw)
To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging,
linux-kernel, Pavel Skripkin
Cc: Fabio M. De Francesco
Replace usb_control_msg() with the new usb_control_msg_recv() and
usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
Remove camelcase and the unneded initial 'p' (for pointer?) from
the pIo_buf variable and from the pintfhdl and pdata arguments of
usbctrl_vendorreq().
Fabio M. De Francesco (2):
staging: r8188eu: Use usb_control_msg_recv/send() in
usbctrl_vendorreq()
staging: r8188eu: Make some clean-ups in usbctrl_vendorreq()
drivers/staging/r8188eu/hal/usb_ops_linux.c | 47 ++++++++++-----------
1 file changed, 23 insertions(+), 24 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-23 22:37 [PATCH 0/2] staging: r8188eu: Use new usb_control_msg_recv/send() Fabio M. De Francesco @ 2021-08-23 22:37 ` Fabio M. De Francesco 2021-08-24 0:08 ` Phillip Potter 2021-08-24 8:13 ` Pavel Skripkin 2021-08-23 22:37 ` [PATCH 2/2] staging: r8188eu: Make some clean-ups " Fabio M. De Francesco 1 sibling, 2 replies; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-23 22:37 UTC (permalink / raw) To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Pavel Skripkin Cc: Fabio M. De Francesco Replace usb_control_msg() with the new usb_control_msg_recv() and usb_control_msg_send() API of USB Core in usbctrl_vendorreq(). Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the RFC patch. drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++----------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c index a93d5cfe4635..6f51660b967a 100644 --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c @@ -15,9 +15,8 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, struct adapter *adapt = pintfhdl->padapter; struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt); struct usb_device *udev = dvobjpriv->pusbdev; - unsigned int pipe; + u8 pipe; int status = 0; - u8 reqtype; u8 *pIo_buf; int vendorreq_times = 0; @@ -44,22 +43,22 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, } while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) { - memset(pIo_buf, 0, len); - if (requesttype == 0x01) { - pipe = usb_rcvctrlpipe(udev, 0);/* read_in */ - reqtype = REALTEK_USB_VENQT_READ; + status = usb_control_msg_recv(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, + REALTEK_USB_VENQT_READ, value, + REALTEK_USB_VENQT_CMD_IDX, + pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, + GFP_KERNEL); } else { - pipe = usb_sndctrlpipe(udev, 0);/* write_out */ - reqtype = REALTEK_USB_VENQT_WRITE; memcpy(pIo_buf, pdata, len); + status = usb_control_msg_send(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, + REALTEK_USB_VENQT_WRITE, value, + REALTEK_USB_VENQT_CMD_IDX, + pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, + GFP_KERNEL); } - status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, - reqtype, value, REALTEK_USB_VENQT_CMD_IDX, - pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT); - - if (status == len) { /* Success this control transfer. */ + if (!status) { /* Success this control transfer. */ rtw_reset_continual_urb_error(dvobjpriv); if (requesttype == 0x01) memcpy(pdata, pIo_buf, len); -- 2.32.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-23 22:37 ` [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() Fabio M. De Francesco @ 2021-08-24 0:08 ` Phillip Potter 2021-08-24 0:31 ` Fabio M. De Francesco 2021-08-24 8:13 ` Pavel Skripkin 1 sibling, 1 reply; 29+ messages in thread From: Phillip Potter @ 2021-08-24 0:08 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Larry Finger, Greg Kroah-Hartman, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List, Pavel Skripkin On Mon, 23 Aug 2021 at 23:38, Fabio M. De Francesco <fmdefrancesco@gmail.com> wrote: > > Replace usb_control_msg() with the new usb_control_msg_recv() and > usb_control_msg_send() API of USB Core in usbctrl_vendorreq(). > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > > Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the > RFC patch. > > drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++----------- > 1 file changed, 12 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c > index a93d5cfe4635..6f51660b967a 100644 > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c > @@ -15,9 +15,8 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > struct adapter *adapt = pintfhdl->padapter; > struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt); > struct usb_device *udev = dvobjpriv->pusbdev; > - unsigned int pipe; > + u8 pipe; > int status = 0; > - u8 reqtype; > u8 *pIo_buf; > int vendorreq_times = 0; > > @@ -44,22 +43,22 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > } > > while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) { > - memset(pIo_buf, 0, len); > - > if (requesttype == 0x01) { > - pipe = usb_rcvctrlpipe(udev, 0);/* read_in */ > - reqtype = REALTEK_USB_VENQT_READ; > + status = usb_control_msg_recv(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > + REALTEK_USB_VENQT_READ, value, > + REALTEK_USB_VENQT_CMD_IDX, > + pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > + GFP_KERNEL); > } else { > - pipe = usb_sndctrlpipe(udev, 0);/* write_out */ > - reqtype = REALTEK_USB_VENQT_WRITE; > memcpy(pIo_buf, pdata, len); > + status = usb_control_msg_send(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > + REALTEK_USB_VENQT_WRITE, value, > + REALTEK_USB_VENQT_CMD_IDX, > + pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > + GFP_KERNEL); > } > > - status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > - reqtype, value, REALTEK_USB_VENQT_CMD_IDX, > - pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT); > - > - if (status == len) { /* Success this control transfer. */ > + if (!status) { /* Success this control transfer. */ > rtw_reset_continual_urb_error(dvobjpriv); > if (requesttype == 0x01) > memcpy(pdata, pIo_buf, len); > -- > 2.32.0 > Dear Fabio, Thanks for the patch. Sorry, but for some reason with my N10-Nano I can't get a connection at all with this patch applied - it just won't associate with my network. Interface shows up and no OOPS in log, but just disassociates/no IP address/interface down etc. so perhaps semantics differ slightly here somehow? Tried two separate rollbacks/builds/runs just to make sure I wasn't losing my mind :-) Regards, Phil ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 0:08 ` Phillip Potter @ 2021-08-24 0:31 ` Fabio M. De Francesco 2021-08-24 1:38 ` Fabio M. De Francesco 0 siblings, 1 reply; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 0:31 UTC (permalink / raw) To: Phillip Potter Cc: Larry Finger, Greg Kroah-Hartman, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List, Pavel Skripkin On Tuesday, August 24, 2021 2:08:49 AM CEST Phillip Potter wrote: > On Mon, 23 Aug 2021 at 23:38, Fabio M. De Francesco > <fmdefrancesco@gmail.com> wrote: > > > > Replace usb_control_msg() with the new usb_control_msg_recv() and > > usb_control_msg_send() API of USB Core in usbctrl_vendorreq(). > > > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > --- > > > > Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the > > RFC patch. > > > > drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++----------- > > 1 file changed, 12 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c > > index a93d5cfe4635..6f51660b967a 100644 > > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c > > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c > > @@ -15,9 +15,8 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > > struct adapter *adapt = pintfhdl->padapter; > > struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt); > > struct usb_device *udev = dvobjpriv->pusbdev; > > - unsigned int pipe; > > + u8 pipe; > > int status = 0; > > - u8 reqtype; > > u8 *pIo_buf; > > int vendorreq_times = 0; > > > > @@ -44,22 +43,22 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > > } > > > > while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) { > > - memset(pIo_buf, 0, len); > > - > > if (requesttype == 0x01) { > > - pipe = usb_rcvctrlpipe(udev, 0);/* read_in */ > > - reqtype = REALTEK_USB_VENQT_READ; > > + status = usb_control_msg_recv(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > > + REALTEK_USB_VENQT_READ, value, > > + REALTEK_USB_VENQT_CMD_IDX, > > + pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > > + GFP_KERNEL); > > } else { > > - pipe = usb_sndctrlpipe(udev, 0);/* write_out */ > > - reqtype = REALTEK_USB_VENQT_WRITE; > > memcpy(pIo_buf, pdata, len); > > + status = usb_control_msg_send(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > > + REALTEK_USB_VENQT_WRITE, value, > > + REALTEK_USB_VENQT_CMD_IDX, > > + pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > > + GFP_KERNEL); > > } > > > > - status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > > - reqtype, value, REALTEK_USB_VENQT_CMD_IDX, > > - pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT); > > - > > - if (status == len) { /* Success this control transfer. */ > > + if (!status) { /* Success this control transfer. */ > > rtw_reset_continual_urb_error(dvobjpriv); > > if (requesttype == 0x01) > > memcpy(pdata, pIo_buf, len); > > -- > > 2.32.0 > > > > Dear Fabio, > > Thanks for the patch. Sorry, but for some reason with my N10-Nano I > can't get a connection at all with this patch applied - it just won't > associate with my network. Interface shows up and no OOPS in log, but > just disassociates/no IP address/interface down etc. so perhaps > semantics differ slightly here somehow? Tried two separate > rollbacks/builds/runs just to make sure I wasn't losing my mind :-) > > Regards, > Phil > Dear Philip, Thanks for testing. As I wrote in my RFC, I strongly suspected that I was not able to correctly understand the semantics of the new API. I'll try to read the code anew and try to understand what is wrong here. However, I also think that I won't be able to figure it out. Maybe that I have to wait for Greg to give me some hint about what are the errors in using usb_control_msg_send/recv() the way I did. Anyway, thanks a lot for the time you spent testing. Regards, Fabio ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 0:31 ` Fabio M. De Francesco @ 2021-08-24 1:38 ` Fabio M. De Francesco 2021-08-24 2:01 ` Fabio M. De Francesco 2021-08-24 21:59 ` Phillip Potter 0 siblings, 2 replies; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 1:38 UTC (permalink / raw) To: Phillip Potter Cc: Larry Finger, Greg Kroah-Hartman, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List, Pavel Skripkin On Tuesday, August 24, 2021 2:31:11 AM CEST Fabio M. De Francesco wrote: > On Tuesday, August 24, 2021 2:08:49 AM CEST Phillip Potter wrote: > > On Mon, 23 Aug 2021 at 23:38, Fabio M. De Francesco > > <fmdefrancesco@gmail.com> wrote: > > > > > > Replace usb_control_msg() with the new usb_control_msg_recv() and > > > usb_control_msg_send() API of USB Core in usbctrl_vendorreq(). > > > > > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > > --- > > > > > > Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the > > > RFC patch. > > > > > > drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++----------- > > > 1 file changed, 12 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c > > > index a93d5cfe4635..6f51660b967a 100644 > > > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c > > > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c > > > @@ -15,9 +15,8 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > > > struct adapter *adapt = pintfhdl->padapter; > > > struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt); > > > struct usb_device *udev = dvobjpriv->pusbdev; > > > - unsigned int pipe; > > > + u8 pipe; > > > int status = 0; > > > - u8 reqtype; > > > u8 *pIo_buf; > > > int vendorreq_times = 0; > > > > > > @@ -44,22 +43,22 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > > > } > > > > > > while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) { > > > - memset(pIo_buf, 0, len); > > > - > > > if (requesttype == 0x01) { > > > - pipe = usb_rcvctrlpipe(udev, 0);/* read_in */ > > > - reqtype = REALTEK_USB_VENQT_READ; > > > + status = usb_control_msg_recv(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > > > + REALTEK_USB_VENQT_READ, value, > > > + REALTEK_USB_VENQT_CMD_IDX, > > > + pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > > > + GFP_KERNEL); > > > } else { > > > - pipe = usb_sndctrlpipe(udev, 0);/* write_out */ > > > - reqtype = REALTEK_USB_VENQT_WRITE; > > > memcpy(pIo_buf, pdata, len); > > > + status = usb_control_msg_send(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > > > + REALTEK_USB_VENQT_WRITE, value, > > > + REALTEK_USB_VENQT_CMD_IDX, > > > + pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > > > + GFP_KERNEL); > > > } > > > > > > - status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > > > - reqtype, value, REALTEK_USB_VENQT_CMD_IDX, > > > - pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT); > > > - > > > - if (status == len) { /* Success this control transfer. */ > > > + if (!status) { /* Success this control transfer. */ > > > rtw_reset_continual_urb_error(dvobjpriv); > > > if (requesttype == 0x01) > > > memcpy(pdata, pIo_buf, len); > > > -- > > > 2.32.0 > > > > > > > Dear Fabio, > > > > Thanks for the patch. Sorry, but for some reason with my N10-Nano I > > can't get a connection at all with this patch applied - it just won't > > associate with my network. Interface shows up and no OOPS in log, but > > just disassociates/no IP address/interface down etc. so perhaps > > semantics differ slightly here somehow? Tried two separate > > rollbacks/builds/runs just to make sure I wasn't losing my mind :-) > > > > Regards, > > Phil > > > Dear Philip, > > Thanks for testing. As I wrote in my RFC, I strongly suspected that I was > not able to correctly understand the semantics of the new API. I'll try to > read the code anew and try to understand what is wrong here. > > However, I also think that I won't be able to figure it out. Maybe that I > have to wait for Greg to give me some hint about what are the errors in > using usb_control_msg_send/recv() the way I did. > > Anyway, thanks a lot for the time you spent testing. > > Regards, > > Fabio > Dear Philip, I think that I've inadvertently switched the order by which usb_control_msg_send() and memcpy() are called. I'm very sorry for not doing my tests, but (as I had said before) at the moment I don't have my device with me. I'm about to send a v2 series. Thanks very much for testing on my behalf. Regards, Fabio ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 1:38 ` Fabio M. De Francesco @ 2021-08-24 2:01 ` Fabio M. De Francesco 2021-08-24 5:44 ` Christophe JAILLET 2021-08-24 21:59 ` Phillip Potter 1 sibling, 1 reply; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 2:01 UTC (permalink / raw) To: Phillip Potter Cc: Larry Finger, Greg Kroah-Hartman, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List, Pavel Skripkin On Tuesday, August 24, 2021 3:38:03 AM CEST Fabio M. De Francesco wrote: > I think that I've inadvertently switched the order by which usb_control_msg_send() > and memcpy() are called. I'm very sorry for not doing my tests, but (as I had said > before) at the moment I don't have my device with me. No, I did not switch them. There must be something else... Sorry for the noise. Fabio ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 2:01 ` Fabio M. De Francesco @ 2021-08-24 5:44 ` Christophe JAILLET 2021-08-24 10:38 ` Fabio M. De Francesco 0 siblings, 1 reply; 29+ messages in thread From: Christophe JAILLET @ 2021-08-24 5:44 UTC (permalink / raw) To: Fabio M. De Francesco, Phillip Potter Cc: Larry Finger, Greg Kroah-Hartman, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List, Pavel Skripkin Le 24/08/2021 à 04:01, Fabio M. De Francesco a écrit : > On Tuesday, August 24, 2021 3:38:03 AM CEST Fabio M. De Francesco wrote: >> I think that I've inadvertently switched the order by which usb_control_msg_send() >> and memcpy() are called. I'm very sorry for not doing my tests, but (as I had said >> before) at the moment I don't have my device with me. > > No, I did not switch them. There must be something else... > Sorry for the noise. > > Fabio > Hi, 'usb_control_msg_recv()' looks like: int usb_control_msg_recv(struct usb_device *dev, __u8 endpoint, ...) { unsigned int pipe = usb_rcvctrlpipe(dev, endpoint); ... ret = usb_control_msg(dev, pipe, ...); 'usb_control_msg()' looks like: int usb_control_msg(struct usb_device *dev, unsigned int pipe, ...) { The difference is that one expect an 'endpoint' (and compute the pipe from it), and the other expect a 'pipe'. Also, in your code, 'pipe' looks un-initialized. So, my guess is that you should rename 'pipe' into 'endpoint' (to keep the semantic), have "endpoint = 0;" somewhere and pass it to usb_control_msg_{recv|send}. Or just remove 'pipe' and pass an explicit 0 directly. Not sure it is enough, but it looks like a difference between before and after your patch. just my 2c, CJ ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 5:44 ` Christophe JAILLET @ 2021-08-24 10:38 ` Fabio M. De Francesco 2021-08-24 17:03 ` Christophe JAILLET 0 siblings, 1 reply; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 10:38 UTC (permalink / raw) To: Phillip Potter, Christophe JAILLET Cc: Larry Finger, Greg Kroah-Hartman, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List, Pavel Skripkin On Tuesday, August 24, 2021 7:44:40 AM CEST Christophe JAILLET wrote: > Le 24/08/2021 à 04:01, Fabio M. De Francesco a écrit : > > On Tuesday, August 24, 2021 3:38:03 AM CEST Fabio M. De Francesco wrote: > >> I think that I've inadvertently switched the order by which usb_control_msg_send() > >> and memcpy() are called. I'm very sorry for not doing my tests, but (as I had said > >> before) at the moment I don't have my device with me. > > > > No, I did not switch them. There must be something else... > > Sorry for the noise. > > > > Fabio > > > > Hi, > > 'usb_control_msg_recv()' looks like: > > int usb_control_msg_recv(struct usb_device *dev, __u8 endpoint, ...) > { > unsigned int pipe = usb_rcvctrlpipe(dev, endpoint); > ... > ret = usb_control_msg(dev, pipe, ...); > > > 'usb_control_msg()' looks like: > int usb_control_msg(struct usb_device *dev, unsigned int pipe, ...) > { > > The difference is that one expect an 'endpoint' (and compute the pipe > from it), and the other expect a 'pipe'. Hi Christophe, Yes, correct. That's why I changed the type of 'pipe' from "unsigned int" to "u8". I also saw that usb_control_msg_recv/send take care of calling usb_rcvctrpipe() and usb_sndctrlpipe(); so, in my patch I deleted those calls. Not related to my patch... why Linux has u8 and __u8? What are the different use cases they are meant for? > Also, in your code, 'pipe' looks un-initialized. Oh yes, good catch. Thanks! > So, my guess is that you should rename 'pipe' into 'endpoint' (to keep > the semantic), > have "endpoint = 0;" somewhere and pass it to > usb_control_msg_{recv|send}. > Or just remove 'pipe' and pass an explicit 0 directly. I've just seen that in other drivers the code passes an explicit 0. So, also according to your suggestion, I'll remove "pipe/endpoint". > Not sure it is enough, but it looks like a difference between before and > after your patch. Since I cannot see other issues, I'm about to fix the code as said above and then submit a v2 series. Your 2c are worth much more than how much you think :) Thanks very much, Fabio > just my 2c, > CJ > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 10:38 ` Fabio M. De Francesco @ 2021-08-24 17:03 ` Christophe JAILLET 0 siblings, 0 replies; 29+ messages in thread From: Christophe JAILLET @ 2021-08-24 17:03 UTC (permalink / raw) To: Fabio M. De Francesco, Phillip Potter Cc: Larry Finger, Greg Kroah-Hartman, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List, Pavel Skripkin Le 24/08/2021 à 12:38, Fabio M. De Francesco a écrit : > Not related to my patch... why Linux has u8 and __u8? What are the > different use cases they are meant for? Maybe: https://elixir.bootlin.com/linux/v5.14-rc6/source/include/uapi/asm-generic/int-l64.h#L16 helps ? > Your 2c are worth much more than how much you think :) If you insist, I could send you my Paypal address, but knowing that it may help someone should already be enough for me :). Let me know if it was the root cause of the issue. CJ > > Thanks very much, > > Fabio > >> just my 2c, >> CJ >> > > > > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 1:38 ` Fabio M. De Francesco 2021-08-24 2:01 ` Fabio M. De Francesco @ 2021-08-24 21:59 ` Phillip Potter 1 sibling, 0 replies; 29+ messages in thread From: Phillip Potter @ 2021-08-24 21:59 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Larry Finger, Greg Kroah-Hartman, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List, Pavel Skripkin On Tue, 24 Aug 2021 at 02:38, Fabio M. De Francesco <fmdefrancesco@gmail.com> wrote: > > Dear Philip, > > I think that I've inadvertently switched the order by which usb_control_msg_send() > and memcpy() are called. I'm very sorry for not doing my tests, but (as I had said > before) at the moment I don't have my device with me. > > I'm about to send a v2 series. > > Thanks very much for testing on my behalf. > > Regards, > > Fabio > > > Dear Fabio, Just going through my e-mails now after the work day, so sorry for not replying sooner. Don't feel you have to apologise for things like this honestly, we are all human and there is a huge amount of churn on this driver at the moment. I know there are subsequent e-mails indicating this was not the problem anyway, but in any case, I am happy to test when I'm able to, and don't worry if it doesn't always work - my code is guilty of the same thing constantly :-) Regards, Phil ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-23 22:37 ` [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() Fabio M. De Francesco 2021-08-24 0:08 ` Phillip Potter @ 2021-08-24 8:13 ` Pavel Skripkin 2021-08-24 8:53 ` Fabio M. De Francesco 1 sibling, 1 reply; 29+ messages in thread From: Pavel Skripkin @ 2021-08-24 8:13 UTC (permalink / raw) To: Fabio M. De Francesco, Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel On 8/24/21 1:37 AM, 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(). > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > > Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the > RFC patch. > > drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++----------- > 1 file changed, 12 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c > index a93d5cfe4635..6f51660b967a 100644 > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c > @@ -15,9 +15,8 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > struct adapter *adapt = pintfhdl->padapter; > struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt); > struct usb_device *udev = dvobjpriv->pusbdev; > - unsigned int pipe; > + u8 pipe; > int status = 0; > - u8 reqtype; > u8 *pIo_buf; > int vendorreq_times = 0; > > @@ -44,22 +43,22 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > } > > while (++vendorreq_times <= MAX_USBCTRL_VENDORREQ_TIMES) { > - memset(pIo_buf, 0, len); > - > if (requesttype == 0x01) { > - pipe = usb_rcvctrlpipe(udev, 0);/* read_in */ > - reqtype = REALTEK_USB_VENQT_READ; > + status = usb_control_msg_recv(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > + REALTEK_USB_VENQT_READ, value, > + REALTEK_USB_VENQT_CMD_IDX, > + pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > + GFP_KERNEL); > } else { > - pipe = usb_sndctrlpipe(udev, 0);/* write_out */ > - reqtype = REALTEK_USB_VENQT_WRITE; > memcpy(pIo_buf, pdata, len); > + status = usb_control_msg_send(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > + REALTEK_USB_VENQT_WRITE, value, > + REALTEK_USB_VENQT_CMD_IDX, > + pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > + GFP_KERNEL); > } > > - status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > - reqtype, value, REALTEK_USB_VENQT_CMD_IDX, > - pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT); > - > - if (status == len) { /* Success this control transfer. */ > + if (!status) { /* Success this control transfer. */ > rtw_reset_continual_urb_error(dvobjpriv); > if (requesttype == 0x01) > memcpy(pdata, pIo_buf, len); > Hi, Fabio! Christophe is right about semantic part. Also, if (!status) { } else { if (status < 0) { <- | } else { | | } <- } Extra if-else is not needed, since status can be 0 and < 0, there is no 3rd state, like it was before. With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 8:13 ` Pavel Skripkin @ 2021-08-24 8:53 ` Fabio M. De Francesco 2021-08-24 11:07 ` Pavel Skripkin 0 siblings, 1 reply; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 8:53 UTC (permalink / raw) To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Pavel Skripkin On Tuesday, August 24, 2021 10:13:46 AM CEST Pavel Skripkin wrote: > On 8/24/21 1:37 AM, 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(). > > > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > --- > > > > Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the > > RFC patch. > > > > drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++----------- > > 1 file changed, 12 insertions(+), 13 deletions(-) > > > > [...] > > > Hi, Fabio! > > Christophe is right about semantic part. Hi Pavel, I haven't yet read Christophe's message (but I'm going to do it ASAP). I hope he found out what is wrong with the code, what made Phil's tests fail. > Also, > > if (!status) { > > } else { > if (status < 0) { <- > | > } else { | > | > } <- > } > > Extra if-else is not needed, since status can be 0 and < 0, there is no > 3rd state, like it was before. Correct, thanks! Now I read the following from the documentation of the new API... "Return: If successful, 0 is returned, Otherwise, a negative error number." I'll remove that status < 0 check and whatever else is no more necessary. Thanks, again :) Regards, Fabio > With regards, > Pavel Skripkin > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 8:53 ` Fabio M. De Francesco @ 2021-08-24 11:07 ` Pavel Skripkin 2021-08-24 12:01 ` Fabio M. De Francesco 0 siblings, 1 reply; 29+ messages in thread From: Pavel Skripkin @ 2021-08-24 11:07 UTC (permalink / raw) To: Fabio M. De Francesco, Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel On 8/24/21 11:53 AM, Fabio M. De Francesco wrote: > On Tuesday, August 24, 2021 10:13:46 AM CEST Pavel Skripkin wrote: >> On 8/24/21 1:37 AM, 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(). >> > >> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> >> > --- >> > >> > Thanks to Pavel Skripkin <paskripkin@gmail.com> for his review of the >> > RFC patch. >> > >> > drivers/staging/r8188eu/hal/usb_ops_linux.c | 25 ++++++++++----------- >> > 1 file changed, 12 insertions(+), 13 deletions(-) >> > >> > [...] >> > >> Hi, Fabio! >> >> Christophe is right about semantic part. > > Hi Pavel, > > I haven't yet read Christophe's message (but I'm going to do it ASAP). > I hope he found out what is wrong with the code, what made Phil's tests > fail. > >> Also, >> >> if (!status) { >> >> } else { >> if (status < 0) { <- >> | >> } else { | >> | >> } <- >> } >> >> Extra if-else is not needed, since status can be 0 and < 0, there is no >> 3rd state, like it was before. > > Correct, thanks! > > Now I read the following from the documentation of the new API... > > "Return: If successful, 0 is returned, Otherwise, a negative error number." > > I'll remove that status < 0 check and whatever else is no more necessary. > Thanks, again :) > > Regards, > Btw, not related to your patch, but I start think, that this check: if (!pIo_buf) { DBG_88E("[%s] pIo_buf == NULL\n", __func__); status = -ENOMEM; goto release_mutex; } Should be wrapped as if (WARN_ON(unlikely(!pIo_buf)) { ... } Since usb_vendor_req_buf is initialized in ->probe() and I can't see possible calltrace, which can cause zeroing this pointer. Something _completely_ wrong is going on if usb_vendor_req_buf is NULL, and we should complain loud about it. What do you think? With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 11:07 ` Pavel Skripkin @ 2021-08-24 12:01 ` Fabio M. De Francesco 2021-08-24 12:09 ` Pavel Skripkin 0 siblings, 1 reply; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 12:01 UTC (permalink / raw) To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Pavel Skripkin On Tuesday, August 24, 2021 1:07:46 PM CEST Pavel Skripkin wrote: > > Btw, not related to your patch, but I start think, that this check: > > > if (!pIo_buf) { > DBG_88E("[%s] pIo_buf == NULL\n", __func__); > status = -ENOMEM; > goto release_mutex; > } > > Should be wrapped as > > if (WARN_ON(unlikely(!pIo_buf)) { > ... > } > > Since usb_vendor_req_buf is initialized in ->probe() and I can't see > possible calltrace, which can cause zeroing this pointer. I see that usb_vendor_req_buf is initialized in rtw_init_intf_priv(). It depends on a kzalloc() success on allocating memory. Obviously it could fail to allocate. If it fails, rtw_init_intf_priv() returns _FAIL to its caller(s) (whichever they are - I didn't go too deep in understanding the possible calls chains). What does really matter is that dvobjpriv->usb_vendor_req_buf _could_ _indeed_ _be_ in an _un-initialized_ _status_ when it is assigned to pIo_buf. > Something _completely_ wrong is going on if usb_vendor_req_buf is NULL, > and we should complain loud about it. What do you think? That "if (!pIo_buf)" in usbctrl_vendorreq() manages a possible but remote (I guess) un-initialization by releasing a mutex and returning -ENOMEM. I think that technically speaking it would suffice if callers read and manage properly the -ENOMEM returned by usbctrl_vendorreq(). Said that, I have no sufficient experience to say if exiting and returning -ENOMEM would suffice to not make happen "_something _completely_ wrong_" or if a WARN_ON is required. I'm sorry for not being able to go beyond the confirmation that indeed dvobjpriv->usb_vendor_req could be un-initialized. Regards, Fabio > With regards, > Pavel Skripkin > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 12:01 ` Fabio M. De Francesco @ 2021-08-24 12:09 ` Pavel Skripkin 2021-08-24 14:55 ` Fabio M. De Francesco 0 siblings, 1 reply; 29+ messages in thread From: Pavel Skripkin @ 2021-08-24 12:09 UTC (permalink / raw) To: Fabio M. De Francesco, Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel On 8/24/21 3:01 PM, Fabio M. De Francesco wrote: > On Tuesday, August 24, 2021 1:07:46 PM CEST Pavel Skripkin wrote: >> >> Btw, not related to your patch, but I start think, that this check: >> >> >> if (!pIo_buf) { >> DBG_88E("[%s] pIo_buf == NULL\n", __func__); >> status = -ENOMEM; >> goto release_mutex; >> } >> >> Should be wrapped as >> >> if (WARN_ON(unlikely(!pIo_buf)) { >> ... >> } >> >> Since usb_vendor_req_buf is initialized in ->probe() and I can't see >> possible calltrace, which can cause zeroing this pointer. > > I see that usb_vendor_req_buf is initialized in rtw_init_intf_priv(). It depends on a > kzalloc() success on allocating memory. Obviously it could fail to allocate. If it fails, > rtw_init_intf_priv() returns _FAIL to its caller(s) (whichever they are - I didn't go too > deep in understanding the possible calls chains). > Call chain is the most interesting part here :) rtw_drv_init() <-- probe() usb_dvobj_init() rtw_init_intf_priv() If kzalloc fails, then whole ->probe() routine fails, i.e device will be disconnected. There is no read() calls before rtw_init_intf_priv(), so if kzalloc() call was successful, there is no way how usb_vendor_req_buf can be NULL, since read() can happen only in case of successfully connected device. Anyway, it can be NULL in case of out-of-bound write or smth else, but there is no explicit usb_alloc_vendor_req_buf = NULL in this driver. We should complain about completely wrong driver behavior, IMO :) Does it make sense? With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() 2021-08-24 12:09 ` Pavel Skripkin @ 2021-08-24 14:55 ` Fabio M. De Francesco 0 siblings, 0 replies; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 14:55 UTC (permalink / raw) To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Pavel Skripkin On Tuesday, August 24, 2021 2:09:10 PM CEST Pavel Skripkin wrote: > On 8/24/21 3:01 PM, Fabio M. De Francesco wrote: > > On Tuesday, August 24, 2021 1:07:46 PM CEST Pavel Skripkin wrote: > >> > >> Btw, not related to your patch, but I start think, that this check: > >> > >> > >> if (!pIo_buf) { > >> DBG_88E("[%s] pIo_buf == NULL\n", __func__); > >> status = -ENOMEM; > >> goto release_mutex; > >> } > >> > >> Should be wrapped as > >> > >> if (WARN_ON(unlikely(!pIo_buf)) { > >> ... > >> } > >> > >> Since usb_vendor_req_buf is initialized in ->probe() and I can't see > >> possible calltrace, which can cause zeroing this pointer. > > > > I see that usb_vendor_req_buf is initialized in rtw_init_intf_priv(). It depends on a > > kzalloc() success on allocating memory. Obviously it could fail to allocate. If it fails, > > rtw_init_intf_priv() returns _FAIL to its caller(s) (whichever they are - I didn't go too > > deep in understanding the possible calls chains). > > > > Call chain is the most interesting part here :) > > rtw_drv_init() <-- probe() > usb_dvobj_init() > rtw_init_intf_priv() > > If kzalloc fails, then whole ->probe() routine fails, i.e device will be > disconnected. I guess that if probe fails and then the device get disconnected it's not a big problem, in the sense that nothing of very bad could happen. > There is no read() calls before rtw_init_intf_priv(), so > if kzalloc() call was successful, there is no way how usb_vendor_req_buf > can be NULL, since read() can happen only in case of successfully > connected device. Yes, though I have very little knowledge of how drivers work, it make sense to me too that read(s) can happen only in case of successful connection. > Anyway, it can be NULL in case of out-of-bound write or smth else, This is really something I don't know. > but > there is no explicit usb_alloc_vendor_req_buf = NULL in this driver. > We should complain about completely wrong driver behavior, IMO :) > > Does it make sense? I'm not sure, whether or not we now have an answer to your question about the necessity to use WARN_ON... I think it's up to your judgement, because I cannot help on this topic :( Regards, Fabio > With regards, > Pavel Skripkin > ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-23 22:37 [PATCH 0/2] staging: r8188eu: Use new usb_control_msg_recv/send() Fabio M. De Francesco 2021-08-23 22:37 ` [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() Fabio M. De Francesco @ 2021-08-23 22:37 ` Fabio M. De Francesco 2021-08-24 0:10 ` Phillip Potter 1 sibling, 1 reply; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-23 22:37 UTC (permalink / raw) To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Pavel Skripkin Cc: Fabio M. De Francesco After replacing usb_control_msg() with the new usb_control_msg_recv() and usb_control_msg_send() API of USB Core, remove camelcase from the pIo_buf variable that is passed as argument to the new API and remove the initial 'p' (that probably stands for "pointer") from the same pIo_buf and from the pintfhdl and pdata arguments of usbctrl_vendorreq(). Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- drivers/staging/r8188eu/hal/usb_ops_linux.c | 26 ++++++++++----------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c index 6f51660b967a..5ce31ae18ed8 100644 --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c @@ -10,14 +10,14 @@ #include "../include/recv_osdep.h" #include "../include/rtl8188e_hal.h" -static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, u16 len, u8 requesttype) +static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u16 len, u8 requesttype) { - struct adapter *adapt = pintfhdl->padapter; + struct adapter *adapt = intfhdl->padapter; struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt); struct usb_device *udev = dvobjpriv->pusbdev; u8 pipe; int status = 0; - u8 *pIo_buf; + u8 *io_buf; int vendorreq_times = 0; if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) { @@ -34,10 +34,10 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, mutex_lock(&dvobjpriv->usb_vendor_req_mutex); /* Acquire IO memory for vendorreq */ - pIo_buf = dvobjpriv->usb_vendor_req_buf; + io_buf = dvobjpriv->usb_vendor_req_buf; - if (!pIo_buf) { - DBG_88E("[%s] pIo_buf == NULL\n", __func__); + if (!io_buf) { + DBG_88E("[%s] io_buf == NULL\n", __func__); status = -ENOMEM; goto release_mutex; } @@ -47,25 +47,25 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, status = usb_control_msg_recv(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, REALTEK_USB_VENQT_READ, value, REALTEK_USB_VENQT_CMD_IDX, - pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, + io_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, GFP_KERNEL); } else { - memcpy(pIo_buf, pdata, len); + memcpy(io_buf, data, len); status = usb_control_msg_send(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, REALTEK_USB_VENQT_WRITE, value, REALTEK_USB_VENQT_CMD_IDX, - pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, + io_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, GFP_KERNEL); } if (!status) { /* Success this control transfer. */ rtw_reset_continual_urb_error(dvobjpriv); if (requesttype == 0x01) - memcpy(pdata, pIo_buf, len); + memcpy(data, io_buf, len); } else { /* error cases */ DBG_88E("reg 0x%x, usb %s %u fail, status:%d value=0x%x, vendorreq_times:%d\n", value, (requesttype == 0x01) ? "read" : "write", - len, status, *(u32 *)pdata, vendorreq_times); + len, status, *(u32 *)data, vendorreq_times); if (status < 0) { if (status == (-ESHUTDOWN) || status == -ENODEV) { @@ -77,8 +77,8 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, } else { /* status != len && status >= 0 */ if (status > 0) { if (requesttype == 0x01) { - /* For Control read transfer, we have to copy the read data from pIo_buf to pdata. */ - memcpy(pdata, pIo_buf, len); + /* For Control read transfer, we have to copy the read data from io_buf to data. */ + memcpy(data, io_buf, len); } } } -- 2.32.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-23 22:37 ` [PATCH 2/2] staging: r8188eu: Make some clean-ups " Fabio M. De Francesco @ 2021-08-24 0:10 ` Phillip Potter 0 siblings, 0 replies; 29+ messages in thread From: Phillip Potter @ 2021-08-24 0:10 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Larry Finger, Greg Kroah-Hartman, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List, Pavel Skripkin On Mon, 23 Aug 2021 at 23:38, Fabio M. De Francesco <fmdefrancesco@gmail.com> wrote: > > After replacing usb_control_msg() with the new usb_control_msg_recv() and > usb_control_msg_send() API of USB Core, remove camelcase from the pIo_buf > variable that is passed as argument to the new API and remove the initial > 'p' (that probably stands for "pointer") from the same pIo_buf and from > the pintfhdl and pdata arguments of usbctrl_vendorreq(). > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > drivers/staging/r8188eu/hal/usb_ops_linux.c | 26 ++++++++++----------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c > index 6f51660b967a..5ce31ae18ed8 100644 > --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c > +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c > @@ -10,14 +10,14 @@ > #include "../include/recv_osdep.h" > #include "../include/rtl8188e_hal.h" > > -static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, u16 len, u8 requesttype) > +static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u16 len, u8 requesttype) > { > - struct adapter *adapt = pintfhdl->padapter; > + struct adapter *adapt = intfhdl->padapter; > struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt); > struct usb_device *udev = dvobjpriv->pusbdev; > u8 pipe; > int status = 0; > - u8 *pIo_buf; > + u8 *io_buf; > int vendorreq_times = 0; > > if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) { > @@ -34,10 +34,10 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > mutex_lock(&dvobjpriv->usb_vendor_req_mutex); > > /* Acquire IO memory for vendorreq */ > - pIo_buf = dvobjpriv->usb_vendor_req_buf; > + io_buf = dvobjpriv->usb_vendor_req_buf; > > - if (!pIo_buf) { > - DBG_88E("[%s] pIo_buf == NULL\n", __func__); > + if (!io_buf) { > + DBG_88E("[%s] io_buf == NULL\n", __func__); > status = -ENOMEM; > goto release_mutex; > } > @@ -47,25 +47,25 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > status = usb_control_msg_recv(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > REALTEK_USB_VENQT_READ, value, > REALTEK_USB_VENQT_CMD_IDX, > - pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > + io_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > GFP_KERNEL); > } else { > - memcpy(pIo_buf, pdata, len); > + memcpy(io_buf, data, len); > status = usb_control_msg_send(udev, pipe, REALTEK_USB_VENQT_CMD_REQ, > REALTEK_USB_VENQT_WRITE, value, > REALTEK_USB_VENQT_CMD_IDX, > - pIo_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > + io_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, > GFP_KERNEL); > } > > if (!status) { /* Success this control transfer. */ > rtw_reset_continual_urb_error(dvobjpriv); > if (requesttype == 0x01) > - memcpy(pdata, pIo_buf, len); > + memcpy(data, io_buf, len); > } else { /* error cases */ > DBG_88E("reg 0x%x, usb %s %u fail, status:%d value=0x%x, vendorreq_times:%d\n", > value, (requesttype == 0x01) ? "read" : "write", > - len, status, *(u32 *)pdata, vendorreq_times); > + len, status, *(u32 *)data, vendorreq_times); > > if (status < 0) { > if (status == (-ESHUTDOWN) || status == -ENODEV) { > @@ -77,8 +77,8 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, > } else { /* status != len && status >= 0 */ > if (status > 0) { > if (requesttype == 0x01) { > - /* For Control read transfer, we have to copy the read data from pIo_buf to pdata. */ > - memcpy(pdata, pIo_buf, len); > + /* For Control read transfer, we have to copy the read data from io_buf to data. */ > + memcpy(data, io_buf, len); > } > } > } > -- > 2.32.0 > Dear Fabio, This one doesn't apply at all for me - it may just be because of the churn in my local tree though, I don't have all previous patches applied as I've been testing select series etc. Many thanks. Regards, Phil ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 0/2] staging: r8188eu: Use new usb_control_msg_recv/send()
@ 2021-08-24 14:28 Fabio M. De Francesco
2021-08-24 14:28 ` [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() Fabio M. De Francesco
0 siblings, 1 reply; 29+ messages in thread
From: Fabio M. De Francesco @ 2021-08-24 14:28 UTC (permalink / raw)
To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging,
linux-kernel, Pavel Skripkin, Christophe JAILLET
Cc: Fabio M. De Francesco
Replace usb_control_msg() with the new usb_control_msg_recv() and
usb_control_msg_send() API of USB Core in usbctrl_vendorreq().
After replacing usb_control_msg() with the new usb_control_msg_recv() and
usb_control_msg_send() API of USB Core, remove camelcase from the pIo_buf
variable that is passed as argument to the new API and remove the initial
'p' (that probably stands for "pointer") from the same pIo_buf and from
the pintfhdl and pdata arguments of usbctrl_vendorreq().
Fabio M. De Francesco (2):
staging: r8188eu: Use usb_control_msg_recv/send() in
usbctrl_vendorreq()
staging: r8188eu: Make some clean-ups in usbctrl_vendorreq()
drivers/staging/r8188eu/hal/usb_ops_linux.c | 65 +++++++++------------
1 file changed, 27 insertions(+), 38 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 14:28 [PATCH v2 0/2] staging: r8188eu: Use new usb_control_msg_recv/send() Fabio M. De Francesco @ 2021-08-24 14:28 ` Fabio M. De Francesco 2021-08-24 14:39 ` Pavel Skripkin 0 siblings, 1 reply; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 14:28 UTC (permalink / raw) To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Pavel Skripkin, Christophe JAILLET Cc: Fabio M. De Francesco After replacing usb_control_msg() with the new usb_control_msg_recv() and usb_control_msg_send() API of USB Core, remove camelcase from the pIo_buf variable that is passed as argument to the new API and remove the initial 'p' (that probably stands for "pointer") from the same pIo_buf and from the pintfhdl and pdata arguments of usbctrl_vendorreq(). Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- drivers/staging/r8188eu/hal/usb_ops_linux.c | 22 ++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c index 13e925d21e00..a038aaa5a624 100644 --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c @@ -10,13 +10,13 @@ #include "../include/recv_osdep.h" #include "../include/rtl8188e_hal.h" -static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, u16 len, u8 requesttype) +static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u16 len, u8 requesttype) { - struct adapter *adapt = pintfhdl->padapter; + struct adapter *adapt = intfhdl->padapter; struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt); struct usb_device *udev = dvobjpriv->pusbdev; int status = 0; - u8 *pIo_buf; + u8 *io_buf; int vendorreq_times = 0; if ((adapt->bSurpriseRemoved) || (adapt->pwrctrlpriv.pnp_bstop_trx)) { @@ -33,10 +33,10 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, mutex_lock(&dvobjpriv->usb_vendor_req_mutex); /* Acquire IO memory for vendorreq */ - pIo_buf = dvobjpriv->usb_vendor_req_buf; + io_buf = dvobjpriv->usb_vendor_req_buf; - if (!pIo_buf) { - DBG_88E("[%s] pIo_buf == NULL\n", __func__); + if (!io_buf) { + DBG_88E("[%s] io_buf == NULL\n", __func__); status = -ENOMEM; goto release_mutex; } @@ -45,14 +45,14 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, if (requesttype == 0x01) { status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ, REALTEK_USB_VENQT_READ, value, - REALTEK_USB_VENQT_CMD_IDX, pIo_buf, + REALTEK_USB_VENQT_CMD_IDX, io_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, GFP_KERNEL); } else { - memcpy(pIo_buf, pdata, len); + memcpy(io_buf, data, len); status = usb_control_msg_send(udev, 0, REALTEK_USB_VENQT_CMD_REQ, REALTEK_USB_VENQT_WRITE, value, - REALTEK_USB_VENQT_CMD_IDX, pIo_buf, + REALTEK_USB_VENQT_CMD_IDX, io_buf, len, RTW_USB_CONTROL_MSG_TIMEOUT, GFP_KERNEL); } @@ -60,11 +60,11 @@ static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void *pdata, if (!status) { /* Success this control transfer. */ rtw_reset_continual_urb_error(dvobjpriv); if (requesttype == 0x01) - memcpy(pdata, pIo_buf, len); + memcpy(data, io_buf, len); } else { /* error cases */ DBG_88E("reg 0x%x, usb %s %u fail, status:%d value=0x%x, vendorreq_times:%d\n", value, (requesttype == 0x01) ? "read" : "write", - len, status, *(u32 *)pdata, vendorreq_times); + len, status, *(u32 *)data, vendorreq_times); if (status == (-ESHUTDOWN) || status == -ENODEV) { adapt->bSurpriseRemoved = true; -- 2.32.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 14:28 ` [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() Fabio M. De Francesco @ 2021-08-24 14:39 ` Pavel Skripkin 2021-08-24 15:15 ` Fabio M. De Francesco 0 siblings, 1 reply; 29+ messages in thread From: Pavel Skripkin @ 2021-08-24 14:39 UTC (permalink / raw) To: Fabio M. De Francesco, Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Christophe JAILLET On 8/24/21 5:28 PM, Fabio M. De Francesco wrote: > After replacing usb_control_msg() with the new usb_control_msg_recv() and > usb_control_msg_send() API of USB Core, remove camelcase from the pIo_buf > variable that is passed as argument to the new API and remove the initial > 'p' (that probably stands for "pointer") from the same pIo_buf and from > the pintfhdl and pdata arguments of usbctrl_vendorreq(). > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > drivers/staging/r8188eu/hal/usb_ops_linux.c | 22 ++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > I cannot apply this one on top of the first one: error: patch failed: drivers/staging/r8188eu/hal/usb_ops_linux.c:33 error: drivers/staging/r8188eu/hal/usb_ops_linux.c: patch does not apply With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 14:39 ` Pavel Skripkin @ 2021-08-24 15:15 ` Fabio M. De Francesco 2021-08-24 15:26 ` Pavel Skripkin 0 siblings, 1 reply; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 15:15 UTC (permalink / raw) To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Christophe JAILLET, Pavel Skripkin On Tuesday, August 24, 2021 4:39:51 PM CEST Pavel Skripkin wrote: > On 8/24/21 5:28 PM, Fabio M. De Francesco wrote: > > After replacing usb_control_msg() with the new usb_control_msg_recv() and > > usb_control_msg_send() API of USB Core, remove camelcase from the pIo_buf > > variable that is passed as argument to the new API and remove the initial > > 'p' (that probably stands for "pointer") from the same pIo_buf and from > > the pintfhdl and pdata arguments of usbctrl_vendorreq(). > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > --- > > drivers/staging/r8188eu/hal/usb_ops_linux.c | 22 ++++++++++----------- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > I cannot apply this one on top of the first one: > > error: patch failed: drivers/staging/r8188eu/hal/usb_ops_linux.c:33 > error: drivers/staging/r8188eu/hal/usb_ops_linux.c: patch does not apply > > With regards, > Pavel Skripkin This is the same problem that yesterday Philip had. I cannot understand why it can happen, because I've worked on this soon after 1/2 and in the while Greg didn't apply nothing. I've only worked on one function both in 1/2 and in 2/2 and I would expect that either both of them apply or none of them. What am I missing? Thanks, Fabio ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 15:15 ` Fabio M. De Francesco @ 2021-08-24 15:26 ` Pavel Skripkin 2021-08-24 15:39 ` Fabio M. De Francesco 0 siblings, 1 reply; 29+ messages in thread From: Pavel Skripkin @ 2021-08-24 15:26 UTC (permalink / raw) To: Fabio M. De Francesco, Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Christophe JAILLET On 8/24/21 6:15 PM, Fabio M. De Francesco wrote: > On Tuesday, August 24, 2021 4:39:51 PM CEST Pavel Skripkin wrote: >> On 8/24/21 5:28 PM, Fabio M. De Francesco wrote: >> > After replacing usb_control_msg() with the new usb_control_msg_recv() and >> > usb_control_msg_send() API of USB Core, remove camelcase from the pIo_buf >> > variable that is passed as argument to the new API and remove the initial >> > 'p' (that probably stands for "pointer") from the same pIo_buf and from >> > the pintfhdl and pdata arguments of usbctrl_vendorreq(). >> > >> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> >> > --- >> > drivers/staging/r8188eu/hal/usb_ops_linux.c | 22 ++++++++++----------- >> > 1 file changed, 11 insertions(+), 11 deletions(-) >> > >> >> I cannot apply this one on top of the first one: >> >> error: patch failed: drivers/staging/r8188eu/hal/usb_ops_linux.c:33 >> error: drivers/staging/r8188eu/hal/usb_ops_linux.c: patch does not apply >> >> With regards, >> Pavel Skripkin > I found the problem: > mutex_lock(&dvobjpriv->usb_vendor_req_mutex); > > /* Acquire IO memory for vendorreq */ > - pIo_buf = dvobjpriv->usb_vendor_req_buf; > + io_buf = dvobjpriv->usb_vendor_req_buf; I don't know from where mutex_lock() comes from. In staging-next I have _enter_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL); instead of mutex_lock(&dvobjpriv->usb_vendor_req_mutex); With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 15:26 ` Pavel Skripkin @ 2021-08-24 15:39 ` Fabio M. De Francesco 2021-08-24 15:43 ` Pavel Skripkin 0 siblings, 1 reply; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 15:39 UTC (permalink / raw) To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Christophe JAILLET, Pavel Skripkin On Tuesday, August 24, 2021 5:26:23 PM CEST Pavel Skripkin wrote: > I found the problem: > > > mutex_lock(&dvobjpriv->usb_vendor_req_mutex); > > > > /* Acquire IO memory for vendorreq */ > > - pIo_buf = dvobjpriv->usb_vendor_req_buf; > > + io_buf = dvobjpriv->usb_vendor_req_buf; > > > I don't know from where mutex_lock() comes from. In staging-next I have > > _enter_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL); > > instead of > > mutex_lock(&dvobjpriv->usb_vendor_req_mutex); > Oh, I know where it comes from... :) It's a patch of mine that is in the queue, waiting to be reviewed and applied. Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/ Thanks, Fabio > > With regards, > Pavel Skripkin > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 15:39 ` Fabio M. De Francesco @ 2021-08-24 15:43 ` Pavel Skripkin 2021-08-24 15:59 ` Fabio M. De Francesco 0 siblings, 1 reply; 29+ messages in thread From: Pavel Skripkin @ 2021-08-24 15:43 UTC (permalink / raw) To: Fabio M. De Francesco, Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Christophe JAILLET On 8/24/21 6:39 PM, Fabio M. De Francesco wrote: > On Tuesday, August 24, 2021 5:26:23 PM CEST Pavel Skripkin wrote: >> I found the problem: >> >> > mutex_lock(&dvobjpriv->usb_vendor_req_mutex); >> > >> > /* Acquire IO memory for vendorreq */ >> > - pIo_buf = dvobjpriv->usb_vendor_req_buf; >> > + io_buf = dvobjpriv->usb_vendor_req_buf; >> >> >> I don't know from where mutex_lock() comes from. In staging-next I have >> >> _enter_critical_mutex(&dvobjpriv->usb_vendor_req_mutex, NULL); >> >> instead of >> >> mutex_lock(&dvobjpriv->usb_vendor_req_mutex); >> > Oh, I know where it comes from... :) > > It's a patch of mine that is in the queue, waiting to be reviewed and applied. > Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/ > oh.... there are _a lot_ of pending changes :) I guess, we need smth like public-mirror with already reviewed and working changes With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 15:43 ` Pavel Skripkin @ 2021-08-24 15:59 ` Fabio M. De Francesco 2021-08-24 16:04 ` Pavel Skripkin 0 siblings, 1 reply; 29+ messages in thread From: Fabio M. De Francesco @ 2021-08-24 15:59 UTC (permalink / raw) To: Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Christophe JAILLET, Pavel Skripkin On Tuesday, August 24, 2021 5:43:26 PM CEST Pavel Skripkin wrote: > On 8/24/21 6:39 PM, Fabio M. De Francesco wrote: > > Oh, I know where it comes from... :) > > > > It's a patch of mine that is in the queue, waiting to be reviewed and applied. > > Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/ > > > oh.... there are _a lot_ of pending changes :) > > I guess, we need smth like public-mirror with already reviewed and > working changes It's becoming a serious problem. A lot of times I see people who is asked to rebase and resend, not because they forget to fetch the current tree, instead because the tree changes as soon as Greg start to apply the first patches in the queue and the other patches at the end of the queue cannot be applied. Anyway,I understand that Greg cannot apply a patch at a time soon after submission but in the while the queue grows larger and larger. Regards, Fabio > With regards, > Pavel Skripkin > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 15:59 ` Fabio M. De Francesco @ 2021-08-24 16:04 ` Pavel Skripkin 2021-08-24 16:18 ` Greg Kroah-Hartman 0 siblings, 1 reply; 29+ messages in thread From: Pavel Skripkin @ 2021-08-24 16:04 UTC (permalink / raw) To: Fabio M. De Francesco, Larry Finger, Phillip Potter, Greg Kroah-Hartman, linux-staging, linux-kernel, Christophe JAILLET On 8/24/21 6:59 PM, Fabio M. De Francesco wrote: > On Tuesday, August 24, 2021 5:43:26 PM CEST Pavel Skripkin wrote: >> On 8/24/21 6:39 PM, Fabio M. De Francesco wrote: >> > Oh, I know where it comes from... :) >> > >> > It's a patch of mine that is in the queue, waiting to be reviewed and applied. >> > Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/ >> > >> oh.... there are _a lot_ of pending changes :) >> >> I guess, we need smth like public-mirror with already reviewed and >> working changes > > It's becoming a serious problem. A lot of times I see people who is asked to > rebase and resend, not because they forget to fetch the current tree, instead > because the tree changes as soon as Greg start to apply the first patches in the > queue and the other patches at the end of the queue cannot be applied. > > Anyway,I understand that Greg cannot apply a patch at a time soon after > submission but in the while the queue grows larger and larger. > It can be easily fixed. We need public fork somewhere (github, git.kernel.org ...) and we should ask Greg to add remote-branch to his tree. Then one of the maintainers/reviewers should accept patches to this fork + send pull requests every week (I guess). I can help with picking up and testing after I receive my device and set up qemu environment for testing :) With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 16:04 ` Pavel Skripkin @ 2021-08-24 16:18 ` Greg Kroah-Hartman 2021-08-24 16:24 ` Pavel Skripkin 2021-08-24 22:20 ` Phillip Potter 0 siblings, 2 replies; 29+ messages in thread From: Greg Kroah-Hartman @ 2021-08-24 16:18 UTC (permalink / raw) To: Pavel Skripkin Cc: Fabio M. De Francesco, Larry Finger, Phillip Potter, linux-staging, linux-kernel, Christophe JAILLET On Tue, Aug 24, 2021 at 07:04:31PM +0300, Pavel Skripkin wrote: > On 8/24/21 6:59 PM, Fabio M. De Francesco wrote: > > On Tuesday, August 24, 2021 5:43:26 PM CEST Pavel Skripkin wrote: > > > On 8/24/21 6:39 PM, Fabio M. De Francesco wrote: > > > > Oh, I know where it comes from... :) > > > > > It's a patch of mine that is in the queue, waiting to be > > > reviewed and applied. > > > > Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/ > > > > oh.... there are _a lot_ of pending changes :) > > > > > > I guess, we need smth like public-mirror with already reviewed and > > > working changes > > > > It's becoming a serious problem. A lot of times I see people who is asked to > > rebase and resend, not because they forget to fetch the current tree, instead > > because the tree changes as soon as Greg start to apply the first patches in the > > queue and the other patches at the end of the queue cannot be applied. > > > > Anyway,I understand that Greg cannot apply a patch at a time soon after > > submission but in the while the queue grows larger and larger. > > > > > It can be easily fixed. We need public fork somewhere (github, > git.kernel.org ...) and we should ask Greg to add remote-branch to his tree. No, not going to happen, sorry. I will catch up with patches when I get the chance and then all will be fine. This is highly unusual that there are loads of people all working on the same staging driver. No idea why everyone jumped on this single one... relax, there is no rush here... greg k-h ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 16:18 ` Greg Kroah-Hartman @ 2021-08-24 16:24 ` Pavel Skripkin 2021-08-24 22:20 ` Phillip Potter 1 sibling, 0 replies; 29+ messages in thread From: Pavel Skripkin @ 2021-08-24 16:24 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Fabio M. De Francesco, Larry Finger, Phillip Potter, linux-staging, linux-kernel, Christophe JAILLET On 8/24/21 7:18 PM, Greg Kroah-Hartman wrote: >> It can be easily fixed. We need public fork somewhere (github, >> git.kernel.org ...) and we should ask Greg to add remote-branch to his tree. > > No, not going to happen, sorry. I will catch up with patches when I get > the chance and then all will be fine. This is highly unusual that there Ok, sorry for this idea, I am not familiar with maintaining process :( > are loads of people all working on the same staging driver. No idea why > everyone jumped on this single one... > My guess is 1. It's new 2. There is a lot work to do > relax, there is no rush here... > We don't try to rush, just discussing. Sorry to bother you :) With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() 2021-08-24 16:18 ` Greg Kroah-Hartman 2021-08-24 16:24 ` Pavel Skripkin @ 2021-08-24 22:20 ` Phillip Potter 1 sibling, 0 replies; 29+ messages in thread From: Phillip Potter @ 2021-08-24 22:20 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Pavel Skripkin, Fabio M. De Francesco, Larry Finger, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List, Christophe JAILLET On Tue, 24 Aug 2021 at 17:18, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Aug 24, 2021 at 07:04:31PM +0300, Pavel Skripkin wrote: > > On 8/24/21 6:59 PM, Fabio M. De Francesco wrote: > > > On Tuesday, August 24, 2021 5:43:26 PM CEST Pavel Skripkin wrote: > > > > On 8/24/21 6:39 PM, Fabio M. De Francesco wrote: > > > > > Oh, I know where it comes from... :) > > > > > > It's a patch of mine that is in the queue, waiting to be > > > > reviewed and applied. > > > > > Please see: https://lore.kernel.org/lkml/20210819221241.31987-1-fmdefrancesco@gmail.com/ > > > > > oh.... there are _a lot_ of pending changes :) > > > > > > > > I guess, we need smth like public-mirror with already reviewed and > > > > working changes > > > > > > It's becoming a serious problem. A lot of times I see people who is asked to > > > rebase and resend, not because they forget to fetch the current tree, instead > > > because the tree changes as soon as Greg start to apply the first patches in the > > > queue and the other patches at the end of the queue cannot be applied. > > > > > > Anyway,I understand that Greg cannot apply a patch at a time soon after > > > submission but in the while the queue grows larger and larger. > > > > > > > > > It can be easily fixed. We need public fork somewhere (github, > > git.kernel.org ...) and we should ask Greg to add remote-branch to his tree. > > No, not going to happen, sorry. I will catch up with patches when I get > the chance and then all will be fine. This is highly unusual that there > are loads of people all working on the same staging driver. No idea why > everyone jumped on this single one... > > relax, there is no rush here... > > greg k-h Yeah I'm with Greg on this one - we don't need github forks etc, my strategy has thus far been to just wait for staging-testing to coalesce into a more up-to-date state and then work on top of that as needed. Extra forks just introduce more complexity and more to watch + keep track of in my opinion, as the e-mails still keep flowing in anyway :-) Regards, Phil ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2021-08-24 22:20 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-23 22:37 [PATCH 0/2] staging: r8188eu: Use new usb_control_msg_recv/send() Fabio M. De Francesco 2021-08-23 22:37 ` [PATCH 1/2] staging: r8188eu: Use usb_control_msg_recv/send() in usbctrl_vendorreq() Fabio M. De Francesco 2021-08-24 0:08 ` Phillip Potter 2021-08-24 0:31 ` Fabio M. De Francesco 2021-08-24 1:38 ` Fabio M. De Francesco 2021-08-24 2:01 ` Fabio M. De Francesco 2021-08-24 5:44 ` Christophe JAILLET 2021-08-24 10:38 ` Fabio M. De Francesco 2021-08-24 17:03 ` Christophe JAILLET 2021-08-24 21:59 ` Phillip Potter 2021-08-24 8:13 ` Pavel Skripkin 2021-08-24 8:53 ` Fabio M. De Francesco 2021-08-24 11:07 ` Pavel Skripkin 2021-08-24 12:01 ` Fabio M. De Francesco 2021-08-24 12:09 ` Pavel Skripkin 2021-08-24 14:55 ` Fabio M. De Francesco 2021-08-23 22:37 ` [PATCH 2/2] staging: r8188eu: Make some clean-ups " Fabio M. De Francesco 2021-08-24 0:10 ` Phillip Potter 2021-08-24 14:28 [PATCH v2 0/2] staging: r8188eu: Use new usb_control_msg_recv/send() Fabio M. De Francesco 2021-08-24 14:28 ` [PATCH 2/2] staging: r8188eu: Make some clean-ups in usbctrl_vendorreq() Fabio M. De Francesco 2021-08-24 14:39 ` Pavel Skripkin 2021-08-24 15:15 ` Fabio M. De Francesco 2021-08-24 15:26 ` Pavel Skripkin 2021-08-24 15:39 ` Fabio M. De Francesco 2021-08-24 15:43 ` Pavel Skripkin 2021-08-24 15:59 ` Fabio M. De Francesco 2021-08-24 16:04 ` Pavel Skripkin 2021-08-24 16:18 ` Greg Kroah-Hartman 2021-08-24 16:24 ` Pavel Skripkin 2021-08-24 22:20 ` Phillip Potter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).