Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: wcheng=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org BA675C43460
Subject: Re: [PATCH v2] usb: dwc3: gadget: Stop EP0 transfers during pullup
 disable
To:     Felipe Balbi <balbi@kernel.org>
Cc:     gregkh@linuxfoundation.org, Thinh.Nguyen@synopsys.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        jackp@codeaurora.org
References: <20210823092324.1949-1-wcheng@codeaurora.org>
 <87eeakld0d.fsf@kernel.org>
From:   Wesley Cheng <wcheng@codeaurora.org>
Message-ID: <9e11d0dc-c043-6b55-2c33-fb1a55b18156@codeaurora.org>
Date:   Mon, 23 Aug 2021 03:59:29 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <87eeakld0d.fsf@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Felipe,

On 8/23/2021 2:34 AM, Felipe Balbi wrote:
> 
> Wesley Cheng <wcheng@codeaurora.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@codeaurora.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