[PATCH v2 2/3] xhci: Set URB actual length for stopped control transfers

[PATCH v2 2/3] xhci: Set URB actual length for stopped control transfers

[Mathias Nyman]

A control transfer that stopped at the status stage incorrectly
warned about a "unexpected TRB Type 4", and did not set the
transferred actual_length for the URB.

The transferred actual_length should be set the same way for
COMP_STOPPED control transfers as in the generic cases.

generic case if we get an event at:

TRB_SETUP stage:
  length = 0;

TRB_DATA/TRB_NORMAL state:
  length = requested - remaining;

TRB_STATUS stage:
  length = requested

The URB actual_length for control transfers doesn't care about sent
bytes of the SETUP stage, or remaining bytes of the  STATUS stage.

Cc: <stable@vger.kernel.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d9936c7..584b6fe 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1975,25 +1975,15 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
 		*status = 0;
 		break;
 	case COMP_STOPPED_SHORT_PACKET:
-		if (trb_type == TRB_DATA || trb_type == TRB_NORMAL)
+		if (trb_type == TRB_DATA || trb_type == TRB_NORMAL) {
+			td->urb_length_set = true;
 			td->urb->actual_length = remaining;
-		else
+		} else {
 			xhci_warn(xhci, "WARN: Stopped Short Packet on ctrl setup or status TRB\n");
+		}
 		goto finish_td;
 	case COMP_STOPPED:
-		switch (trb_type) {
-		case TRB_SETUP:
-			td->urb->actual_length = 0;
-			goto finish_td;
-		case TRB_DATA:
-		case TRB_NORMAL:
-			td->urb->actual_length = requested - remaining;
-			goto finish_td;
-		default:
-			xhci_warn(xhci, "WARN: unexpected TRB Type %d\n",
-				  trb_type);
-			goto finish_td;
-		}
+		break;
 	case COMP_STOPPED_LENGTH_INVALID:
 		goto finish_td;
 	default:
-- 
1.9.1

Re: [PATCH v2 2/3] xhci: Set URB actual length for stopped control transfers

[Felipe Balbi]

Hi,

Mathias Nyman <mathias.nyman@linux.intel.com> writes:
> A control transfer that stopped at the status stage incorrectly
> warned about a "unexpected TRB Type 4", and did not set the
> transferred actual_length for the URB.
>
> The transferred actual_length should be set the same way for
> COMP_STOPPED control transfers as in the generic cases.
>
> generic case if we get an event at:
>
> TRB_SETUP stage:
>   length = 0;
>
> TRB_DATA/TRB_NORMAL state:
>   length = requested - remaining;
>
> TRB_STATUS stage:
>   length = requested
>
> The URB actual_length for control transfers doesn't care about sent
> bytes of the SETUP stage, or remaining bytes of the  STATUS stage.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-ring.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d9936c7..584b6fe 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1975,25 +1975,15 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
>  		*status = 0;
>  		break;
>  	case COMP_STOPPED_SHORT_PACKET:
> -		if (trb_type == TRB_DATA || trb_type == TRB_NORMAL)
> +		if (trb_type == TRB_DATA || trb_type == TRB_NORMAL) {
> +			td->urb_length_set = true;
>  			td->urb->actual_length = remaining;
> -		else
> +		} else {
>  			xhci_warn(xhci, "WARN: Stopped Short Packet on ctrl setup or status TRB\n");
> +		}
>  		goto finish_td;
>  	case COMP_STOPPED:
> -		switch (trb_type) {
> -		case TRB_SETUP:
> -			td->urb->actual_length = 0;
> -			goto finish_td;
> -		case TRB_DATA:
> -		case TRB_NORMAL:
> -			td->urb->actual_length = requested - remaining;
> -			goto finish_td;
> -		default:
> -			xhci_warn(xhci, "WARN: unexpected TRB Type %d\n",
> -				  trb_type);
> -			goto finish_td;
> -		}
> +		break;

instead of this giant patch, why didn't you just add case TRB_STATUS to
the switch statement above? It would've been a single line that would
solve your problem.

Not to mention that we loose the xhci_warn() for unexpected TRB types
which might help finding valid issues (such as $subject).

Note that now we do nothing on COMP_STOPPED, this means that we're
relying on some generic case that, arguably, could be removed
altogether. $subject is just re-adding part of obfuscation we had and
took so much work to clean up.

How have you tested $subject, btw?

-- 
balbi

Re: [PATCH v2 2/3] xhci: Set URB actual length for stopped control transfers

[Mathias Nyman]

