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.3.2 smtp.codeaurora.org 10EA4C433C8
From:   Wesley Cheng <wcheng@codeaurora.org>
Subject: Re: [PATCH 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC
 unbinded
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     balbi@kernel.org, gregkh@linuxfoundation.org,
        Thinh.Nguyen@synopsys.com, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, jackp@codeaurora.org
References: <20201028234311.6464-1-wcheng@codeaurora.org>
 <20201028234311.6464-2-wcheng@codeaurora.org>
 <20201029010748.GA1303156@rowland.harvard.edu>
Message-ID: <a47d78f3-0a11-9de8-dbea-ad745d496845@codeaurora.org>
Date:   Tue, 3 Nov 2020 11:02:25 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: <20201029010748.GA1303156@rowland.harvard.edu>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/28/2020 6:07 PM, Alan Stern wrote:
> On Wed, Oct 28, 2020 at 04:43:10PM -0700, Wesley Cheng wrote:
>> The DWC3 runtime suspend routine checks for the USB connected parameter to
>> determine if the controller can enter into a low power state.  The
>> connected state is only set to false after receiving a disconnect event.
>> However, in the case of a device initiated disconnect (i.e. UDC unbind),
>> the controller is halted and a disconnect event is never generated.  Set
>> the connected flag to false if issuing a device initiated disconnect to
>> allow the controller to be suspended.
>>
>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>> ---
>>  drivers/usb/dwc3/gadget.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5d879b7606d5..6364429b2122 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1995,6 +1995,11 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>  	unsigned long		flags;
>>  	int			ret;
>>  
>> +	if (pm_runtime_suspended(dwc->dev)) {
>> +		pm_request_resume(dwc->dev);
>> +		return 0;
>> +	}
> 
> Isn't this racy?  What happens if the controller was active but a 
> runtime suspend occurs right here?
> 
> Alan Stern
> 

Hi Alan,

Ah, yes you're right.  I was hoping that the PM runtime layer would be
utilizing the spinlock when reading out the runtime status, but even
then, we wouldn't be able to catch intermediate states with this API
(i.e. RPM_RESUMING or RPM_SUSPENDING)

Tried a few different approaches, and came up with something like the
following:

static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
{
...
	ret = pm_runtime_get_sync(dwc->dev);
	if (!ret) {
		pm_runtime_put(dwc->dev);
		return 0;
	}
	...
	pm_runtime_put(dwc->dev);

	return 0;
}

I think this should be good to address your concern.  The only way we
would be able to ensure that the runtime PM state doesn't enter
idle/suspend is if we increment the usage count for the duration we're
accessing the DWC3 registers.  With the synchronous PM runtime resume
call, we can also ensure that no pending runtime suspends are executing
in parallel while running this code.

The check for the zero return value would be for avoiding running the
DWC3 run stop sequence for the case where we executed the runtime PM
resume, as the DWC3 runtime PM resume routine will set the run stop bit
in there.

Thanks

Regards,
Wesley Cheng
>> +
>>  	is_on = !!is_on;
>>  
>>  	/*
>> @@ -2050,6 +2055,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>  			dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
>>  						dwc->ev_buf->length;
>>  		}
>> +		dwc->connected = false;
>>  	}
>>  
>>  	ret = dwc3_gadget_run_stop(dwc, is_on, false);
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project

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