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: <6f5f0e4c-d18f-b8cb-30cc-53bbfa158c2f@quicinc.com>
Date:   Mon, 19 Jun 2023 18:57:47 +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
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
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>
 <fca531e0-88ec-ba19-2c11-e8965ac653b2@quicinc.com>
Content-Language: en-US
In-Reply-To: <fca531e0-88ec-ba19-2c11-e8965ac653b2@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 6/19/2023 6:20 PM, Krishna Kurapati PSSNV wrote:
> 
> 
> 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. >
Typo here:

So when we plug out the cable, the disconnect event won't be generated 
and on systems like android the user space will never clear the UDC upon 
disconnect.

> 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