On 27.03.2017 17:47, Felipe Balbi wrote:
>
> Hi,
>
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>> A control transfer that stopped at the status stage incorrectly
>> warned about a "unexpected TRB Type 4", and did not set the
>> transferred actual_length for the URB.
>>
>> The transferred actual_length should be set the same way for
>> COMP_STOPPED control transfers as in the generic cases.
>>
>> generic case if we get an event at:
>>
>> TRB_SETUP stage:
>>    length = 0;
>>
>> TRB_DATA/TRB_NORMAL state:
>>    length = requested - remaining;
>>
>> TRB_STATUS stage:
>>    length = requested
>>
>> The URB actual_length for control transfers doesn't care about sent
>> bytes of the SETUP stage, or remaining bytes of the  STATUS stage.
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>   drivers/usb/host/xhci-ring.c | 20 +++++---------------
>>   1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index d9936c7..584b6fe 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -1975,25 +1975,15 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
>>   		*status = 0;
>>   		break;
>>   	case COMP_STOPPED_SHORT_PACKET:
>> -		if (trb_type == TRB_DATA || trb_type == TRB_NORMAL)
>> +		if (trb_type == TRB_DATA || trb_type == TRB_NORMAL) {
>> +			td->urb_length_set = true;
>>   			td->urb->actual_length = remaining;
>> -		else
>> +		} else {
>>   			xhci_warn(xhci, "WARN: Stopped Short Packet on ctrl setup or status TRB\n");
>> +		}
>>   		goto finish_td;
>>   	case COMP_STOPPED:
>> -		switch (trb_type) {
>> -		case TRB_SETUP:
>> -			td->urb->actual_length = 0;
>> -			goto finish_td;
>> -		case TRB_DATA:
>> -		case TRB_NORMAL:
>> -			td->urb->actual_length = requested - remaining;
>> -			goto finish_td;
>> -		default:
>> -			xhci_warn(xhci, "WARN: unexpected TRB Type %d\n",
>> -				  trb_type);
>> -			goto finish_td;
>> -		}
>> +		break;
>
> instead of this giant patch, why didn't you just add case TRB_STATUS to
> the switch statement above? It would've been a single line that would
> solve your problem.

giant ?
1 file changed, 5 insertions(+), 15 deletions(-)

why add an extra case when it can be handled with the generic case?
This is almost code duplication already.

>
> Not to mention that we loose the xhci_warn() for unexpected TRB types
> which might help finding valid issues (such as $subject).

That warning was added recently and turned out to be more harmful than useful.
The type can be seen from traces instead.

We're not warning about TRB types in other cases. And we would need to
worry about link, noop, event-data types as well.

>
> Note that now we do nothing on COMP_STOPPED, this means that we're
> relying on some generic case that, arguably, could be removed
> altogether. $subject is just re-adding part of obfuscation we had and
> took so much work to clean up.

?

It's the other way around.
We want the generic way to handle as much as it can, like the successful cases,
the short packets etc. we only want to mess with it when absolutely necessary

-Mathias  

[PATCH v2 3/3] xhci: Manually give back cancelled URB if queuing it for cancel fails

[Mathias Nyman]

Manually give back URB if we are can not add it to the cancel queue, and
stop the endpoint normally.
This can happen if device just reset before URB timed out and dequeued,
leading to missing endpoint ring.

This caused a hang on Dell Inspiron 5558/0VNM2T at resume from suspend
as urb was never returned.

[  245.270505] INFO: task rtsx_usb_ms_1:254 blocked for more than 120 seconds.
[  245.272244]       Tainted: G        W       4.11.0-rc3-ARCH #2
[  245.273983] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  245.275737] rtsx_usb_ms_1   D    0   254      2 0x00000000
[  245.277524] Call Trace:
[  245.279278]  __schedule+0x2d3/0x8a0
[  245.281077]  schedule+0x3d/0x90
[  245.281961]  usb_kill_urb.part.3+0x6c/0xa0 [usbcore]
[  245.282861]  ? wake_atomic_t_function+0x60/0x60
[  245.283760]  usb_kill_urb+0x21/0x30 [usbcore]
[  245.284649]  usb_start_wait_urb+0xe5/0x170 [usbcore]
[  245.285541]  ? try_to_del_timer_sync+0x53/0x80
[  245.286434]  usb_bulk_msg+0xbd/0x160 [usbcore]
[  245.287326]  rtsx_usb_send_cmd+0x63/0x90 [rtsx_usb]

Reported-by: diego.viola@gmail.com
Tested-by: diego.viola@gmail.com
Cc: stable@vger.kernel.org
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 50aee8b..953fd8f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1477,6 +1477,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	struct xhci_ring *ep_ring;
 	struct xhci_virt_ep *ep;
 	struct xhci_command *command;
