linux-kernel - Re: [PATCH v2] usb: dwc3: gadget: Stop EP0 transfers during pullup disable

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] [day] [month] [year] [list]

Message-ID: <9e11d0dc-c043-6b55-2c33-fb1a55b18156@codeaurora.org>
Date:   Mon, 23 Aug 2021 03:59:29 -0700
From:   Wesley Cheng <wcheng@...eaurora.org>
To:     Felipe Balbi <balbi@...nel.org>
Cc:     gregkh@...uxfoundation.org, Thinh.Nguyen@...opsys.com,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        jackp@...eaurora.org
Subject: Re: [PATCH v2] usb: dwc3: gadget: Stop EP0 transfers during pullup
 disable

Hi Felipe,

On 8/23/2021 2:34 AM, Felipe Balbi wrote:
> 
> Wesley Cheng <wcheng@...eaurora.org> writes:
> 
>> During a USB cable disconnect, or soft disconnect scenario, a pending
>> SETUP transaction may not be completed, leading to the following
>> error:
>>
>>     dwc3 a600000.dwc3: timed out waiting for SETUP phase
>>
>> If this occurs, then the entire pullup disable routine is skipped and
>> proper cleanup and halting of the controller does not complete.
> 
> nit: might want to add a blank line between paragraphs to aid
> readability
> 
>> Instead of returning an error (which is ignored from the UDC
>> perspective), allow the pullup disable to routine to continue, which
>                                          ^^
>                                          remove this?
> 
>> will also handle disabling of EP0/1.  This will end any active
>> transfers as well.  Ensure to clear any delayed_status as well, as the
>> timeout could happen within the STATUS stage.
>>
>> Signed-off-by: Wesley Cheng <wcheng@...eaurora.org>
>> ---
>> Changes in v2:
>>  - Removed calls to dwc3_ep0_end_control_data() and just allow the ep disables
>>    on EP0 handle the proper ending of transfers.
>>  - Ensure that delayed_status is cleared, as ran into enumeration issues if the
>>    SETUP transaction fails on a STATUS stage.  Saw delayed_status == TRUE on the
>>    next connect, which blocked further SETUP transactions to be handled.
>>
>>  drivers/usb/dwc3/gadget.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5d084542718d..8b6a95c35741 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2430,7 +2430,6 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>  				msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
>>  		if (ret == 0) {
>>  			dev_err(dwc->dev, "timed out waiting for SETUP phase\n");
>> -			return -ETIMEDOUT;
>>  		}
> 
> Since the `if' now has a single statement, you should remove the curly braces.
> 
Thanks for the reviews!  Will fix them up and resend.

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project