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: <fca531e0-88ec-ba19-2c11-e8965ac653b2@quicinc.com>
Date:   Mon, 19 Jun 2023 18:20:43 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.1
Subject: Re: [PATCH v3] usb: dwc3: gadget: Propagate core init errors to UDC
 during pullup
To:     Johan Hovold <johan@kernel.org>,
        Thinh Nguyen <Thinh.Nguyen@synopsys.com>
CC:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        <linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <quic_ppratap@quicinc.com>, <quic_wcheng@quicinc.com>,
        <quic_jackp@quicinc.com>, <quic_ugoswami@quicinc.com>
References: <20230618120949.14868-1-quic_kriskura@quicinc.com>
 <ZI_-c5g20DSJOSu2@hovoldconsulting.com>
Content-Language: en-US
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <ZI_-c5g20DSJOSu2@hovoldconsulting.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/19/2023 12:36 PM, Johan Hovold wrote:
> On Sun, Jun 18, 2023 at 05:39:49PM +0530, Krishna Kurapati wrote:
>> In scenarios where pullup relies on resume (get sync) to initialize
>> the controller and set the run stop bit, then core_init is followed by
>> gadget_resume which will eventually set run stop bit.
>>
>> But in cases where the core_init fails, the return value is not sent
>> back to udc appropriately. So according to UDC the controller has
>> started but in reality we never set the run stop bit.
>>
>> On systems like Android, there are uevents sent to HAL depending on
>> whether the configfs_bind / configfs_disconnect were invoked. In the
>> above mentioned scnenario, if the core init fails, the run stop won't
>> be set and the cable plug-out won't result in generation of any
>> disconnect event and userspace would never get any uevent regarding
>> cable plug out and we never call pullup(0) again. Furthermore none of
>> the next Plug-In/Plug-Out's would be known to configfs.
>>
>> Return back the appropriate result to UDC to let the userspace/
>> configfs know that the pullup failed so they can take appropriate
>> action.
>>
>> Fixes: 77adb8bdf422 ("usb: dwc3: gadget: Allow runtime suspend if UDC unbinded")
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>> Changes in v3: Added changelog mising in v2
>> Changes in v2: Added Fixes tag
>>
>>   drivers/usb/dwc3/gadget.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 578804dc29ca..27cb671e18e3 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2747,7 +2747,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>   	ret = pm_runtime_get_sync(dwc->dev);
>>   	if (!ret || ret < 0) {
>>   		pm_runtime_put(dwc->dev);
>> -		return 0;
>> +		if (ret < 0)
>> +			pm_runtime_set_suspended(dwc->dev);
> 
> This bit is broken and is also not mentioned or explained in the commit
> message. What are you trying to achieve here?
> 
> You cannot set the state like this after runtime PM is enabled and the
> above call will always fail.
> 
Hi Johan,

   Apologies, I didn't understand the comment fully.

The reason why I an returning ret is because, when the first get_sync 
fails because of core_init failure and we return 0 instead of ret, the 
UDC thinks that controller has started successfully but we never set the 
run stop bit. So when we plug out the cable,  the disconnect event won't 
be generated and we never send on systems like android the user space 
will never clear the UDC upon disconnect. Its a sort of mismatch between 
controller and udc.

Also once the first get_sync fails, the dwc->dev->power.runtime_error 
flag is set and successive calls to get_sync always return -EINVAL. In 
this situation even if UDC/configfs retry pullup, resume_common will 
never be called and we never actually start the controller or resume 
dwc->dev.

By calling set_suspended, I am trying to clear the runtime_error flag so 
that the next retry to pullup will call resume_common and retry 
core_init and set run_stop.

Regards,
Krishna,

>> +		return ret;
>>   	}
>>   
>>   	if (dwc->pullups_connected == is_on) {
> 
> Johan