+	struct xhci_virt_device *vdev;
 
 	xhci = hcd_to_xhci(hcd);
 	spin_lock_irqsave(&xhci->lock, flags);
@@ -1485,15 +1486,27 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 
 	/* Make sure the URB hasn't completed or been unlinked already */
 	ret = usb_hcd_check_unlink_urb(hcd, urb, status);
-	if (ret || !urb->hcpriv)
+	if (ret)
 		goto done;
+
+	/* give back URB now if we can't queue it for cancel */
+	vdev = xhci->devs[urb->dev->slot_id];
+	urb_priv = urb->hcpriv;
+	if (!vdev || !urb_priv)
+		goto err_giveback;
+
+	ep_index = xhci_get_endpoint_index(&urb->ep->desc);
+	ep = &vdev->eps[ep_index];
+	ep_ring = xhci_urb_to_transfer_ring(xhci, urb);
+	if (!ep || !ep_ring)
+		goto err_giveback;
+
 	temp = readl(&xhci->op_regs->status);
 	if (temp == 0xffffffff || (xhci->xhc_state & XHCI_STATE_HALTED)) {
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
 				"HW died, freeing TD.");
-		urb_priv = urb->hcpriv;
 		for (i = urb_priv->num_tds_done;
-		     i < urb_priv->num_tds && xhci->devs[urb->dev->slot_id];
+		     i < urb_priv->num_tds;
 		     i++) {
 			td = &urb_priv->td[i];
 			if (!list_empty(&td->td_list))
@@ -1501,23 +1514,9 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 			if (!list_empty(&td->cancelled_td_list))
 				list_del_init(&td->cancelled_td_list);
 		}
-
-		usb_hcd_unlink_urb_from_ep(hcd, urb);
-		spin_unlock_irqrestore(&xhci->lock, flags);
-		usb_hcd_giveback_urb(hcd, urb, -ESHUTDOWN);
-		xhci_urb_free_priv(urb_priv);
-		return ret;
+		goto err_giveback;
 	}
 
-	ep_index = xhci_get_endpoint_index(&urb->ep->desc);
-	ep = &xhci->devs[urb->dev->slot_id]->eps[ep_index];
-	ep_ring = xhci_urb_to_transfer_ring(xhci, urb);
-	if (!ep_ring) {
-		ret = -EINVAL;
-		goto done;
-	}
-
-	urb_priv = urb->hcpriv;
 	i = urb_priv->num_tds_done;
 	if (i < urb_priv->num_tds)
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
@@ -1554,6 +1553,14 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 done:
 	spin_unlock_irqrestore(&xhci->lock, flags);
 	return ret;
