Re: [PATCH 17/24] usb: host: xhci: Remove unused variable 'len'

[Mathias Nyman]

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