Re: [PATCH v3] xhci: fix reporting of 0-sized URBs in control endpoint
On Thu, 26 Feb 2015, Mathias Nyman wrote: > On 26.02.2015 16:57, Alan Stern wrote: > > On Thu, 26 Feb 2015, Mathias Nyman wrote: > > > >> I'm starting to like your idea of setting the urb->actual_length in > >> advance, > >> It may actually simplify things. > > > > But it will make unlinking more difficult. Also, what will you do if > > there is more than one TRB? > > > > current xhci driver can't handle more than one data trb in control tranfers: > xhci-ring.c, Yes, that's right. I was thinking about bulk transfers. I guess they don't suffer from this problem, though. > Shouldn't control urbs only be given back when they finish (SUCCESS, STALL, > SHORT etc), > are dequeued, or some major host failure causes us to empty the rings? Yes. In fact, that's true for every URB, not just control. > I thought it would be enough to set urb->actual_length = 0 for the ctrl URBs > in all other > cases than short or successful completion? urb->actual_length should always be set to the number of bytes actually transferred, as closely as you can tell. For example, suppose you have a 1500-byte transfer. If the first packet successfully sends 1024 bytes but the second packet fails (so the entire URB fails), actual_length should be set to 1024. 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: [PATCH v3] xhci: fix reporting of 0-sized URBs in control endpoint
On 26.02.2015 16:57, Alan Stern wrote: > On Thu, 26 Feb 2015, Mathias Nyman wrote: > >> I'm starting to like your idea of setting the urb->actual_length in advance, >> It may actually simplify things. > > But it will make unlinking more difficult. Also, what will you do if > there is more than one TRB? > current xhci driver can't handle more than one data trb in control tranfers: xhci-ring.c, int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags, struct urb *urb, int slot_id, unsigned int ep_index) ... /* 1 TRB for setup, 1 for status */ num_trbs = 2; /* * Don't need to check if we need additional event data and normal TRBs, * since data in control transfers will never get bigger than 16MB * XXX: can we get a buffer that crosses 64KB boundaries? */ if (urb->transfer_buffer_length > 0) num_trbs++; Shouldn't control urbs only be given back when they finish (SUCCESS, STALL, SHORT etc), are dequeued, or some major host failure causes us to empty the rings? I thought it would be enough to set urb->actual_length = 0 for the ctrl URBs in all other cases than short or successful completion? I'll send a RFC so you can see if it makes sense to try out this path, or If Im only shooting myself in the foot. -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: [PATCH v3] xhci: fix reporting of 0-sized URBs in control endpoint
On Thu, 26 Feb 2015, Mathias Nyman wrote: > I'm starting to like your idea of setting the urb->actual_length in advance, > It may actually simplify things. But it will make unlinking more difficult. Also, what will you do if there is more than one TRB? 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: [PATCH v3] xhci: fix reporting of 0-sized URBs in control endpoint
... > > The second event is always COMP_SUCCESS and the event->transfer_len is > always set to 0 in that one. The 3 cases I've seen are: > > case 1: 1 event on last TRB > COMP_SUCCESS, event->len=0 > > case 2: short event but with data > COMP_SHORT_TX, event->len < urb->transfer_buffer_len > COMP_SUCCESS, event->len=0 > > case 3: short event with no data > COMP_SHORT_TX, event->len = urb->transfer_buffer_len > COMP_SUCCESS, event->len=0 > Ok, I was hoping COMP_SUCCESS event->len in case 2 and 3 would show the same value as the previous COMP_SHORT_TX event->len >>> >>> The other thing I thought of was to somehow always initialize the URB >>> actual length to the transfer buffer length from the very beginning, >>> and only update it if a COMP_SHORT_TX event is received. Not sure if >>> that would be much more complex to handle, though. >>> >> >> This could be an option, need to look into it. > I'm starting to like your idea of setting the urb->actual_length in advance, It may actually simplify things. I already started implementing a testpatch, will send it shortly If you'd like to test it with your device and hso driver. -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: [PATCH v3] xhci: fix reporting of 0-sized URBs in control endpoint
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 must set 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. >>> >>> I think that the only case you should see a COMP_SUCCESS after a >>> COMP_SHORT_TX >>> is if xhci hits a link TRB while automatically moving to the next TRB after >>> the >>> short packet. >>> >>> If Event Data TRBs are used, or a later TRB in that TD has the IOC flag set >>> then xhci should just >>> generate a second transfer event with the same COMP_SHORT_TX completion >>> code. >>> (xhci specs section 4.10.1.1) >>> >> >> Well, I can only speak for this usecase I have here with the Option >> HSO modem; in this case the COMP_SHORT_TX+COMP_SUCCESS pair happens >> always, as the hso driver submits a URB with a 1024 byte buffer, and >> the modem usually replies with AT responses character by character; >> for each character I end up getting both events: first with >> COMP_SHORT_TX and the event length set to 1023, second with >> COMP_SUCESS and event length set to 0. >> > > Looking at the code it seems that xhci controllers after 0.96 generate a > spurious COMP_SUCCESS after short packet, code says: > > /* In xhci controllers which follow xhci 1.0 spec gives a spurious > * success event after a short transfer. This quirk will ignore such > * spurious event. > */ > if (xhci->hci_version > 0x96) > xhci->quirks |= XHCI_SPURIOUS_SUCCESS; > >> I read the xhci specs as well and I also got the impression that there >> wasn't anything explicitly stating that a COMP_SUCCESS event always >> follows a COMP_SHORT_TX. But, the code already assumes that if you get >> a COMP_SHORT_TX event for a TRB which isn't the last one, you should >> still get an event for the last TRB (i.e. finish_td() isn't called for >> a COMP_SHORT_TX event if the TRB isn't the first one (event_trb != >> ep_ring->dequeue) and if it isn't the last one (event_trb != >> td->last_trb). > > Thats right, we always set the IOC (interrupt on completion) in the last TRB > of > a control transfer, so it will always interrupt, generating a transfer event. > > I read through your previous mails where you were investigating this and also > looked a bit > in more detail at the xhci code. I now understand better the issue. Its > clearly a bug in xhci driver. > > Can you still add some debugging and check the if the second event after the > COMP_SHORT_TX really > is a COMP_SUCCESS ? and also what it says about the transfer length. > > printing out these values for the second event should do it: > GET_COMP_CODE(le32_to_cpu(event->transfer_len)) > EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) > The second event is always COMP_SUCCESS and the event->transfer_len is always set to 0 in that one. The 3 cases I've seen are: case 1: 1 event on last TRB COMP_SUCCESS, event->len=0 case 2: short event but with data COMP_SHORT_TX, event->len < urb->transfer_buffer_len COMP_SUCCESS, event->len=0 case 3: short event with no data COMP_SHORT_TX, event->len = urb->transfer_buffer_len COMP_SUCCESS, event->len=0 > Right now process_ctrl_td() sets the urb->actual_length based on TRB position > in TD (first, last, neither) and > what value urb->actual_length contains. I think that by checking the actual > event completion code, and the event > reported remaining length we could get this correct without adding any > additional fields to struct xhci_td. > The problem with that logic is that just by checking the last event completion code and event->length we cannot know whether there was a previous COMP_SHORT_TX event. i.e. when processing that last event you wouldn't know whether it was case 1, case 2 or case 3 from above. > If I find the time I'll rewrite this part, if not then I'll add your patch, > (v4) > If you want to suggest any other approach, let me know and I'll spend time with it. > >> >>> 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 should be fixed, handling short packets look a bit messy in general >>> right now >>> This patch changes the xhci driver to rely not only on the urb->actual_length, but also on the ep_ring->last_td_was_short flag, which is set to true when a COMP_SHORT_TX event is received. 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
Re: [PATCH v3] xhci: fix reporting of 0-sized URBs in control endpoint
On 23.02.2015 19:02, Aleksander Morgado wrote: > On Mon, Feb 23, 2015 at 4:23 PM, Mathias Nyman > wrote: >> Hi >> >> On 23.02.2015 13:52, Aleksander Morgado 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 must set 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. >>> >> >> I think that the only case you should see a COMP_SUCCESS after a >> COMP_SHORT_TX >> is if xhci hits a link TRB while automatically moving to the next TRB after >> the >> short packet. >> >> If Event Data TRBs are used, or a later TRB in that TD has the IOC flag set >> then xhci should just >> generate a second transfer event with the same COMP_SHORT_TX completion code. >> (xhci specs section 4.10.1.1) >> > > Well, I can only speak for this usecase I have here with the Option > HSO modem; in this case the COMP_SHORT_TX+COMP_SUCCESS pair happens > always, as the hso driver submits a URB with a 1024 byte buffer, and > the modem usually replies with AT responses character by character; > for each character I end up getting both events: first with > COMP_SHORT_TX and the event length set to 1023, second with > COMP_SUCESS and event length set to 0. > Looking at the code it seems that xhci controllers after 0.96 generate a spurious COMP_SUCCESS after short packet, code says: /* In xhci controllers which follow xhci 1.0 spec gives a spurious * success event after a short transfer. This quirk will ignore such * spurious event. */ if (xhci->hci_version > 0x96) xhci->quirks |= XHCI_SPURIOUS_SUCCESS; > I read the xhci specs as well and I also got the impression that there > wasn't anything explicitly stating that a COMP_SUCCESS event always > follows a COMP_SHORT_TX. But, the code already assumes that if you get > a COMP_SHORT_TX event for a TRB which isn't the last one, you should > still get an event for the last TRB (i.e. finish_td() isn't called for > a COMP_SHORT_TX event if the TRB isn't the first one (event_trb != > ep_ring->dequeue) and if it isn't the last one (event_trb != > td->last_trb). Thats right, we always set the IOC (interrupt on completion) in the last TRB of a control transfer, so it will always interrupt, generating a transfer event. I read through your previous mails where you were investigating this and also looked a bit in more detail at the xhci code. I now understand better the issue. Its clearly a bug in xhci driver. Can you still add some debugging and check the if the second event after the COMP_SHORT_TX really is a COMP_SUCCESS ? and also what it says about the transfer length. printing out these values for the second event should do it: GET_COMP_CODE(le32_to_cpu(event->transfer_len)) EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) Right now process_ctrl_td() sets the urb->actual_length based on TRB position in TD (first, last, neither) and what value urb->actual_length contains. I think that by checking the actual event completion code, and the event reported remaining length we could get this correct without adding any additional fields to struct xhci_td. If I find the time I'll rewrite this part, if not then I'll add your patch, (v4) > >> >>> 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 should be fixed, handling short packets look a bit messy in general >> right now >> >>> >>> This patch changes the xhci driver to rely not only on the >>> urb->actual_length, >>> but also on the ep_ring->last_td_was_short flag, which is set to true when a >>> COMP_SHORT_TX event is received. >>> >>> 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: Aleksander Morgado >>> --- >>> >>> Hey, >>> >>> This is the third update of the patch: >>> >>> * v2 modified the commit message to make it shorter and clearer. >>> >>> * v3 updated the format of the commented lines in the patch. >>> >>> Cheers! >>> >>> --- >>> drivers/usb/host/xhci-ring.c | 16 +++- >>> 1 file changed, 11 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >>> index 88da8d6..eda3276 100644 >>> --- a/drivers/usb/host/xhci-ring.c >>> +++ b/drivers/usb/host/xhci-ring.c >>> @@ -1955,7
Re: [PATCH v3] xhci: fix reporting of 0-sized URBs in control endpoint
On Mon, Feb 23, 2015 at 4:23 PM, Mathias Nyman
wrote:
> Hi
>
> On 23.02.2015 13:52, Aleksander Morgado 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 must set 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.
>>
>
> I think that the only case you should see a COMP_SUCCESS after a COMP_SHORT_TX
> is if xhci hits a link TRB while automatically moving to the next TRB after
> the
> short packet.
>
> If Event Data TRBs are used, or a later TRB in that TD has the IOC flag set
> then xhci should just
> generate a second transfer event with the same COMP_SHORT_TX completion code.
> (xhci specs section 4.10.1.1)
>
Well, I can only speak for this usecase I have here with the Option
HSO modem; in this case the COMP_SHORT_TX+COMP_SUCCESS pair happens
always, as the hso driver submits a URB with a 1024 byte buffer, and
the modem usually replies with AT responses character by character;
for each character I end up getting both events: first with
COMP_SHORT_TX and the event length set to 1023, second with
COMP_SUCESS and event length set to 0.
I read the xhci specs as well and I also got the impression that there
wasn't anything explicitly stating that a COMP_SUCCESS event always
follows a COMP_SHORT_TX. But, the code already assumes that if you get
a COMP_SHORT_TX event for a TRB which isn't the last one, you should
still get an event for the last TRB (i.e. finish_td() isn't called for
a COMP_SHORT_TX event if the TRB isn't the first one (event_trb !=
ep_ring->dequeue) and if it isn't the last one (event_trb !=
td->last_trb).
>
>> 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 should be fixed, handling short packets look a bit messy in general
> right now
>
>>
>> This patch changes the xhci driver to rely not only on the
>> urb->actual_length,
>> but also on the ep_ring->last_td_was_short flag, which is set to true when a
>> COMP_SHORT_TX event is received.
>>
>> 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: Aleksander Morgado
>> ---
>>
>> Hey,
>>
>> This is the third update of the patch:
>>
>> * v2 modified the commit message to make it shorter and clearer.
>>
>> * v3 updated the format of the commented lines in the patch.
>>
>> Cheers!
>>
>> ---
>> drivers/usb/host/xhci-ring.c | 16 +++-
>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 88da8d6..eda3276 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -1955,7 +1955,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci,
>> struct xhci_td *td,
>> /* Did we already see a short data
>>* stage? */
>> *status = -EREMOTEIO;
>> - } else {
>> + } else if (!ep_ring->last_td_was_short) {
>> td->urb->actual_length =
>> td->urb->transfer_buffer_length;
>> }
>> @@ -2447,10 +2447,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>> ret = skip_isoc_td(xhci, td, event, ep, &status);
>> goto cleanup;
>> }
>> - if (trb_comp_code == COMP_SHORT_TX)
>> - ep_ring->last_td_was_short = true;
>> - else
>> - ep_ring->last_td_was_short = false;
>>
>> if (ep->skip) {
>> xhci_dbg(xhci, "Found td. Clear skip flag.\n");
>> @@ -2484,6 +2480,16 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>> ret = process_bulk_intr_td(xhci, td, event_trb, event,
>>ep, &status);
>>
>> + /*
>> + * Flag whether the just processed TRB was short. Do it after
>> + * processing, so that the processor methods can also use this
>> + * flag.
>> + */
>> + if (trb_comp_code == COMP_SHORT_TX)
>> + ep_ring->last_td_was_short = true;
>> + else
>> + ep_ring->last_td_was_short = false;
>> +
>
> How about the case where we only get one COMP_SHORT_TX event for that control
> transfer,
> xhci then advances to the next TD
Re: [PATCH v3] xhci: fix reporting of 0-sized URBs in control endpoint
Hi
On 23.02.2015 13:52, Aleksander Morgado 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 must set 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.
>
I think that the only case you should see a COMP_SUCCESS after a COMP_SHORT_TX
is if xhci hits a link TRB while automatically moving to the next TRB after the
short packet.
If Event Data TRBs are used, or a later TRB in that TD has the IOC flag set
then xhci should just
generate a second transfer event with the same COMP_SHORT_TX completion code.
(xhci specs section 4.10.1.1)
> 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 should be fixed, handling short packets look a bit messy in general right
now
>
> This patch changes the xhci driver to rely not only on the urb->actual_length,
> but also on the ep_ring->last_td_was_short flag, which is set to true when a
> COMP_SHORT_TX event is received.
>
> 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: Aleksander Morgado
> ---
>
> Hey,
>
> This is the third update of the patch:
>
> * v2 modified the commit message to make it shorter and clearer.
>
> * v3 updated the format of the commented lines in the patch.
>
> Cheers!
>
> ---
> drivers/usb/host/xhci-ring.c | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 88da8d6..eda3276 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1955,7 +1955,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci,
> struct xhci_td *td,
> /* Did we already see a short data
>* stage? */
> *status = -EREMOTEIO;
> - } else {
> + } else if (!ep_ring->last_td_was_short) {
> td->urb->actual_length =
> td->urb->transfer_buffer_length;
> }
> @@ -2447,10 +2447,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> ret = skip_isoc_td(xhci, td, event, ep, &status);
> goto cleanup;
> }
> - if (trb_comp_code == COMP_SHORT_TX)
> - ep_ring->last_td_was_short = true;
> - else
> - ep_ring->last_td_was_short = false;
>
> if (ep->skip) {
> xhci_dbg(xhci, "Found td. Clear skip flag.\n");
> @@ -2484,6 +2480,16 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> ret = process_bulk_intr_td(xhci, td, event_trb, event,
>ep, &status);
>
> + /*
> + * Flag whether the just processed TRB was short. Do it after
> + * processing, so that the processor methods can also use this
> + * flag.
> + */
> + if (trb_comp_code == COMP_SHORT_TX)
> + ep_ring->last_td_was_short = true;
> + else
> + ep_ring->last_td_was_short = false;
> +
How about the case where we only get one COMP_SHORT_TX event for that control
transfer,
xhci then advances to the next TD, which completes successfully? That
successful TD won't get
its td->urb->actual length set because the last_td_was_short flag it still set?
-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
