| Message ID | 20210526130037.856068-18-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rid W=1 warnings from USB | expand |
On Wed, May 26, 2021 at 02:00:30PM +0100, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > drivers/usb/host/xhci.c: In function ‘xhci_unmap_temp_buf’: > drivers/usb/host/xhci.c:1349:15: warning: variable ‘len’ set but not used [-Wunused-but-set-variable] > > Cc: Mathias Nyman <mathias.nyman@intel.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-usb@vger.kernel.org > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/usb/host/xhci.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 27283654ca080..ac2a7d4288883 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -1346,7 +1346,6 @@ static bool xhci_urb_temp_buffer_required(struct usb_hcd *hcd, > > static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) > { > - unsigned int len; > unsigned int buf_len; > enum dma_data_direction dir; > > @@ -1362,10 +1361,10 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) > dir); > > if (usb_urb_dir_in(urb)) > - len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs, > - urb->transfer_buffer, > - buf_len, > - 0); > + sg_pcopy_from_buffer(urb->sg, urb->num_sgs, > + urb->transfer_buffer, > + buf_len, > + 0); Sorry, but no, I keep rejecting this over and over, it needs to handle the error handling properly and not paper over it like this :( All the bots keep tripping up on it, you are not alone. thanks, greg k-h
On Thu, 27 May 2021, Greg Kroah-Hartman wrote: > On Wed, May 26, 2021 at 02:00:30PM +0100, Lee Jones wrote: > > Fixes the following W=1 kernel build warning(s): > > > > drivers/usb/host/xhci.c: In function ‘xhci_unmap_temp_buf’: > > drivers/usb/host/xhci.c:1349:15: warning: variable ‘len’ set but not used [-Wunused-but-set-variable] > > > > Cc: Mathias Nyman <mathias.nyman@intel.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: linux-usb@vger.kernel.org > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/usb/host/xhci.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index 27283654ca080..ac2a7d4288883 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -1346,7 +1346,6 @@ static bool xhci_urb_temp_buffer_required(struct usb_hcd *hcd, > > > > static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) > > { > > - unsigned int len; > > unsigned int buf_len; > > enum dma_data_direction dir; > > > > @@ -1362,10 +1361,10 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) > > dir); > > > > if (usb_urb_dir_in(urb)) > > - len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs, > > - urb->transfer_buffer, > > - buf_len, > > - 0); > > + sg_pcopy_from_buffer(urb->sg, urb->num_sgs, > > + urb->transfer_buffer, > > + buf_len, > > + 0); > > Sorry, but no, I keep rejecting this over and over, it needs to handle > the error handling properly and not paper over it like this :( Will fix. > All the bots keep tripping up on it, you are not alone.
On 27.5.2021 11.16, Lee Jones wrote: > On Thu, 27 May 2021, Greg Kroah-Hartman wrote: > >> On Wed, May 26, 2021 at 02:00:30PM +0100, Lee Jones wrote: >>> Fixes the following W=1 kernel build warning(s): >>> >>> drivers/usb/host/xhci.c: In function ‘xhci_unmap_temp_buf’: >>> drivers/usb/host/xhci.c:1349:15: warning: variable ‘len’ set but not used [-Wunused-but-set-variable] >>> >>> Cc: Mathias Nyman <mathias.nyman@intel.com> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> Cc: linux-usb@vger.kernel.org >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>> --- >>> drivers/usb/host/xhci.c | 9 ++++----- >>> 1 file changed, 4 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >>> index 27283654ca080..ac2a7d4288883 100644 >>> --- a/drivers/usb/host/xhci.c >>> +++ b/drivers/usb/host/xhci.c >>> @@ -1346,7 +1346,6 @@ static bool xhci_urb_temp_buffer_required(struct usb_hcd *hcd, >>> >>> static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) >>> { >>> - unsigned int len; >>> unsigned int buf_len; >>> enum dma_data_direction dir; >>> >>> @@ -1362,10 +1361,10 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) >>> dir); >>> >>> if (usb_urb_dir_in(urb)) >>> - len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs, >>> - urb->transfer_buffer, >>> - buf_len, >>> - 0); >>> + sg_pcopy_from_buffer(urb->sg, urb->num_sgs, >>> + urb->transfer_buffer, >>> + buf_len, >>> + 0); >> >> Sorry, but no, I keep rejecting this over and over, it needs to handle >> the error handling properly and not paper over it like this :( > > Will fix. > >> All the bots keep tripping up on it, you are not alone. > This is getting a lot of attention. Something like this should fix it: diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 27283654ca08..306ab81421fd 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1361,12 +1361,16 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) urb->transfer_buffer_length, dir); - if (usb_urb_dir_in(urb)) + if (usb_urb_dir_in(urb)) { len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs, urb->transfer_buffer, buf_len, 0); - + if (len != buf_len) { + xhci_dbg(xhci, "Copy from tmp buf to urb sg list failed\n"); + urb->actual_length = len; + } + } urb->transfer_flags &= ~URB_DMA_MAP_SINGLE; kfree(urb->transfer_buffer); urb->transfer_buffer = NULL; urb->actual_length is now properly set. The debug level message will help me find the cause if we ever need to debug oddly behaving devices. Note this is a very rarly taken codepath for quirky xHC harware that can't handle a specific sequence of buffer lengths queued. I can write a proper commit message and push this forward -Mathias
On Thu, May 27, 2021 at 12:23:24PM +0300, Mathias Nyman wrote: > On 27.5.2021 11.16, Lee Jones wrote: > > On Thu, 27 May 2021, Greg Kroah-Hartman wrote: > > > >> On Wed, May 26, 2021 at 02:00:30PM +0100, Lee Jones wrote: > >>> Fixes the following W=1 kernel build warning(s): > >>> > >>> drivers/usb/host/xhci.c: In function ‘xhci_unmap_temp_buf’: > >>> drivers/usb/host/xhci.c:1349:15: warning: variable ‘len’ set but not used [-Wunused-but-set-variable] > >>> > >>> Cc: Mathias Nyman <mathias.nyman@intel.com> > >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >>> Cc: linux-usb@vger.kernel.org > >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> > >>> --- > >>> drivers/usb/host/xhci.c | 9 ++++----- > >>> 1 file changed, 4 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > >>> index 27283654ca080..ac2a7d4288883 100644 > >>> --- a/drivers/usb/host/xhci.c > >>> +++ b/drivers/usb/host/xhci.c > >>> @@ -1346,7 +1346,6 @@ static bool xhci_urb_temp_buffer_required(struct usb_hcd *hcd, > >>> > >>> static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) > >>> { > >>> - unsigned int len; > >>> unsigned int buf_len; > >>> enum dma_data_direction dir; > >>> > >>> @@ -1362,10 +1361,10 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) > >>> dir); > >>> > >>> if (usb_urb_dir_in(urb)) > >>> - len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs, > >>> - urb->transfer_buffer, > >>> - buf_len, > >>> - 0); > >>> + sg_pcopy_from_buffer(urb->sg, urb->num_sgs, > >>> + urb->transfer_buffer, > >>> + buf_len, > >>> + 0); > >> > >> Sorry, but no, I keep rejecting this over and over, it needs to handle > >> the error handling properly and not paper over it like this :( > > > > Will fix. > > > >> All the bots keep tripping up on it, you are not alone. > > > > This is getting a lot of attention. Something like this should fix it: > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 27283654ca08..306ab81421fd 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -1361,12 +1361,16 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) > urb->transfer_buffer_length, > dir); > > - if (usb_urb_dir_in(urb)) > + if (usb_urb_dir_in(urb)) { > len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs, > urb->transfer_buffer, > buf_len, > 0); > - > + if (len != buf_len) { > + xhci_dbg(xhci, "Copy from tmp buf to urb sg list failed\n"); > + urb->actual_length = len; > + } > + } > urb->transfer_flags &= ~URB_DMA_MAP_SINGLE; > kfree(urb->transfer_buffer); > urb->transfer_buffer = NULL; > > urb->actual_length is now properly set. > The debug level message will help me find the cause if we ever need > to debug oddly behaving devices. > > Note this is a very rarly taken codepath for quirky xHC harware that > can't handle a specific sequence of buffer lengths queued. > > I can write a proper commit message and push this forward That looks good, I kept waiting for one of the people who kept trying to report "build warning fixes" to do it right, but I'll gladly take it from you as well :) thanks, greg k-h
On Thu, 27 May 2021, Mathias Nyman wrote: > On 27.5.2021 11.16, Lee Jones wrote: > > On Thu, 27 May 2021, Greg Kroah-Hartman wrote: > > > >> On Wed, May 26, 2021 at 02:00:30PM +0100, Lee Jones wrote: > >>> Fixes the following W=1 kernel build warning(s): > >>> > >>> drivers/usb/host/xhci.c: In function ‘xhci_unmap_temp_buf’: > >>> drivers/usb/host/xhci.c:1349:15: warning: variable ‘len’ set but not used [-Wunused-but-set-variable] > >>> > >>> Cc: Mathias Nyman <mathias.nyman@intel.com> > >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >>> Cc: linux-usb@vger.kernel.org > >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> > >>> --- > >>> drivers/usb/host/xhci.c | 9 ++++----- > >>> 1 file changed, 4 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > >>> index 27283654ca080..ac2a7d4288883 100644 > >>> --- a/drivers/usb/host/xhci.c > >>> +++ b/drivers/usb/host/xhci.c > >>> @@ -1346,7 +1346,6 @@ static bool xhci_urb_temp_buffer_required(struct usb_hcd *hcd, > >>> > >>> static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) > >>> { > >>> - unsigned int len; > >>> unsigned int buf_len; > >>> enum dma_data_direction dir; > >>> > >>> @@ -1362,10 +1361,10 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) > >>> dir); > >>> > >>> if (usb_urb_dir_in(urb)) > >>> - len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs, > >>> - urb->transfer_buffer, > >>> - buf_len, > >>> - 0); > >>> + sg_pcopy_from_buffer(urb->sg, urb->num_sgs, > >>> + urb->transfer_buffer, > >>> + buf_len, > >>> + 0); > >> > >> Sorry, but no, I keep rejecting this over and over, it needs to handle > >> the error handling properly and not paper over it like this :( > > > > Will fix. > > > >> All the bots keep tripping up on it, you are not alone. > > > > This is getting a lot of attention. Something like this should fix it: > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 27283654ca08..306ab81421fd 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -1361,12 +1361,16 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) > urb->transfer_buffer_length, > dir); > > - if (usb_urb_dir_in(urb)) > + if (usb_urb_dir_in(urb)) { > len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs, > urb->transfer_buffer, > buf_len, > 0); > - > + if (len != buf_len) { > + xhci_dbg(xhci, "Copy from tmp buf to urb sg list failed\n"); > + urb->actual_length = len; > + } > + } > urb->transfer_flags &= ~URB_DMA_MAP_SINGLE; > kfree(urb->transfer_buffer); > urb->transfer_buffer = NULL; > > urb->actual_length is now properly set. > The debug level message will help me find the cause if we ever need > to debug oddly behaving devices. > > Note this is a very rarly taken codepath for quirky xHC harware that > can't handle a specific sequence of buffer lengths queued. > > I can write a proper commit message and push this forward Okay by me.
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 27283654ca080..ac2a7d4288883 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1346,7 +1346,6 @@ static bool xhci_urb_temp_buffer_required(struct usb_hcd *hcd, static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) { - unsigned int len; unsigned int buf_len; enum dma_data_direction dir; @@ -1362,10 +1361,10 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb) dir); if (usb_urb_dir_in(urb)) - len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs, - urb->transfer_buffer, - buf_len, - 0); + sg_pcopy_from_buffer(urb->sg, urb->num_sgs, + urb->transfer_buffer, + buf_len, + 0); urb->transfer_flags &= ~URB_DMA_MAP_SINGLE; kfree(urb->transfer_buffer);
Fixes the following W=1 kernel build warning(s): drivers/usb/host/xhci.c: In function ‘xhci_unmap_temp_buf’: drivers/usb/host/xhci.c:1349:15: warning: variable ‘len’ set but not used [-Wunused-but-set-variable] Cc: Mathias Nyman <mathias.nyman@intel.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-usb@vger.kernel.org Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/usb/host/xhci.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)