Re: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-03-02 Thread Alan Stern 
On Mon, 2 Mar 2015, Aleksander Morgado wrote:

 I think my patch also lacks the -EREMOTEIO return for the case when 0
 bytes are transferred. I'll try to update it today.

It's not necessary.  The USB core automatically sets the status to 
-EREMOTEIO if the status is equal to 0 and the transfer was short and 
URB_SHORT_NOT_OK was set.  See __usb_hcd_giveback_urb().

Alan Stern

--
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: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-03-02 Thread Mathias Nyman 
On 26.02.2015 20:01, Alan Stern wrote:
 On Thu, 26 Feb 2015, Mathias Nyman wrote:
 
 On 26.02.2015 18:12, Mathias Nyman wrote:
 When a control transfer has a short data stage, the xHCI controller 
 generates
 two transfer events: a COMP_SHORT_TX event that specifies the untransferred
 amount, and a COMP_SUCCESS event. But when the data stage is not short, only
 the COMP_SUCCESS event occurs. Therefore, xhci-hcd sets urb-actual_length
 to urb-transfer_buffer_length while processing the COMP_SUCCESS event,
 unless urb-actual_length was set already by a previous COMP_SHORT_TX event.

 The driver checks this by seeing whether urb-actual_length == 0, but this
 alone is the wrong test, as it is entirely possible for a short transfer to
 have an urb-actual_length = 0.

 This patch changes the xhci driver to set the urb-actual_length in advance
 to the expected value of a successful control transfer.
 The urb-actual_length is then only adjusted in case of short transfers or
 other special events, but not on COMP_SUCCESS events.

 This fixes a bug which affected the HSO plugin, which relies on URBs with
 urb-actual_length == 0 to halt re-submitting the RX URB in the control
 endpoint.

 Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com
 ---
  drivers/usb/host/xhci-ring.c | 73 
 ++--
  1 file changed, 37 insertions(+), 36 deletions(-)

 diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
 index b46b5b9..0e02e79 100644
 --- a/drivers/usb/host/xhci-ring.c
 +++ b/drivers/usb/host/xhci-ring.c
 @@ -732,7 +732,11 @@ remove_finished_td:
 /* Clean up the cancelled URB */
 /* Doesn't matter what we pass for status, since the core will
  * just overwrite it (because the URB has been unlinked).
 +* Control urbs have the urb-actual_length pre-set, clear it
 +* as well
  */
 +   if (usb_endpoint_xfer_control(cur_td-urb-ep-desc))
 +   cur_td-urb-actual_length = 0;
 
 Now this would do the wrong thing if some data had been sent or 
 received before the URB was cancelled.

Thats right, this would overwrite any length set in a transfer event
for this TD before canceling the the TD

 
 Does the controller write back information about the number of bytes to
 the data TRB?  Maybe it would be easier to get the values from there
 instead of trying to use the event structures.

Unfortunately it looks like we need to rely on the lengths from the events.

Streams do have a Event Data Transfer Length Accumulator (EDTLA) in the stream
context that xhci writes to when the endpoint is stopped, but this requires that
the host supports a Stopped EDTLA capability, and is only for streams.

I think that maybe Aleksanders suggestion of adding the length_set flag is 
the way to go, atleast that something that we can get to the next rc release,
and it's not as intrusive.

-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

