Re: [PATCH 02/37] usb: host: xhci: handle COMP_STOP from SETUP phase too

[Felipe Balbi]

Hi,

Mathias Nyman  writes:
> On 29.12.2016 13:00, Felipe Balbi wrote:
>> Stop Endpoint command can come at any point and we
>> have no control of that. We should make sure to
>> handle COMP_STOP on SETUP phase as well, otherwise
>> urb->actual_lenght might be set to negative values
>> in some occasions such as below:
>>
>>   urb->length = 4;
>>   build_control_transfer_td_for(urb, ep);
>>
>>  stop_endpoint(ep);
>>
>> COMP_STOP:
>>  [...]
>>  urb->actual_length = urb->length - trb->length;
>>
>> trb->length is 8 for SETUP stage (8 control request
>> bytes), so actual_length would be set to -4 in this
>> case.
>>
>> While doing that, also make sure to use TRB_TYPE
>> field of the actual TRB instead of matching pointers
>> to figure out in which stage of the control transfer
>> we got our completion event.
>>
>> Signed-off-by: Felipe Balbi 
>>
>> ---
>
> Cherry-picked this one for usb-linus and stable

cool, let me know if you want me to rebase the others on top of anything
else. They're all sitting on top of v4.10-rc1 for now.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/37] usb: host: xhci: handle COMP_STOP from SETUP phase too

[Mathias Nyman]

On 29.12.2016 13:00, Felipe Balbi wrote:

Stop Endpoint command can come at any point and we
have no control of that. We should make sure to
handle COMP_STOP on SETUP phase as well, otherwise
urb->actual_lenght might be set to negative values
in some occasions such as below:

  urb->length = 4;
  build_control_transfer_td_for(urb, ep);

stop_endpoint(ep);

COMP_STOP:
[...]
urb->actual_length = urb->length - trb->length;

trb->length is 8 for SETUP stage (8 control request
bytes), so actual_length would be set to -4 in this
case.

While doing that, also make sure to use TRB_TYPE
field of the actual TRB instead of matching pointers
to figure out in which stage of the control transfer
we got our completion event.

Signed-off-by: Felipe Balbi 

---


Cherry-picked this one for usb-linus and stable

-Mathias

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/37] usb: host: xhci: handle COMP_STOP from SETUP phase too

[Felipe Balbi]

Stop Endpoint command can come at any point and we
have no control of that. We should make sure to
handle COMP_STOP on SETUP phase as well, otherwise
urb->actual_lenght might be set to negative values
in some occasions such as below:

 urb->length = 4;
 build_control_transfer_td_for(urb, ep);

stop_endpoint(ep);

COMP_STOP:
[...]
urb->actual_length = urb->length - trb->length;

trb->length is 8 for SETUP stage (8 control request
bytes), so actual_length would be set to -4 in this
case.

While doing that, also make sure to use TRB_TYPE
field of the actual TRB instead of matching pointers
to figure out in which stage of the control transfer
we got our completion event.

Signed-off-by: Felipe Balbi 

---

changes since v1


  o handle multi-trb Data Stage
---
 drivers/usb/host/xhci-ring.c | 34 ++
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 76402b44f832..02506c44380d 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1939,8 +1939,9 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
struct xhci_ep_ctx *ep_ctx;
u32 trb_comp_code;
u32 remaining, requested;
-   bool on_data_stage;
+   u32 trb_type;
 
+   trb_type = TRB_FIELD_TO_TYPE(le32_to_cpu(ep_trb->generic.field[3]));
slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags));
xdev = xhci->devs[slot_id];
ep_index = TRB_TO_EP_ID(le32_to_cpu(event->flags)) - 1;
@@ -1950,14 +1951,11 @@ 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));
 
-   /* not setup (dequeue), or status stage means we are at data stage */
-   on_data_stage = (ep_trb != ep_ring->dequeue && ep_trb != td->last_trb);
-
switch (trb_comp_code) {
case COMP_SUCCESS:
-   if (ep_trb != td->last_trb) {
+   if (trb_type != TRB_STATUS) {
xhci_warn(xhci, "WARN: Success on ctrl %s TRB without 
IOC set?\n",
- on_data_stage ? "data" : "setup");
+   (trb_type == TRB_DATA) ? "data" : 
"setup");
*status = -ESHUTDOWN;
break;
}
@@ -1967,15 +1965,25 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
*status = 0;
break;
case COMP_STOP_SHORT:
-   if (on_data_stage)
+   if (trb_type == TRB_DATA ||
+   trb_type == TRB_NORMAL)
td->urb->actual_length = remaining;
else
xhci_warn(xhci, "WARN: Stopped Short Packet on ctrl 
setup or status TRB\n");
goto finish_td;
case COMP_STOP:
-   if (on_data_stage)
+   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;
+   goto finish_td;
+   default:
+   xhci_warn(xhci, "WARN: unexpected TRB Type %d\n", 
trb_type);
+   goto finish_td;
+   }
case COMP_STOP_INVAL:
goto finish_td;
default:
@@ -1987,7 +1995,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
/* else fall through */
case COMP_STALL:
/* Did we transfer part of the data (middle) phase? */
-   if (on_data_stage)
+   if (trb_type == TRB_DATA ||
+   trb_type == TRB_NORMAL)
td->urb->actual_length = requested - remaining;
else if (!td->urb_length_set)
td->urb->actual_length = 0;
@@ -1995,14 +2004,15 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
}
 
/* stopped at setup stage, no data transferred */
-   if (ep_trb == ep_ring->dequeue)
+   if (trb_type == TRB_SETUP)
goto finish_td;
 
/*
 * if on data stage then update the actual_length of the URB and flag it
 * as set, so it won't be overwritten in the event for the last TRB.
 */
-   if (on_data_stage) {
+   if (trb_type == TRB_DATA ||
+   trb_type == TRB_NORMAL) {
td->urb_length_set = true;
td->urb->actual_length = requested - remaining;
xhci_dbg(xhci, "Waiting for status stage event\n");
-- 
2.11.0

--