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@xxxxxxxxx>
>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> Cc: linux-usb@xxxxxxxxxxxxxxx
>>> Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
>>> ---
>>> 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