Re: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-03-02 Thread Mathias Nyman 
On 26.02.2015 23:59, Aleksander Morgado wrote:
 On Thu, Feb 26, 2015 at 5:12 PM, Mathias Nyman
 mathias.ny...@linux.intel.com wrote:
 When a control transfer has a short data stage, the xHCI controller generates
 two transfer events: a COMP_SHORT_TX event that specifies the untransferred
 amount, and a COMP_SUCCESS event. But when the data stage is not short, only
 the COMP_SUCCESS event occurs. Therefore, xhci-hcd sets urb-actual_length
 to urb-transfer_buffer_length while processing the COMP_SUCCESS event,
 unless urb-actual_length was set already by a previous COMP_SHORT_TX event.

 The driver checks this by seeing whether urb-actual_length == 0, but this
 alone is the wrong test, as it is entirely possible for a short transfer to
 have an urb-actual_length = 0.

 This patch changes the xhci driver to set the urb-actual_length in advance
 to the expected value of a successful control transfer.
 The urb-actual_length is then only adjusted in case of short transfers or
 other special events, but not on COMP_SUCCESS events.

 This fixes a bug which affected the HSO plugin, which relies on URBs with
 urb-actual_length == 0 to halt re-submitting the RX URB in the control
 endpoint.

 Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com
 ---
  drivers/usb/host/xhci-ring.c | 73 
 ++--
  1 file changed, 37 insertions(+), 36 deletions(-)

 diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
 index b46b5b9..0e02e79 100644
 --- a/drivers/usb/host/xhci-ring.c
 +++ b/drivers/usb/host/xhci-ring.c
 @@ -732,7 +732,11 @@ remove_finished_td:
 /* Clean up the cancelled URB */
 /* Doesn't matter what we pass for status, since the core 
 will
  * just overwrite it (because the URB has been unlinked).
 +* Control urbs have the urb-actual_length pre-set, clear it
 +* as well
  */
 +   if (usb_endpoint_xfer_control(cur_td-urb-ep-desc))
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, 0);

 /* Stop processing the cancelled list if the watchdog timer 
 is
 @@ -755,6 +759,7 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, 
 struct xhci_ring *ring)
 list_del_init(cur_td-td_list);
 if (!list_empty(cur_td-cancelled_td_list))
 list_del_init(cur_td-cancelled_td_list);
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
 }
  }
 @@ -792,6 +797,7 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd 
 *xhci,
 cur_td = list_first_entry(ep-cancelled_td_list,
 struct xhci_td, cancelled_td_list);
 list_del_init(cur_td-cancelled_td_list);
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
 }
  }
 @@ -1888,6 +1894,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 int ep_index;
 struct xhci_ep_ctx *ep_ctx;
 u32 trb_comp_code;
 +   bool force_finish_td = false;

 slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event-flags));
 xdev = xhci-devs[slot_id];
 @@ -1906,7 +1913,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 xhci_warn(xhci, WARN: Success on ctrl data TRB 
 without IOC set??\n);
 *status = -ESHUTDOWN;
 -   } else {
 +   } else if (*status == -EINPROGRESS) {
 +   /* only set to 0 if no previous event set it earlier 
 */
 *status = 0;
 }
 break;
 @@ -1918,6 +1926,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 break;
 case COMP_STOP_INVAL:
 case COMP_STOP:
 +   /* we don't continue stopped TDs, so length can be set to 0 
 */
 +   td-urb-actual_length = 0;
 return finish_td(xhci, td, event_trb, event, ep, status, 
 false);
 default:
 if (!xhci_requires_manual_halt_cleanup(xhci,
 @@ -1928,44 +1938,26 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 trb_comp_code, ep_index);
 /* else fall through */
 case COMP_STALL:
 -   /* Did we transfer part of the data (middle) phase? */
 -   if (event_trb != ep_ring-dequeue 
 -   event_trb != td-last_trb)
 -   td-urb-actual_length =
 -   td-urb-transfer_buffer_length -
 -   
 EVENT_TRB_LEN(le32_to_cpu(event-transfer_len));
 -   else
 -

Re: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-03-02 Thread Aleksander Morgado 
On Mon, Mar 2, 2015 at 12:57 PM, Mathias Nyman
mathias.ny...@linux.intel.com wrote:
 On 26.02.2015 23:59, Aleksander Morgado wrote:
 On Thu, Feb 26, 2015 at 5:12 PM, Mathias Nyman
 mathias.ny...@linux.intel.com wrote:
 When a control transfer has a short data stage, the xHCI controller 
 generates
 two transfer events: a COMP_SHORT_TX event that specifies the untransferred
 amount, and a COMP_SUCCESS event. But when the data stage is not short, only
 the COMP_SUCCESS event occurs. Therefore, xhci-hcd sets urb-actual_length
 to urb-transfer_buffer_length while processing the COMP_SUCCESS event,
 unless urb-actual_length was set already by a previous COMP_SHORT_TX event.

 The driver checks this by seeing whether urb-actual_length == 0, but this
 alone is the wrong test, as it is entirely possible for a short transfer to
 have an urb-actual_length = 0.

 This patch changes the xhci driver to set the urb-actual_length in advance
 to the expected value of a successful control transfer.
 The urb-actual_length is then only adjusted in case of short transfers or
 other special events, but not on COMP_SUCCESS events.

 This fixes a bug which affected the HSO plugin, which relies on URBs with
 urb-actual_length == 0 to halt re-submitting the RX URB in the control
 endpoint.

 Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com
 ---
  drivers/usb/host/xhci-ring.c | 73 
 ++--
  1 file changed, 37 insertions(+), 36 deletions(-)

 diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
 index b46b5b9..0e02e79 100644
 --- a/drivers/usb/host/xhci-ring.c
 +++ b/drivers/usb/host/xhci-ring.c
 @@ -732,7 +732,11 @@ remove_finished_td:
 /* Clean up the cancelled URB */
 /* Doesn't matter what we pass for status, since the core 
 will
  * just overwrite it (because the URB has been unlinked).
 +* Control urbs have the urb-actual_length pre-set, clear 
 it
 +* as well
  */
 +   if (usb_endpoint_xfer_control(cur_td-urb-ep-desc))
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, 0);

 /* Stop processing the cancelled list if the watchdog timer 
 is
 @@ -755,6 +759,7 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, 
 struct xhci_ring *ring)
 list_del_init(cur_td-td_list);
 if (!list_empty(cur_td-cancelled_td_list))
 list_del_init(cur_td-cancelled_td_list);
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
 }
  }
 @@ -792,6 +797,7 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd 
 *xhci,
 cur_td = list_first_entry(ep-cancelled_td_list,
 struct xhci_td, cancelled_td_list);
 list_del_init(cur_td-cancelled_td_list);
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
 }
  }
 @@ -1888,6 +1894,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 int ep_index;
 struct xhci_ep_ctx *ep_ctx;
 u32 trb_comp_code;
 +   bool force_finish_td = false;

 slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event-flags));
 xdev = xhci-devs[slot_id];
 @@ -1906,7 +1913,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 xhci_warn(xhci, WARN: Success on ctrl data TRB 
 without IOC set??\n);
 *status = -ESHUTDOWN;
 -   } else {
 +   } else if (*status == -EINPROGRESS) {
 +   /* only set to 0 if no previous event set it 
 earlier */
 *status = 0;
 }
 break;
 @@ -1918,6 +1926,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 break;
 case COMP_STOP_INVAL:
 case COMP_STOP:
 +   /* we don't continue stopped TDs, so length can be set to 0 
 */
 +   td-urb-actual_length = 0;
 return finish_td(xhci, td, event_trb, event, ep, status, 
 false);
 default:
 if (!xhci_requires_manual_halt_cleanup(xhci,
 @@ -1928,44 +1938,26 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 trb_comp_code, ep_index);
 /* else fall through */
 case COMP_STALL:
 -   /* Did we transfer part of the data (middle) phase? */
 -   if (event_trb != ep_ring-dequeue 
 -   event_trb != td-last_trb)
 -   td-urb-actual_length =
 -   td-urb-transfer_buffer_length -
 -

Re: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-03-02 Thread Alan Stern 
On Mon, 2 Mar 2015, Aleksander Morgado wrote:

 On Mon, Mar 2, 2015 at 4:02 PM, Alan Stern st...@rowland.harvard.edu wrote:
  On Mon, 2 Mar 2015, Aleksander Morgado wrote:
 
  I think my patch also lacks the -EREMOTEIO return for the case when 0
  bytes are transferred. I'll try to update it today.
 
  It's not necessary.  The USB core automatically sets the status to
  -EREMOTEIO if the status is equal to 0 and the transfer was short and
  URB_SHORT_NOT_OK was set.  See __usb_hcd_giveback_urb()
 
 There are actually several places in xhci-ring.c where that check is
 done in order to return -EREMOTEIO; does that mean that all of them
 are redundant? e.g. not only the ones in process_ctrl_td() but also in
 finish_td().

Yes, they are redundant.

Alan Stern

--
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: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-03-02 Thread Aleksander Morgado 
On Mon, Mar 2, 2015 at 4:02 PM, Alan Stern st...@rowland.harvard.edu wrote:
 On Mon, 2 Mar 2015, Aleksander Morgado wrote:

 I think my patch also lacks the -EREMOTEIO return for the case when 0
 bytes are transferred. I'll try to update it today.

 It's not necessary.  The USB core automatically sets the status to
 -EREMOTEIO if the status is equal to 0 and the transfer was short and
 URB_SHORT_NOT_OK was set.  See __usb_hcd_giveback_urb()

There are actually several places in xhci-ring.c where that check is
done in order to return -EREMOTEIO; does that mean that all of them
are redundant? e.g. not only the ones in process_ctrl_td() but also in
finish_td().

-- 
Aleksander
https://aleksander.es
--
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: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-03-02 Thread Alan Stern 
On Mon, 2 Mar 2015, Aleksander Morgado wrote:

 On Mon, Mar 2, 2015 at 6:10 PM, Alan Stern st...@rowland.harvard.edu wrote:
  On Mon, Mar 2, 2015 at 4:02 PM, Alan Stern st...@rowland.harvard.edu 
  wrote:
   On Mon, 2 Mar 2015, Aleksander Morgado wrote:
  
   I think my patch also lacks the -EREMOTEIO return for the case when 0
   bytes are transferred. I'll try to update it today.
  
   It's not necessary.  The USB core automatically sets the status to
   -EREMOTEIO if the status is equal to 0 and the transfer was short and
   URB_SHORT_NOT_OK was set.  See __usb_hcd_giveback_urb()
 
  There are actually several places in xhci-ring.c where that check is
  done in order to return -EREMOTEIO; does that mean that all of them
  are redundant? e.g. not only the ones in process_ctrl_td() but also in
  finish_td().
 
  Yes, they are redundant.
 
 I guess my v5 of the patch is still ok; right?

I didn't notice anything wrong with it.

 I can prepare a new
 patch to remove all the redundant -EREMOTEIO returns later.

Alan Stern

--
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: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-02-27 Thread Aleksander Morgado 
On Thu, Feb 26, 2015 at 5:12 PM, Mathias Nyman
mathias.ny...@linux.intel.com wrote:
 When a control transfer has a short data stage, the xHCI controller generates
 two transfer events: a COMP_SHORT_TX event that specifies the untransferred
 amount, and a COMP_SUCCESS event. But when the data stage is not short, only
 the COMP_SUCCESS event occurs. Therefore, xhci-hcd sets urb-actual_length
 to urb-transfer_buffer_length while processing the COMP_SUCCESS event,
 unless urb-actual_length was set already by a previous COMP_SHORT_TX event.

 The driver checks this by seeing whether urb-actual_length == 0, but this
 alone is the wrong test, as it is entirely possible for a short transfer to
 have an urb-actual_length = 0.

 This patch changes the xhci driver to set the urb-actual_length in advance
 to the expected value of a successful control transfer.
 The urb-actual_length is then only adjusted in case of short transfers or
 other special events, but not on COMP_SUCCESS events.

 This fixes a bug which affected the HSO plugin, which relies on URBs with
 urb-actual_length == 0 to halt re-submitting the RX URB in the control
 endpoint.

 Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com

Tested this patch (with the extra else if suggested in the follow up
commit) and it seems to work correctly with the HSO plugin. Not sure
if it'll end up being the last version or not, but anyway:

Tested-by: Aleksander Morgado aleksan...@aleksander.es

Let me know if you want me to test anything else.

 ---
  drivers/usb/host/xhci-ring.c | 73 
 ++--
  1 file changed, 37 insertions(+), 36 deletions(-)

 diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
 index b46b5b9..0e02e79 100644
 --- a/drivers/usb/host/xhci-ring.c
 +++ b/drivers/usb/host/xhci-ring.c
 @@ -732,7 +732,11 @@ remove_finished_td:
 /* Clean up the cancelled URB */
 /* Doesn't matter what we pass for status, since the core will
  * just overwrite it (because the URB has been unlinked).
 +* Control urbs have the urb-actual_length pre-set, clear it
 +* as well
  */
 +   if (usb_endpoint_xfer_control(cur_td-urb-ep-desc))
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, 0);

 /* Stop processing the cancelled list if the watchdog timer is
 @@ -755,6 +759,7 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, 
 struct xhci_ring *ring)
 list_del_init(cur_td-td_list);
 if (!list_empty(cur_td-cancelled_td_list))
 list_del_init(cur_td-cancelled_td_list);
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
 }
  }
 @@ -792,6 +797,7 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci,
 cur_td = list_first_entry(ep-cancelled_td_list,
 struct xhci_td, cancelled_td_list);
 list_del_init(cur_td-cancelled_td_list);
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
 }
  }
 @@ -1888,6 +1894,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 int ep_index;
 struct xhci_ep_ctx *ep_ctx;
 u32 trb_comp_code;
 +   bool force_finish_td = false;

 slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event-flags));
 xdev = xhci-devs[slot_id];
 @@ -1906,7 +1913,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 xhci_warn(xhci, WARN: Success on ctrl data TRB 
 without IOC set??\n);
 *status = -ESHUTDOWN;
 -   } else {
 +   } else if (*status == -EINPROGRESS) {
 +   /* only set to 0 if no previous event set it earlier 
 */
 *status = 0;
 }
 break;
 @@ -1918,6 +1926,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 break;
 case COMP_STOP_INVAL:
 case COMP_STOP:
 +   /* we don't continue stopped TDs, so length can be set to 0 */
 +   td-urb-actual_length = 0;
 return finish_td(xhci, td, event_trb, event, ep, status, 
 false);
 default:
 if (!xhci_requires_manual_halt_cleanup(xhci,
 @@ -1928,44 +1938,26 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 trb_comp_code, ep_index);
 /* else fall through */
 case COMP_STALL:
 -   /* Did we transfer part of the data (middle) phase? */
 -   if (event_trb != ep_ring-dequeue 
 -   event_trb != td-last_trb)
 -

[RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-02-26 Thread Mathias Nyman 
When a control transfer has a short data stage, the xHCI controller generates
two transfer events: a COMP_SHORT_TX event that specifies the untransferred
amount, and a COMP_SUCCESS event. But when the data stage is not short, only
the COMP_SUCCESS event occurs. Therefore, xhci-hcd sets urb-actual_length
to urb-transfer_buffer_length while processing the COMP_SUCCESS event,
unless urb-actual_length was set already by a previous COMP_SHORT_TX event.

The driver checks this by seeing whether urb-actual_length == 0, but this
alone is the wrong test, as it is entirely possible for a short transfer to
have an urb-actual_length = 0.

This patch changes the xhci driver to set the urb-actual_length in advance
to the expected value of a successful control transfer.
The urb-actual_length is then only adjusted in case of short transfers or
other special events, but not on COMP_SUCCESS events.

This fixes a bug which affected the HSO plugin, which relies on URBs with
urb-actual_length == 0 to halt re-submitting the RX URB in the control
endpoint.

Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com
---
 drivers/usb/host/xhci-ring.c | 73 ++--
 1 file changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index b46b5b9..0e02e79 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -732,7 +732,11 @@ remove_finished_td:
/* Clean up the cancelled URB */
/* Doesn't matter what we pass for status, since the core will
 * just overwrite it (because the URB has been unlinked).
+* Control urbs have the urb-actual_length pre-set, clear it
+* as well
 */
+   if (usb_endpoint_xfer_control(cur_td-urb-ep-desc))
+   cur_td-urb-actual_length = 0;
xhci_giveback_urb_in_irq(xhci, cur_td, 0);
 
/* Stop processing the cancelled list if the watchdog timer is
@@ -755,6 +759,7 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, 
struct xhci_ring *ring)
list_del_init(cur_td-td_list);
if (!list_empty(cur_td-cancelled_td_list))
list_del_init(cur_td-cancelled_td_list);
+   cur_td-urb-actual_length = 0;
xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
}
 }
@@ -792,6 +797,7 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci,
cur_td = list_first_entry(ep-cancelled_td_list,
struct xhci_td, cancelled_td_list);
list_del_init(cur_td-cancelled_td_list);
+   cur_td-urb-actual_length = 0;
xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
}
 }
@@ -1888,6 +1894,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
int ep_index;
struct xhci_ep_ctx *ep_ctx;
u32 trb_comp_code;
+   bool force_finish_td = false;
 
slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event-flags));
xdev = xhci-devs[slot_id];
@@ -1906,7 +1913,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
xhci_warn(xhci, WARN: Success on ctrl data TRB 
without IOC set??\n);
*status = -ESHUTDOWN;
-   } else {
+   } else if (*status == -EINPROGRESS) {
+   /* only set to 0 if no previous event set it earlier */
*status = 0;
}
break;
@@ -1918,6 +1926,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
break;
case COMP_STOP_INVAL:
case COMP_STOP:
+   /* we don't continue stopped TDs, so length can be set to 0 */
+   td-urb-actual_length = 0;
return finish_td(xhci, td, event_trb, event, ep, status, false);
default:
if (!xhci_requires_manual_halt_cleanup(xhci,
@@ -1928,44 +1938,26 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
trb_comp_code, ep_index);
/* else fall through */
case COMP_STALL:
-   /* Did we transfer part of the data (middle) phase? */
-   if (event_trb != ep_ring-dequeue 
-   event_trb != td-last_trb)
-   td-urb-actual_length =
-   td-urb-transfer_buffer_length -
-   EVENT_TRB_LEN(le32_to_cpu(event-transfer_len));
-   else
-   td-urb-actual_length = 0;
-
-   return finish_td(xhci, td, event_trb, event, ep, status, false);
+   /* length will be set later below if we stall on data stage */
+   td-urb-actual_length = 0;
+

Re: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-02-26 Thread Mathias Nyman 
On 26.02.2015 18:12, Mathias Nyman wrote:
 When a control transfer has a short data stage, the xHCI controller generates
 two transfer events: a COMP_SHORT_TX event that specifies the untransferred
 amount, and a COMP_SUCCESS event. But when the data stage is not short, only
 the COMP_SUCCESS event occurs. Therefore, xhci-hcd sets urb-actual_length
 to urb-transfer_buffer_length while processing the COMP_SUCCESS event,
 unless urb-actual_length was set already by a previous COMP_SHORT_TX event.
 
 The driver checks this by seeing whether urb-actual_length == 0, but this
 alone is the wrong test, as it is entirely possible for a short transfer to
 have an urb-actual_length = 0.
 
 This patch changes the xhci driver to set the urb-actual_length in advance
 to the expected value of a successful control transfer.
 The urb-actual_length is then only adjusted in case of short transfers or
 other special events, but not on COMP_SUCCESS events.
 
 This fixes a bug which affected the HSO plugin, which relies on URBs with
 urb-actual_length == 0 to halt re-submitting the RX URB in the control
 endpoint.
 
 Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com
 ---
  drivers/usb/host/xhci-ring.c | 73 
 ++--
  1 file changed, 37 insertions(+), 36 deletions(-)
 
 diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
 index b46b5b9..0e02e79 100644
 --- a/drivers/usb/host/xhci-ring.c
 +++ b/drivers/usb/host/xhci-ring.c
 @@ -732,7 +732,11 @@ remove_finished_td:
   /* Clean up the cancelled URB */
   /* Doesn't matter what we pass for status, since the core will
* just overwrite it (because the URB has been unlinked).
 +  * Control urbs have the urb-actual_length pre-set, clear it
 +  * as well
*/
 + if (usb_endpoint_xfer_control(cur_td-urb-ep-desc))
 + cur_td-urb-actual_length = 0;
   xhci_giveback_urb_in_irq(xhci, cur_td, 0);
  
   /* Stop processing the cancelled list if the watchdog timer is
 @@ -755,6 +759,7 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, 
 struct xhci_ring *ring)
   list_del_init(cur_td-td_list);
   if (!list_empty(cur_td-cancelled_td_list))
   list_del_init(cur_td-cancelled_td_list);
 + cur_td-urb-actual_length = 0;
   xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
   }
  }
 @@ -792,6 +797,7 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci,
   cur_td = list_first_entry(ep-cancelled_td_list,
   struct xhci_td, cancelled_td_list);
   list_del_init(cur_td-cancelled_td_list);
 + cur_td-urb-actual_length = 0;
   xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
   }
  }
 @@ -1888,6 +1894,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
   int ep_index;
   struct xhci_ep_ctx *ep_ctx;
   u32 trb_comp_code;
 + bool force_finish_td = false;
  
   slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event-flags));
   xdev = xhci-devs[slot_id];
 @@ -1906,7 +1913,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
   xhci_warn(xhci, WARN: Success on ctrl data TRB 
   without IOC set??\n);
   *status = -ESHUTDOWN;
 - } else {
 + } else if (*status == -EINPROGRESS) {
 + /* only set to 0 if no previous event set it earlier */
   *status = 0;
   }
   break;
 @@ -1918,6 +1926,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
   break;
   case COMP_STOP_INVAL:
   case COMP_STOP:
 + /* we don't continue stopped TDs, so length can be set to 0 */
 + td-urb-actual_length = 0;
   return finish_td(xhci, td, event_trb, event, ep, status, false);
   default:
   if (!xhci_requires_manual_halt_cleanup(xhci,
 @@ -1928,44 +1938,26 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
   trb_comp_code, ep_index);
   /* else fall through */
   case COMP_STALL:
 - /* Did we transfer part of the data (middle) phase? */
 - if (event_trb != ep_ring-dequeue 
 - event_trb != td-last_trb)
 - td-urb-actual_length =
 - td-urb-transfer_buffer_length -
 - EVENT_TRB_LEN(le32_to_cpu(event-transfer_len));
 - else
 - td-urb-actual_length = 0;
 -
 - return finish_td(xhci, td, event_trb, event, ep, status, false);
 + /* length will be set later below if we stall on data stage */
 +

Re: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-02-26 Thread Alan Stern 
On Thu, 26 Feb 2015, Mathias Nyman wrote:

 On 26.02.2015 18:12, Mathias Nyman wrote:
  When a control transfer has a short data stage, the xHCI controller 
  generates
  two transfer events: a COMP_SHORT_TX event that specifies the untransferred
  amount, and a COMP_SUCCESS event. But when the data stage is not short, only
  the COMP_SUCCESS event occurs. Therefore, xhci-hcd sets urb-actual_length
  to urb-transfer_buffer_length while processing the COMP_SUCCESS event,
  unless urb-actual_length was set already by a previous COMP_SHORT_TX event.
  
  The driver checks this by seeing whether urb-actual_length == 0, but this
  alone is the wrong test, as it is entirely possible for a short transfer to
  have an urb-actual_length = 0.
  
  This patch changes the xhci driver to set the urb-actual_length in advance
  to the expected value of a successful control transfer.
  The urb-actual_length is then only adjusted in case of short transfers or
  other special events, but not on COMP_SUCCESS events.
  
  This fixes a bug which affected the HSO plugin, which relies on URBs with
  urb-actual_length == 0 to halt re-submitting the RX URB in the control
  endpoint.
  
  Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com
  ---
   drivers/usb/host/xhci-ring.c | 73 
  ++--
   1 file changed, 37 insertions(+), 36 deletions(-)
  
  diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
  index b46b5b9..0e02e79 100644
  --- a/drivers/usb/host/xhci-ring.c
  +++ b/drivers/usb/host/xhci-ring.c
  @@ -732,7 +732,11 @@ remove_finished_td:
  /* Clean up the cancelled URB */
  /* Doesn't matter what we pass for status, since the core will
   * just overwrite it (because the URB has been unlinked).
  +* Control urbs have the urb-actual_length pre-set, clear it
  +* as well
   */
  +   if (usb_endpoint_xfer_control(cur_td-urb-ep-desc))
  +   cur_td-urb-actual_length = 0;

Now this would do the wrong thing if some data had been sent or 
received before the URB was cancelled.

Does the controller write back information about the number of bytes to
the data TRB?  Maybe it would be easier to get the values from there
instead of trying to use the event structures.

Alan Stern

--
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: [RFC PATCH] xhci: fix reporting of 0-sized URBs in control endpoints

2015-02-26 Thread Aleksander Morgado 
On Thu, Feb 26, 2015 at 5:12 PM, Mathias Nyman
mathias.ny...@linux.intel.com wrote:
 When a control transfer has a short data stage, the xHCI controller generates
 two transfer events: a COMP_SHORT_TX event that specifies the untransferred
 amount, and a COMP_SUCCESS event. But when the data stage is not short, only
 the COMP_SUCCESS event occurs. Therefore, xhci-hcd sets urb-actual_length
 to urb-transfer_buffer_length while processing the COMP_SUCCESS event,
 unless urb-actual_length was set already by a previous COMP_SHORT_TX event.

 The driver checks this by seeing whether urb-actual_length == 0, but this
 alone is the wrong test, as it is entirely possible for a short transfer to
 have an urb-actual_length = 0.

 This patch changes the xhci driver to set the urb-actual_length in advance
 to the expected value of a successful control transfer.
 The urb-actual_length is then only adjusted in case of short transfers or
 other special events, but not on COMP_SUCCESS events.

 This fixes a bug which affected the HSO plugin, which relies on URBs with
 urb-actual_length == 0 to halt re-submitting the RX URB in the control
 endpoint.

 Signed-off-by: Mathias Nyman mathias.ny...@linux.intel.com
 ---
  drivers/usb/host/xhci-ring.c | 73 
 ++--
  1 file changed, 37 insertions(+), 36 deletions(-)

 diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
 index b46b5b9..0e02e79 100644
 --- a/drivers/usb/host/xhci-ring.c
 +++ b/drivers/usb/host/xhci-ring.c
 @@ -732,7 +732,11 @@ remove_finished_td:
 /* Clean up the cancelled URB */
 /* Doesn't matter what we pass for status, since the core will
  * just overwrite it (because the URB has been unlinked).
 +* Control urbs have the urb-actual_length pre-set, clear it
 +* as well
  */
 +   if (usb_endpoint_xfer_control(cur_td-urb-ep-desc))
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, 0);

 /* Stop processing the cancelled list if the watchdog timer is
 @@ -755,6 +759,7 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, 
 struct xhci_ring *ring)
 list_del_init(cur_td-td_list);
 if (!list_empty(cur_td-cancelled_td_list))
 list_del_init(cur_td-cancelled_td_list);
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
 }
  }
 @@ -792,6 +797,7 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci,
 cur_td = list_first_entry(ep-cancelled_td_list,
 struct xhci_td, cancelled_td_list);
 list_del_init(cur_td-cancelled_td_list);
 +   cur_td-urb-actual_length = 0;
 xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
 }
  }
 @@ -1888,6 +1894,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 int ep_index;
 struct xhci_ep_ctx *ep_ctx;
 u32 trb_comp_code;
 +   bool force_finish_td = false;

 slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event-flags));
 xdev = xhci-devs[slot_id];
 @@ -1906,7 +1913,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 xhci_warn(xhci, WARN: Success on ctrl data TRB 
 without IOC set??\n);
 *status = -ESHUTDOWN;
 -   } else {
 +   } else if (*status == -EINPROGRESS) {
 +   /* only set to 0 if no previous event set it earlier 
 */
 *status = 0;
 }
 break;
 @@ -1918,6 +1926,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 break;
 case COMP_STOP_INVAL:
 case COMP_STOP:
 +   /* we don't continue stopped TDs, so length can be set to 0 */
 +   td-urb-actual_length = 0;
 return finish_td(xhci, td, event_trb, event, ep, status, 
 false);
 default:
 if (!xhci_requires_manual_halt_cleanup(xhci,
 @@ -1928,44 +1938,26 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
 struct xhci_td *td,
 trb_comp_code, ep_index);
 /* else fall through */
 case COMP_STALL:
 -   /* Did we transfer part of the data (middle) phase? */
 -   if (event_trb != ep_ring-dequeue 
 -   event_trb != td-last_trb)
 -   td-urb-actual_length =
 -   td-urb-transfer_buffer_length -
 -   
 EVENT_TRB_LEN(le32_to_cpu(event-transfer_len));
 -   else
 -   td-urb-actual_length = 0;
 -
 -   return finish_td(xhci,