+
+err_giveback:
+	if (urb_priv)
+		xhci_urb_free_priv(urb_priv);
+	usb_hcd_unlink_urb_from_ep(hcd, urb);
+	spin_unlock_irqrestore(&xhci->lock, flags);
+	usb_hcd_giveback_urb(hcd, urb, -ESHUTDOWN);
+	return ret;
 }
 
 /* Drop an endpoint from a new bandwidth configuration for this device.
-- 
1.9.1

Re: [PATCH v2 3/3] xhci: Manually give back cancelled URB if queuing it for cancel fails

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]


Hi,

Mathias Nyman <mathias.nyman@linux.intel.com> writes:
> Manually give back URB if we are can not add it to the cancel queue, and
> stop the endpoint normally.

this sentence doesn't parse very well ;-)

> This can happen if device just reset before URB timed out and dequeued,
> leading to missing endpoint ring.

seems like this could be extended a bit too.

> @@ -1554,6 +1553,14 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>  done:
>  	spin_unlock_irqrestore(&xhci->lock, flags);
>  	return ret;
> +
> +err_giveback:
> +	if (urb_priv)
> +		xhci_urb_free_priv(urb_priv);
> +	usb_hcd_unlink_urb_from_ep(hcd, urb);

well, aren't you introducing another regression here?

if the return status from usb_hcd_check_unlink_urb() is -EBUSY, then,
perhaps, you really shouldn't give it back now.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 3/3] xhci: Manually give back cancelled URB if queuing it for cancel fails

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

Felipe Balbi <felipe.balbi@linux.intel.com> writes:

> Hi,
>
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>> Manually give back URB if we are can not add it to the cancel queue, and
>> stop the endpoint normally.
>
> this sentence doesn't parse very well ;-)
>
>> This can happen if device just reset before URB timed out and dequeued,
>> leading to missing endpoint ring.
>
> seems like this could be extended a bit too.
>
>> @@ -1554,6 +1553,14 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>>  done:
>>  	spin_unlock_irqrestore(&xhci->lock, flags);
>>  	return ret;
>> +
>> +err_giveback:
>> +	if (urb_priv)
>> +		xhci_urb_free_priv(urb_priv);
>> +	usb_hcd_unlink_urb_from_ep(hcd, urb);
>
> well, aren't you introducing another regression here?
>
> if the return status from usb_hcd_check_unlink_urb() is -EBUSY, then,
> perhaps, you really shouldn't give it back now.

not to mention the following excerpt from usb_hcd_check_unlink_urb()
kdoc:

 * Return: 0 for no error, otherwise a negative error code (in which case
 * the dequeue() method must fail).  The possible error codes are:
 *
 *	-EIDRM: @urb was not submitted or has already completed.
 *		The completion function may not have been called yet.
 *
 *	-EBUSY: @urb has already been unlinked.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 3/3] xhci: Manually give back cancelled URB if queuing it for cancel fails

[Mathias Nyman]

On 27.03.2017 17:54, Felipe Balbi wrote:
>
> Hi,
>
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>> Manually give back URB if we are can not add it to the cancel queue, and
>> stop the endpoint normally.
>
> this sentence doesn't parse very well ;-)
>
>> This can happen if device just reset before URB timed out and dequeued,
>> leading to missing endpoint ring.
>
> seems like this could be extended a bit too.
>
>> @@ -1554,6 +1553,14 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>>   done:
>>   	spin_unlock_irqrestore(&xhci->lock, flags);
>>   	return ret;
>> +
>> +err_giveback:
>> +	if (urb_priv)
>> +		xhci_urb_free_priv(urb_priv);
>> +	usb_hcd_unlink_urb_from_ep(hcd, urb);
>
> well, aren't you introducing another regression here?
>
> if the return status from usb_hcd_check_unlink_urb() is -EBUSY, then,
> perhaps, you really shouldn't give it back now.

I'm not giving it back in that case, it works as it should:


ret = usb_hcd_check_unlink_urb(hcd, urb, status);
if (ret)
	goto done;
...
done:
  	spin_unlock_irqrestore(&xhci->lock, flags);
  	return ret;

-Mathias
  

Re: [PATCH v2 3/3] xhci: Manually give back cancelled URB if queuing it for cancel fails

[Felipe Balbi]

Hi,

Mathias Nyman <mathias.nyman@linux.intel.com> writes:
> On 27.03.2017 17:54, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>>> Manually give back URB if we are can not add it to the cancel queue, and
>>> stop the endpoint normally.
>>
>> this sentence doesn't parse very well ;-)
>>
>>> This can happen if device just reset before URB timed out and dequeued,
>>> leading to missing endpoint ring.
>>
>> seems like this could be extended a bit too.
>>
>>> @@ -1554,6 +1553,14 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>>>   done:
>>>   	spin_unlock_irqrestore(&xhci->lock, flags);
>>>   	return ret;
>>> +
>>> +err_giveback:
>>> +	if (urb_priv)
>>> +		xhci_urb_free_priv(urb_priv);
>>> +	usb_hcd_unlink_urb_from_ep(hcd, urb);
>>
>> well, aren't you introducing another regression here?
>>
>> if the return status from usb_hcd_check_unlink_urb() is -EBUSY, then,
>> perhaps, you really shouldn't give it back now.
>
> I'm not giving it back in that case, it works as it should:
>
>
> ret = usb_hcd_check_unlink_urb(hcd, urb, status);
> if (ret)
> 	goto done;
> ...
> done:
>   	spin_unlock_irqrestore(&xhci->lock, flags);
>   	return ret;

oh, right. I got confused with the goto labels

-- 
balbi

Re: [PATCH v2 3/3] xhci: Manually give back cancelled URB if queuing it for cancel fails

[Mathias Nyman]

On 28.03.2017 09:49, Felipe Balbi wrote:
>
> Hi,
>
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>> On 27.03.2017 17:54, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>>>> Manually give back URB if we are can not add it to the cancel queue, and
>>>> stop the endpoint normally.
>>>
>>> this sentence doesn't parse very well ;-)
>>>
>>>> This can happen if device just reset before URB timed out and dequeued,
>>>> leading to missing endpoint ring.
>>>
>>> seems like this could be extended a bit too.
>>>

I'll clean up those commit messages, and modify patch 2/3 a bit to only touch
the error message and URB actual_length for endpoint stopped at status stage.

This way there is as little change going to usb-linus and stable as possible,
saving the reset for usb-next

-Mathias