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
