Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c8181845-0f6a-9c6b-69bc-4ce9dabdf041@quicinc.com>
Date:   Tue, 4 Apr 2023 11:49:19 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH] usb: dwc3: gadget: Stall and restart EP0 if host is
 unresponsive
Content-Language: en-US
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "quic_jackp@quicinc.com" <quic_jackp@quicinc.com>,
        "quic_ugoswami@quicinc.com" <quic_ugoswami@quicinc.com>
References: <20230331232039.1407-1-quic_wcheng@quicinc.com>
 <20230404011108.727htmnllj7ojwqm@synopsys.com>
From:   Wesley Cheng <quic_wcheng@quicinc.com>
In-Reply-To: <20230404011108.727htmnllj7ojwqm@synopsys.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Thinh,

On 4/3/2023 6:11 PM, Thinh Nguyen wrote:
> On Fri, Mar 31, 2023, Wesley Cheng wrote:
>> It was observed that there are hosts that may complete pending SETUP
>> transactions before the stop active transfers and controller halt occurs,
>> leading to lingering endxfer commands on DEPs on subsequent pullup/gadget
>> start iterations.
> 
> Can you clarify this a bit further? Even though the controller is
> halted, you still observed activity?
> 

Yes...I didn't understand how that was possible either, but traces 
clearly showed that the controller halt was successful even though there 
were no endxfers issued on some EPs.  Although, I can't say for certain 
if those EPs were actively being used at that time.

>>
>> dwc3_gadget_ep_disable   name=ep8in flags=0x3009  direction=1
>> dwc3_gadget_ep_disable   name=ep4in flags=1  direction=1
>> dwc3_gadget_ep_disable   name=ep3out flags=1  direction=0
>> usb_gadget_disconnect   deactivated=0  connected=0  ret=0
>>
>> The sequence shows that the USB gadget disconnect (dwc3_gadget_pullup(0))
>> routine completed successfully, allowing for the USB gadget to proceed with
>> a USB gadget connect.  However, if this occurs the system runs into an
>> issue where:
>>
>> BUG: spinlock already unlocked on CPU
>>   spin_bug+0x0
>>   dwc3_remove_requests+0x278
>>   dwc3_ep0_out_start+0xb0
>>   __dwc3_gadget_start+0x25c
>>
>> This is due to the pending endxfers, leading to gadget start (w/o lock
>> held) to execute the remove requests, which will unlock the dwc3 spinlock
>> as part of giveback.
>>
>> To mitigate this, resolve the pending endxfers on the pullup disable path
>> by:
>>   1. Re-locating the SETUP phase check after stop active transfers, since
>>   that is where the DWC3_EP_DELAY_STOP is potentially set.  This also allows
>>   for handling of a host that may be unresponsive by using the completion
>>   timeout to trigger the stall and restart for EP0.
>>
>>   2. Do not call gadget stop until the poll for controller halt is
>>   completed.  DEVTEN is cleared as part of gadget stop, so the intention to
>>   allow ep0 events to continue while waiting for controller halt is not
>>   happening.
>>
>> Fixes: c96683798e27 ("usb: dwc3: ep0: Don't prepare beyond Setup stage")
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   drivers/usb/dwc3/gadget.c | 101 ++++++++++++++++++++++----------------
>>   1 file changed, 58 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 3c63fa97a680..9715de8e99bc 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -139,6 +139,24 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state)
>>   	return -ETIMEDOUT;
>>   }
>>   
>> +static void dwc3_ep0_reset_state(struct dwc3 *dwc)
>> +{
>> +	unsigned int	dir;
>> +
>> +	if (dwc->ep0state != EP0_SETUP_PHASE) {
>> +		dir = !!dwc->ep0_expect_in;
>> +		if (dwc->ep0state == EP0_DATA_PHASE)
>> +			dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
>> +		else
>> +			dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);
>> +
>> +		dwc->eps[0]->trb_enqueue = 0;
>> +		dwc->eps[1]->trb_enqueue = 0;
>> +
>> +		dwc3_ep0_stall_and_restart(dwc);
>> +	}
>> +}
>> +
> 
> Can we separate refactoring changes other functional changes? It's
> difficult to review with too many things to keep track of.
> 

Sure I can split this into another patch.

Thanks
Wesley Cheng