2022-09-01 18:49:43

by Wesley Cheng

Subject: [PATCH v6 1/5] usb: dwc3: Avoid unmapping USB requests if endxfer is not complete

If DWC3_EP_DELAYED_STOP is set during stop active transfers, then do not
continue attempting to unmap request buffers during dwc3_remove_requests().
This can lead to SMMU faults, as the controller has not stopped the
processing of the TRB. Defer this sequence to the EP0 out start, which
ensures that there are no pending SETUP transactions before issuing the
endxfer.

Reviewed-by: Thinh Nguyen <[email protected]>
Signed-off-by: Wesley Cheng <[email protected]>
---
drivers/usb/dwc3/core.h | 3 +++
drivers/usb/dwc3/ep0.c | 5 ++++-
drivers/usb/dwc3/gadget.c | 6 +++++-
3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 4fe4287dc934..7d5d2507244b 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1560,6 +1560,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
u32 param);
void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc);
+void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status);
#else
static inline int dwc3_gadget_init(struct dwc3 *dwc)
{ return 0; }
@@ -1581,6 +1582,8 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
{ return 0; }
static inline void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
{ }
+static inline void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep)
+{ }
#endif

#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 33cee0089609..61de693461da 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -293,7 +293,10 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
continue;

dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
- dwc3_stop_active_transfer(dwc3_ep, true, true);
+ if (dwc->connected)
+ dwc3_stop_active_transfer(dwc3_ep, true, true);
+ else
+ dwc3_remove_requests(dwc, dwc3_ep, -ESHUTDOWN);
}
}

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d6c0cb79ace3..6f2491fc109e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -965,12 +965,16 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
return 0;
}

-static void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status)
+void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status)
{
struct dwc3_request *req;

dwc3_stop_active_transfer(dep, true, false);

+ /* If endxfer is delayed, avoid unmapping requests */
+ if (dep->flags & DWC3_EP_DELAY_STOP)
+ return;
+
/* - giveback all requests to gadget driver */
while (!list_empty(&dep->started_list)) {
req = next_request(&dep->started_list);

2022-09-01 19:31:07

by Jack Pham

Subject: Re: [PATCH v6 1/5] usb: dwc3: Avoid unmapping USB requests if endxfer is not complete

On Thu, Sep 01, 2022 at 12:22:47PM -0700, Jack Pham wrote:
> Hi Wesley,
>
> On Thu, Sep 01, 2022 at 11:43:10AM -0700, Wesley Cheng wrote:
> > If DWC3_EP_DELAYED_STOP is set during stop active transfers, then do not
> > continue attempting to unmap request buffers during dwc3_remove_requests().
> > This can lead to SMMU faults, as the controller has not stopped the
> > processing of the TRB. Defer this sequence to the EP0 out start, which
> > ensures that there are no pending SETUP transactions before issuing the
> > endxfer.
> >
> > Reviewed-by: Thinh Nguyen <[email protected]>
> > Signed-off-by: Wesley Cheng <[email protected]>
> > ---
> > drivers/usb/dwc3/core.h | 3 +++
> > drivers/usb/dwc3/ep0.c | 5 ++++-
> > drivers/usb/dwc3/gadget.c | 6 +++++-
> > 3 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 4fe4287dc934..7d5d2507244b 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -1560,6 +1560,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
> > int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
> > u32 param);
> > void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc);
> > +void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status);
> > #else
> > static inline int dwc3_gadget_init(struct dwc3 *dwc)
> > { return 0; }
> > @@ -1581,6 +1582,8 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
> > { return 0; }
> > static inline void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
> > { }
> > +static inline void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep)
>
> Oops, you missed updating this with the additional 'status' parameter as
> well.

Actually, why even bother having this stub at all? This would only be
when neither CONFIG_USB_DWC3_GADGET nor CONFIG_USB_DWC3_DUAL_ROLE are
enabled, but ep0.c itself wouldn't even be compiled in that case.

Jack

2022-09-01 19:51:13

by Jack Pham

Subject: Re: [PATCH v6 1/5] usb: dwc3: Avoid unmapping USB requests if endxfer is not complete

Hi Wesley,

On Thu, Sep 01, 2022 at 11:43:10AM -0700, Wesley Cheng wrote:
> If DWC3_EP_DELAYED_STOP is set during stop active transfers, then do not
> continue attempting to unmap request buffers during dwc3_remove_requests().
> This can lead to SMMU faults, as the controller has not stopped the
> processing of the TRB. Defer this sequence to the EP0 out start, which
> ensures that there are no pending SETUP transactions before issuing the
> endxfer.
>
> Reviewed-by: Thinh Nguyen <[email protected]>
> Signed-off-by: Wesley Cheng <[email protected]>
> ---
> drivers/usb/dwc3/core.h | 3 +++
> drivers/usb/dwc3/ep0.c | 5 ++++-
> drivers/usb/dwc3/gadget.c | 6 +++++-
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4fe4287dc934..7d5d2507244b 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1560,6 +1560,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
> int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
> u32 param);
> void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc);
> +void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status);
> #else
> static inline int dwc3_gadget_init(struct dwc3 *dwc)
> { return 0; }
> @@ -1581,6 +1582,8 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
> { return 0; }
> static inline void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
> { }
> +static inline void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep)

Oops, you missed updating this with the additional 'status' parameter as
well.

Jack