Explicit status phase for DWC3

[Dan Scally <dan.scally@ideasonboard.com>]

[-- Attachment #1: Type: text/plain, Size: 6497 bytes --]

Hi Thinh


I'm trying to update the DWC3 driver to allow the status phase of a 
transaction to be explicit; meaning the gadget driver has the choice to 
either Ack or Stall _after_ the data phase so that the contents of the 
data phase can be validated. I thought that I should be able to achieve 
this by preventing dwc3_ep0_xfernotready() from calling 
dwc3_ep0_do_control_status() (relying on an "explicit_status" flag added 
to the usb_request to decide whether or not to do so) and then calling 
it manually later once the data phase was validated by the gadget driver 
(or indeed userspace). A very barebones version of my attempt to do that 
looks like this:


diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 81c486b3941c..c35436f3d3c3 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -788,6 +788,7 @@ enum dwc3_ep0_state {
         EP0_SETUP_PHASE,
         EP0_DATA_PHASE,
         EP0_STATUS_PHASE,
+       EP0_AWAITING_EXPLICIT_STATUS,
  };

  enum dwc3_link_state {
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 5d642660fd15..692a99debcd9 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -894,10 +894,14 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
                 dwc->ep0_bounced = false;
         }

-       if ((epnum & 1) && ur->actual < ur->length)
+       if ((epnum & 1) && ur->actual < ur->length) {
                 dwc3_ep0_stall_and_restart(dwc);
-       else
+       } else {
+               if (r->request.explicit_status)
+                       dwc->ep0state = EP0_AWAITING_EXPLICIT_STATUS;
+
                 dwc3_gadget_giveback(ep0, r, 0);
+       }
  }

  static void dwc3_ep0_complete_status(struct dwc3 *dwc,
@@ -1111,6 +1115,15 @@ void dwc3_ep0_end_control_data(struct dwc3 *dwc, 
struct dwc3_ep *dep)
         dep->resource_index = 0;
  }

+void dwc3_gadget_ep0_control_ack(struct usb_ep *ep)
+{
+       struct dwc3_ep                  *dep = to_dwc3_ep(ep);
+       struct dwc3                     *dwc = dep->dwc;
+
+       dwc->ep0state = EP0_STATUS_PHASE;
+       __dwc3_ep0_do_control_status(dwc, dep);
+}
+
  static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
                 const struct dwc3_event_depevt *event)
  {
@@ -1140,6 +1153,14 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
                 if (dwc->ep0_next_event != DWC3_EP0_NRDY_STATUS)
                         return;

+               /*
+                * If the request asked for an explicit status stage 
hold off
+                * on sending the status until userspace asks us to.
+                */
+               if (dwc->ep0state == EP0_AWAITING_EXPLICIT_STATUS &&
+                   !event->endpoint_number)
+                       return;
+
                 dwc->ep0state = EP0_STATUS_PHASE;

                 if (dwc->delayed_status) {
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0d89dfa6eef5..85044bbbce02 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2228,6 +2228,7 @@ static const struct usb_ep_ops dwc3_gadget_ep0_ops = {
         .dequeue        = dwc3_gadget_ep_dequeue,
         .set_halt       = dwc3_gadget_ep0_set_halt,
         .set_wedge      = dwc3_gadget_ep_set_wedge,
+       .control_ack    = dwc3_gadget_ep0_control_ack,
  };

  static const struct usb_ep_ops dwc3_gadget_ep_ops = {
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 55a56cf67d73..4fc9737b54ca 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -116,6 +116,7 @@ int __dwc3_gadget_ep0_set_halt(struct usb_ep *ep, 
int value);
  int dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
  int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
                 gfp_t gfp_flags);
+void dwc3_gadget_ep0_control_ack(struct usb_ep *ep);
  int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int 
protocol);
  void dwc3_ep0_send_delayed_status(struct dwc3 *dwc);
  void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool 
interrupt);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 3ad58b7a0824..6ee43966eafb 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -122,6 +122,8 @@ struct usb_request {

         int                     status;
         unsigned                actual;
+
+       bool                    explicit_status;
  };

  /*-------------------------------------------------------------------------*/
@@ -152,6 +154,7 @@ struct usb_ep_ops {

         int (*fifo_status) (struct usb_ep *ep);
         void (*fifo_flush) (struct usb_ep *ep);
+       void (*control_ack) (struct usb_ep *ep);
  };

  /**


And then the ack would be triggered by the gadget driver calling 
dwc3_gadget_ep0_control_ack() (in this case just through 
gadget->ep0->ops->control_ack(gadget->ep0), but probably I would 
abstract it out to the gadget layer or just mixed into usb_ep_queue() 
for ep0...).

When I do this, we do subsequently get the dwc3_ep0_xfer_complete() 
interrupt that calls dwc3_ep0_complete_status(), but the dwc3 gets stuck 
in the loop in dwc3_send_gadget_ep_cmd() immediately afterwards. It 
seems the "CMDACT" bit is never cleared (I assume that means "command 
active"...) but I can't see what's supposed to be clearing that so I 
assume it's internal firmware or something. I can't figure out how to 
proceed at this point, so I'm hoping you might have some advice about 
the right way to go about this. I attached a trace of the dwc3 events 
that shows the problem.

Side note; I realised when looking for your email that I started a 
thread about errors on the interrupt endpoint on the dwc3 and then 
abandoned it; sorry about that. Turns out the UVC gadget doesn't have 
any support for that endpoint anyway so I dropped it as low priority and 
forgot to reply to the thread.

Thanks
Dan

[-- Attachment #2: trace.tar.gz --]
[-- Type: application/gzip, Size: 30149 bytes --]