WARN: Success on ctrl setup TRB without IOC set??

[Mathias Nyman <mathias.nyman@linux.intel.com>]

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