* WARN: Success on ctrl setup TRB without IOC set??
@ 2018-02-09 1:59 Peter Chen
0 siblings, 0 replies; 5+ messages in thread
From: Peter Chen @ 2018-02-09 1:59 UTC (permalink / raw)
To: Mathias Nyman; +Cc: mathias.nyman, linux-usb
On Wed, Feb 07, 2018 at 03:43:23PM +0200, Mathias Nyman wrote:
> On 07.02.2018 11:45, Peter Chen wrote:
> >Hi Mathias,
> >
> >I am implementing USB2 EHSET SINGLE_STEP_SET_FEATURE Test for XHCI port,
> >(see ehset_single_step_set_feature for EHCI), it needs to set IOC
> >for setup packet, and software waits 15 seconds before DATA + STATUS stage.
> >After porting such design for XHCI, it triggers above warning, and
> >returns -ESHUTDOWN for URB. Any reasons why we don't allow completion
> >interrupt for SETUP stage? Thanks.
> >
>
> Current xhci driver control transfer implementation doesn't support
> queuing a control transfer in parts. we set the IOC only for the status
> stage, and we always queue a status stage (and possibly data stage) automatically
> when queuing a control transfer URB.
>
> If we get a success event for the SETUP stage the driver will finish the TD and
> return the whole URB.
>
> For testing purposes you would need to make sure we don't call finish_td() for
> a success event at the SETUP stage, something like this (untested):
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index c5cbc68..f6d005e 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2014,12 +2014,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> switch (trb_comp_code) {
> case COMP_SUCCESS:
> - if (trb_type != TRB_STATUS) {
> - xhci_warn(xhci, "WARN: Success on ctrl %s TRB without IOC set?\n",
> - (trb_type == TRB_DATA) ? "data" : "setup");
> - *status = -ESHUTDOWN;
> - break;
> - }
> + if (trb_type == TRB_SETUP)
> + return 0;
> *status = 0;
> break;
> case COMP_SHORT_PACKET:
>
Thanks, Mathias. I need to call finish_td since it needs to
giveback and cleanup URB. And I see below code at process_ctrl_td,
why we can't call it? In fact, I only see warning message, but
without any errors for transactions. How about only delete message
or change debug level as xhci_dbg?
/* stopped at setup stage, no data transferred */
if (trb_type == TRB_SETUP)
goto finish_td;
^ permalink raw reply [flat|nested] 5+ messages in thread* WARN: Success on ctrl setup TRB without IOC set??
@ 2018-02-24 2:35 Peter Chen
0 siblings, 0 replies; 5+ messages in thread
From: Peter Chen @ 2018-02-24 2:35 UTC (permalink / raw)
To: Mathias Nyman; +Cc: mathias.nyman, linux-usb
On Fri, Feb 09, 2018 at 11:39:28AM +0200, Mathias Nyman wrote:
> On 09.02.2018 04:03, Peter Chen wrote:
> >On Fri, Feb 09, 2018 at 09:59:46AM +0800, Peter Chen wrote:
> >>On Wed, Feb 07, 2018 at 03:43:23PM +0200, Mathias Nyman wrote:
> >>>On 07.02.2018 11:45, Peter Chen wrote:
> >>>>Hi Mathias,
> >>>>
> >>>>I am implementing USB2 EHSET SINGLE_STEP_SET_FEATURE Test for XHCI port,
> >>>>(see ehset_single_step_set_feature for EHCI), it needs to set IOC
> >>>>for setup packet, and software waits 15 seconds before DATA + STATUS stage.
> >>>>After porting such design for XHCI, it triggers above warning, and
> >>>>returns -ESHUTDOWN for URB. Any reasons why we don't allow completion
> >>>>interrupt for SETUP stage? Thanks.
> >>>>
> >>>
> >>>Current xhci driver control transfer implementation doesn't support
> >>>queuing a control transfer in parts. we set the IOC only for the status
> >>>stage, and we always queue a status stage (and possibly data stage) automatically
> >>>when queuing a control transfer URB.
> >>>
> >>>If we get a success event for the SETUP stage the driver will finish the TD and
> >>>return the whole URB.
> >>>
> >>>For testing purposes you would need to make sure we don't call finish_td() for
> >>>a success event at the SETUP stage, something like this (untested):
> >>>
> >>>diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> >>>index c5cbc68..f6d005e 100644
> >>>--- a/drivers/usb/host/xhci-ring.c
> >>>+++ b/drivers/usb/host/xhci-ring.c
> >>>@@ -2014,12 +2014,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> >>> switch (trb_comp_code) {
> >>> case COMP_SUCCESS:
> >>>- if (trb_type != TRB_STATUS) {
> >>>- xhci_warn(xhci, "WARN: Success on ctrl %s TRB without IOC set?\n",
> >>>- (trb_type == TRB_DATA) ? "data" : "setup");
> >>>- *status = -ESHUTDOWN;
> >>>- break;
> >>>- }
> >>>+ if (trb_type == TRB_SETUP)
> >>>+ return 0;
> >>> *status = 0;
> >>> break;
> >>> case COMP_SHORT_PACKET:
> >>>
> >>
> >>Thanks, Mathias. I need to call finish_td since it needs to
> >>giveback and cleanup URB. And I see below code at process_ctrl_td,
> >>why we can't call it? In fact, I only see warning message, but
> >>without any errors for transactions. How about only delete message
> >>or change debug level as xhci_dbg?
> >
> >Should be "or change debug message level and *status value as zero."
> >
>
Sorry to reply late due to New Year holiday at China.
> Ah, ok, I thought you didn't want to give back the URB before the STATUS stage.
>
No, I want to give back the URB after SETUP has finished.
The whole process for test includes below steps:
- Prepare the 1st URB for SETUP
- After the 1st URB is given back, wait 15s.
- Prepare the 2nd URB for DATA + STATUS
- The 2nd URB needs to be given back.
> Note that If you call finish_td on a successful SETUP stage it will give back the
> entire URB, and move the software dequeue pointer past the STATUS stage TRB.
> So software and hardware dequeue pointers are out of sync.
> If IOC is then set on DATA or STATUS stage TRB the driver will complain about
> either
> "WARN Event TRB for slot %d ep %d with no TDs queued?"
> or
> "ERROR Transfer event TRB DMA ptr not part of current TD ..."
> as it thinks all TRBs of this TD are handled and already moved past all them.
I did not meet above errors, I use two URBs for SETUP and DATA + STATUS.
>
> Using xhci_dbg() instead of xhci_warn works for me
>
How about below changes?
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index daa94c3..837af86 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2010,12 +2010,9 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
switch (trb_comp_code) {
case COMP_SUCCESS:
- if (trb_type != TRB_STATUS) {
- xhci_warn(xhci, "WARN: Success on ctrl %s TRB without IOC set?\n",
+ if (trb_type != TRB_STATUS)
+ xhci_dbg(xhci, "on ctrl %s TRB without IOC set?\n",
(trb_type == TRB_DATA) ? "data" : "setup");
- *status = -ESHUTDOWN;
- break;
- }
*status = 0;
break;
case COMP_SHORT_PACKET:
^ permalink raw reply related [flat|nested] 5+ messages in thread* WARN: Success on ctrl setup TRB without IOC set??
@ 2018-02-09 9:39 Mathias Nyman
0 siblings, 0 replies; 5+ messages in thread
From: Mathias Nyman @ 2018-02-09 9:39 UTC (permalink / raw)
To: Peter Chen; +Cc: mathias.nyman, linux-usb
On 09.02.2018 04:03, Peter Chen wrote:
> On Fri, Feb 09, 2018 at 09:59:46AM +0800, Peter Chen wrote:
>> On Wed, Feb 07, 2018 at 03:43:23PM +0200, Mathias Nyman wrote:
>>> On 07.02.2018 11:45, Peter Chen wrote:
>>>> Hi Mathias,
>>>>
>>>> I am implementing USB2 EHSET SINGLE_STEP_SET_FEATURE Test for XHCI port,
>>>> (see ehset_single_step_set_feature for EHCI), it needs to set IOC
>>>> for setup packet, and software waits 15 seconds before DATA + STATUS stage.
>>>> After porting such design for XHCI, it triggers above warning, and
>>>> returns -ESHUTDOWN for URB. Any reasons why we don't allow completion
>>>> interrupt for SETUP stage? Thanks.
>>>>
>>>
>>> Current xhci driver control transfer implementation doesn't support
>>> queuing a control transfer in parts. we set the IOC only for the status
>>> stage, and we always queue a status stage (and possibly data stage) automatically
>>> when queuing a control transfer URB.
>>>
>>> If we get a success event for the SETUP stage the driver will finish the TD and
>>> return the whole URB.
>>>
>>> For testing purposes you would need to make sure we don't call finish_td() for
>>> a success event at the SETUP stage, something like this (untested):
>>>
>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>>> index c5cbc68..f6d005e 100644
>>> --- a/drivers/usb/host/xhci-ring.c
>>> +++ b/drivers/usb/host/xhci-ring.c
>>> @@ -2014,12 +2014,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
>>> switch (trb_comp_code) {
>>> case COMP_SUCCESS:
>>> - if (trb_type != TRB_STATUS) {
>>> - xhci_warn(xhci, "WARN: Success on ctrl %s TRB without IOC set?\n",
>>> - (trb_type == TRB_DATA) ? "data" : "setup");
>>> - *status = -ESHUTDOWN;
>>> - break;
>>> - }
>>> + if (trb_type == TRB_SETUP)
>>> + return 0;
>>> *status = 0;
>>> break;
>>> case COMP_SHORT_PACKET:
>>>
>>
>> Thanks, Mathias. I need to call finish_td since it needs to
>> giveback and cleanup URB. And I see below code at process_ctrl_td,
>> why we can't call it? In fact, I only see warning message, but
>> without any errors for transactions. How about only delete message
>> or change debug level as xhci_dbg?
>
> Should be "or change debug message level and *status value as zero."
>
Ah, ok, I thought you didn't want to give back the URB before the STATUS stage.
Note that If you call finish_td on a successful SETUP stage it will give back the
entire URB, and move the software dequeue pointer past the STATUS stage TRB.
So software and hardware dequeue pointers are out of sync.
If IOC is then set on DATA or STATUS stage TRB the driver will complain about
either
"WARN Event TRB for slot %d ep %d with no TDs queued?"
or
"ERROR Transfer event TRB DMA ptr not part of current TD ..."
as it thinks all TRBs of this TD are handled and already moved past all them.
Using xhci_dbg() instead of xhci_warn works for me
-Mathias
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* WARN: Success on ctrl setup TRB without IOC set??
@ 2018-02-09 2:03 Peter Chen
0 siblings, 0 replies; 5+ messages in thread
From: Peter Chen @ 2018-02-09 2:03 UTC (permalink / raw)
To: Mathias Nyman; +Cc: mathias.nyman, linux-usb
On Fri, Feb 09, 2018 at 09:59:46AM +0800, Peter Chen wrote:
> On Wed, Feb 07, 2018 at 03:43:23PM +0200, Mathias Nyman wrote:
> > On 07.02.2018 11:45, Peter Chen wrote:
> > >Hi Mathias,
> > >
> > >I am implementing USB2 EHSET SINGLE_STEP_SET_FEATURE Test for XHCI port,
> > >(see ehset_single_step_set_feature for EHCI), it needs to set IOC
> > >for setup packet, and software waits 15 seconds before DATA + STATUS stage.
> > >After porting such design for XHCI, it triggers above warning, and
> > >returns -ESHUTDOWN for URB. Any reasons why we don't allow completion
> > >interrupt for SETUP stage? Thanks.
> > >
> >
> > Current xhci driver control transfer implementation doesn't support
> > queuing a control transfer in parts. we set the IOC only for the status
> > stage, and we always queue a status stage (and possibly data stage) automatically
> > when queuing a control transfer URB.
> >
> > If we get a success event for the SETUP stage the driver will finish the TD and
> > return the whole URB.
> >
> > For testing purposes you would need to make sure we don't call finish_td() for
> > a success event at the SETUP stage, something like this (untested):
> >
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index c5cbc68..f6d005e 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -2014,12 +2014,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> > switch (trb_comp_code) {
> > case COMP_SUCCESS:
> > - if (trb_type != TRB_STATUS) {
> > - xhci_warn(xhci, "WARN: Success on ctrl %s TRB without IOC set?\n",
> > - (trb_type == TRB_DATA) ? "data" : "setup");
> > - *status = -ESHUTDOWN;
> > - break;
> > - }
> > + if (trb_type == TRB_SETUP)
> > + return 0;
> > *status = 0;
> > break;
> > case COMP_SHORT_PACKET:
> >
>
> Thanks, Mathias. I need to call finish_td since it needs to
> giveback and cleanup URB. And I see below code at process_ctrl_td,
> why we can't call it? In fact, I only see warning message, but
> without any errors for transactions. How about only delete message
> or change debug level as xhci_dbg?
Should be "or change debug message level and *status value as zero."
>
> /* stopped at setup stage, no data transferred */
> if (trb_type == TRB_SETUP)
> goto finish_td;
>
> --
>
^ permalink raw reply [flat|nested] 5+ messages in thread* WARN: Success on ctrl setup TRB without IOC set??
@ 2018-02-07 13:43 Mathias Nyman
0 siblings, 0 replies; 5+ messages in thread
From: Mathias Nyman @ 2018-02-07 13:43 UTC (permalink / raw)
To: Peter Chen, mathias.nyman; +Cc: linux-usb
On 07.02.2018 11:45, Peter Chen wrote:
> Hi Mathias,
>
> I am implementing USB2 EHSET SINGLE_STEP_SET_FEATURE Test for XHCI port,
> (see ehset_single_step_set_feature for EHCI), it needs to set IOC
> for setup packet, and software waits 15 seconds before DATA + STATUS stage.
> After porting such design for XHCI, it triggers above warning, and
> returns -ESHUTDOWN for URB. Any reasons why we don't allow completion
> interrupt for SETUP stage? Thanks.
>
Current xhci driver control transfer implementation doesn't support
queuing a control transfer in parts. we set the IOC only for the status
stage, and we always queue a status stage (and possibly data stage) automatically
when queuing a control transfer URB.
If we get a success event for the SETUP stage the driver will finish the TD and
return the whole URB.
For testing purposes you would need to make sure we don't call finish_td() for
a success event at the SETUP stage, something like this (untested):
Mathias
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index c5cbc68..f6d005e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2014,12 +2014,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
switch (trb_comp_code) {
case COMP_SUCCESS:
- if (trb_type != TRB_STATUS) {
- xhci_warn(xhci, "WARN: Success on ctrl %s TRB without IOC set?\n",
- (trb_type == TRB_DATA) ? "data" : "setup");
- *status = -ESHUTDOWN;
- break;
- }
+ if (trb_type == TRB_SETUP)
+ return 0;
*status = 0;
break;
case COMP_SHORT_PACKET:
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-24 2:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-09 1:59 WARN: Success on ctrl setup TRB without IOC set?? Peter Chen
-- strict thread matches above, loose matches on Subject: below --
2018-02-24 2:35 Peter Chen
2018-02-09 9:39 Mathias Nyman
2018-02-09 2:03 Peter Chen
2018-02-07 13:43 Mathias Nyman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).