linux-kernel - Re: [PATCH] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 08 Sep 2016 15:00:50 +0300
From:   Felipe Balbi <balbi@...nel.org>
To:     Baolin Wang <baolin.wang@...aro.org>, gregkh@...uxfoundation.org
Cc:     peter@...leysoftware.com, broonie@...nel.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        baolin.wang@...aro.org
Subject: Re: [PATCH] usb: dwc3: gadget: Add disconnect checking when changing function dynamically


Hi,

Baolin Wang <baolin.wang@...aro.org> writes:
> When we change the USB function with configfs dynamically, we possibly met this
> situation: one core is doing the control transfer, another core is trying to
> unregister the USB gadget from userspace, we must wait for completing this
> control tranfer, or it will hang the controller to set the DEVCTRLHLT flag.
>
> Also adding some disconnect checking to avoid queuing any requests when the
> gadget is stopped.
>
> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>
> ---
>  drivers/usb/dwc3/ep0.c    |    8 ++++++++
>  drivers/usb/dwc3/gadget.c |   32 +++++++++++++++++++++++++++-----
>  2 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index fe79d77..11519d7 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -228,6 +228,14 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
>  	int				ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
> +	if (dwc->pullups_connected == false) {
> +		dwc3_trace(trace_dwc3_ep0,
> +			"queuing request %p to %s when gadget is disconnected",
> +			request, dep->name);
> +		ret = -ESHUTDOWN;
> +		goto out;
> +	}

this could go on its own patch, however we can use if
(!dwc->pullups_connected) instead.

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 1783406..bbac8f5 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1040,6 +1040,13 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  	struct dwc3		*dwc = dep->dwc;
>  	int			ret;
>  
> +	if (dwc->pullups_connected == false) {
> +		dwc3_trace(trace_dwc3_gadget,
> +			"queuing request %p to %s when gadget is disconnected",
> +			&req->request, dep->endpoint.name);
> +		return -ESHUTDOWN;
> +	}

ditto

> @@ -1434,6 +1441,13 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>  	if (pm_runtime_suspended(dwc->dev))
>  		return 0;
>  
> +	/*
> +	 * Per databook, when we want to stop the gadget, if a control transfer
> +	 * is still in process, complete it and get the core into setup phase.
> +	 */

Do you have the section reference for this? Which databook version are
you reading?

> +	if (!is_on && dwc->ep0state != EP0_SETUP_PHASE)
> +		return -EBUSY;
> +
>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>  	if (is_on) {
>  		if (dwc->revision <= DWC3_REVISION_187A) {
> @@ -1481,12 +1495,20 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  	struct dwc3		*dwc = gadget_to_dwc(g);
>  	unsigned long		flags;
>  	int			ret;
> +	int			timeout = 500;
>  
>  	is_on = !!is_on;
>  
> -	spin_lock_irqsave(&dwc->lock, flags);
> -	ret = dwc3_gadget_run_stop(dwc, is_on, false);
> -	spin_unlock_irqrestore(&dwc->lock, flags);
> +	do {
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		ret = dwc3_gadget_run_stop(dwc, is_on, false);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +		if (ret != -EBUSY)
> +			break;
> +
> +		udelay(10);
> +	} while (--timeout);

no, this is not a good idea at all. I'd rather see:

wait_event_timeout(dwc->wq, dwc->ep0state == EP0_SETUP_PHASE,
	jiffies + msecs_to_jiffies(500));

or, perhaps, a wait_for_completion_timeout(). Then, ep0.c needs to call
complete() everytime Status Phase completes. That's probably a better
idea ;-)

> @@ -1990,7 +2012,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
>  	 * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
>  	 * early on so DWC3_EP_BUSY flag gets cleared
>  	 */
> -	if (!dep->endpoint.desc)
> +	if (!dep->endpoint.desc || dwc->pullups_connected == false)

is this really necessary? Can we really get pullups_connect set to false
with dep->endpoint.desc still valid?

> @@ -2064,7 +2086,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
>  	 * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
>  	 * early on so DWC3_EP_BUSY flag gets cleared
>  	 */
> -	if (!dep->endpoint.desc)
> +	if (!dep->endpoint.desc || dwc->pullups_connected == false)

ditto

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (801 bytes)