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: -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html