| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f42dd74b-979c-85e4-bc73-0d0834fb4e38@synopsys.com>
Date: Fri, 4 Sep 2020 00:47:29 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Wesley Cheng <wcheng@...eaurora.org>,
"balbi@...nel.org" <balbi@...nel.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"jackp@...eaurora.org" <jackp@...eaurora.org>
Subject: Re: [PATCH v3] usb: dwc3: Stop active transfers before halting the
controller
Wesley Cheng wrote:
> In the DWC3 databook, for a device initiated disconnect or bus reset, the
> driver is required to send dependxfer commands for any pending transfers.
> In addition, before the controller can move to the halted state, the SW
> needs to acknowledge any pending events. If the controller is not halted
> properly, there is a chance the controller will continue accessing stale or
> freed TRBs and buffers.
>
> Signed-off-by: Wesley Cheng <wcheng@...eaurora.org>
>
> ---
> Changes in v3:
> - Removed DWC3_EP_ENABLED check from dwc3_gadget_stop_active_transfers()
> as dwc3_stop_active_transfer() has a check already in place.
> - Calling __dwc3_gadget_stop() which ensures that DWC3 interrupt events
> are cleared, and ep0 eps are cleared for the pullup disabled case. Not
> required to call __dwc3_gadget_start() on pullup enable, as the
> composite driver will execute udc_start() before calling pullup().
>
> Changes in v2:
> - Moved cleanup code to the pullup() API to differentiate between device
> disconnect and hibernation.
> - Added cleanup code to the bus reset case as well.
> - Verified the move to pullup() did not reproduce the problen using the
> same test sequence.
>
> Verified fix by adding a check for ETIMEDOUT during the run stop call.
> Shell script writing to the configfs UDC file to trigger disconnect and
> connect. Batch script to have PC execute data transfers over adb (ie adb
> push) After a few iterations, we'd run into a scenario where the
> controller wasn't halted. With the following change, no failed halts after
> many iterations.
> ---
> drivers/usb/dwc3/ep0.c | 2 +-
> drivers/usb/dwc3/gadget.c | 49 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 59f2e8c31bd1..456aa87e8778 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
> int ret;
>
> spin_lock_irqsave(&dwc->lock, flags);
> - if (!dep->endpoint.desc) {
> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> dep->name);
> ret = -ESHUTDOWN;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3ab6f118c508..73bda7eaa773 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1516,7 +1516,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
> {
> struct dwc3 *dwc = dep->dwc;
>
> - if (!dep->endpoint.desc) {
> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> dep->name);
> return -ESHUTDOWN;
> @@ -1926,6 +1926,21 @@ static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
> return 0;
> }
>
> +static void dwc3_stop_active_transfers(struct dwc3 *dwc)
> +{
> + u32 epnum;
> +
> + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
> + struct dwc3_ep *dep;
> +
> + dep = dwc->eps[epnum];
> + if (!dep)
> + continue;
> +
> + dwc3_remove_requests(dwc, dep);
> + }
> +}
> +
> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> {
> u32 reg;
> @@ -1971,6 +1986,8 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> return 0;
> }
>
> +static void __dwc3_gadget_stop(struct dwc3 *dwc);
> +
> static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> {
> struct dwc3 *dwc = gadget_to_dwc(g);
> @@ -1994,9 +2011,37 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> }
> }
>
> + /*
> + * Synchronize and disable any further event handling while controller
> + * is being enabled/disabled.
> + */
> + disable_irq(dwc->irq_gadget);
> spin_lock_irqsave(&dwc->lock, flags);
> +
> + /* Controller is not halted until pending events are acknowledged */
> + if (!is_on) {
> + u32 count;
> +
> + /*
> + * The databook explicitly mentions for a device-initiated
> + * disconnect sequence, the SW needs to ensure that it ends any
> + * active transfers.
> + */
> + dwc3_stop_active_transfers(dwc);
> + __dwc3_gadget_stop(dwc);
> +
> + count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> + count &= DWC3_GEVNTCOUNT_MASK;
> + if (count > 0) {
> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
> + dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
> + dwc->ev_buf->length;
> + }
> + }
> +
> ret = dwc3_gadget_run_stop(dwc, is_on, false);
> spin_unlock_irqrestore(&dwc->lock, flags);
> + enable_irq(dwc->irq_gadget);
>
> return ret;
> }
> @@ -3100,6 +3145,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
> }
>
> dwc3_reset_gadget(dwc);
> + /* Stop any active/pending transfers when receiving bus reset */
> + dwc3_stop_active_transfers(dwc);
>
> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> reg &= ~DWC3_DCTL_TSTCTRL_MASK;
Looks good to me.
Reviewed-by: Thinh Nguyen <thinhn@...opsys.com>
Thanks,
Thinh
Powered by blists - more mailing lists