From patchwork Tue Jan 24 14:27:13 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Dan Scally X-Patchwork-Id: 13114241 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7F6FDC54E94 for ; Tue, 24 Jan 2023 14:27:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234883AbjAXO1V (ORCPT ); Tue, 24 Jan 2023 09:27:21 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38524 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234881AbjAXO1U (ORCPT ); Tue, 24 Jan 2023 09:27:20 -0500 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B4AA745BCF for ; Tue, 24 Jan 2023 06:27:17 -0800 (PST) Received: from [192.168.0.43] (cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 1C68F8A9; Tue, 24 Jan 2023 15:27:16 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1674570436; bh=h+1QcwUqxBGq74j++jvQAbtBQRut6I9xEyw+Pu/040I=; h=Date:To:Cc:From:Subject:From; b=U8xslfxGVd8UI8AaMYXzBfvizAgVxxkc7aoCP+b/rZJmdfobrJkb0RSDOWueIFAW3 M9ggwgZ+kE7AyxDFeX4MyNfx37x9uqYM5E6k06OpfjmTbCXVFOM7d8jvyHqlBtdKPD HFrQybUWaCnGFtZULb8FazcsDjPINHHjkYYehCgQ= Message-ID: <9ce226b4-90c6-94c4-a5ad-bd623409bc51@ideasonboard.com> Date: Tue, 24 Jan 2023 14:27:13 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Content-Language: en-US To: Thinh Nguyen Cc: linux-usb@vger.kernel.org From: Dan Scally Subject: Explicit status phase for DWC3 Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org 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: 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 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);  };  /**