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

[Peter Chen <hzpeterchen@gmail.com>]

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: