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

[PATCH 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
tranferred actual_length for the URB.

The tranferred actual_length should be set the same way for
COMP_STOPPED control tranfers 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 | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d9936c7..0aca4c9 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1961,6 +1961,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
 	requested = td->urb->transfer_buffer_length;
 	remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
 
+
 	switch (trb_comp_code) {
 	case COMP_SUCCESS:
 		if (trb_type != TRB_STATUS) {
@@ -1975,25 +1976,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

[PATCH 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 2/3] xhci: Set URB actual length for stopped control transfers

[Greg KH]

On Mon, Mar 27, 2017 at 04:58:39PM +0300, Mathias Nyman wrote:
> A control transfer that stopped at the status stage incorrectly
> warned about a "unexpected TRB Type 4", and did not set the
> tranferred actual_length for the URB.
> 
> The tranferred actual_length should be set the same way for
> COMP_STOPPED control tranfers 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 | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d9936c7..0aca4c9 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1961,6 +1961,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
>  	requested = td->urb->transfer_buffer_length;
>  	remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
>  
> +
>  	switch (trb_comp_code) {
>  	case COMP_SUCCESS:
>  		if (trb_type != TRB_STATUS) {

Why the extra blank line?

thanks,

greg k-h

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

[Mathias Nyman]

On 27.03.2017 17:07, Greg KH wrote:
> On Mon, Mar 27, 2017 at 04:58:39PM +0300, Mathias Nyman wrote:
>> A control transfer that stopped at the status stage incorrectly
>> warned about a "unexpected TRB Type 4", and did not set the
>> tranferred actual_length for the URB.
>>
>> The tranferred actual_length should be set the same way for
>> COMP_STOPPED control tranfers 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 | 21 ++++++---------------
>>   1 file changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index d9936c7..0aca4c9 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -1961,6 +1961,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
>>   	requested = td->urb->transfer_buffer_length;
>>   	remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
>>
>> +
>>   	switch (trb_comp_code) {
>>   	case COMP_SUCCESS:
>>   		if (trb_type != TRB_STATUS) {
>
> Why the extra blank line?

Shouldn't be there.
I'll resend, and fix the tranferred -> transferred typos as well.

-Mathias