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: <be57511d-2005-a1f5-d5a5-809e71029aec@quicinc.com>
Date:   Fri, 14 Jul 2023 13:34:36 -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 v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus
 suspend
Content-Language: en-US
To:     Roger Quadros <rogerq@kernel.org>, <Thinh.Nguyen@synopsys.com>,
        <stern@rowland.harvard.edu>
CC:     <gregkh@linuxfoundation.org>, <linux-kernel@vger.kernel.org>,
        <linux-usb@vger.kernel.org>, <quic_wcheng@quicinc.com>,
        <quic_jackp@quicinc.com>
References: <20230711174320.24058-1-quic_eserrao@quicinc.com>
 <20230711174320.24058-4-quic_eserrao@quicinc.com>
 <9fc752db-ea8a-4bf0-a99a-832403668384@kernel.org>
 <3c9c6924-bf79-e8bb-fd44-9262f114504b@quicinc.com>
 <9715b60c-2caa-82d6-42ea-31502272ebc4@kernel.org>
 <aa311ec9-7abd-0684-16cd-e7a2cfa7ef3c@quicinc.com>
 <8b10d8a5-a9df-6a2e-61b9-7119961e2401@kernel.org>
From:   Elson Serrao <quic_eserrao@quicinc.com>
In-Reply-To: <8b10d8a5-a9df-6a2e-61b9-7119961e2401@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 7/14/2023 5:23 AM, Roger Quadros wrote:
> 
> 

>>>>>>     }
>>>>>>       static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>>>>>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>>>>>     {
>>>>>>         if (dwc->pending_events) {
>>>>>>             dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>>>>> +        pm_runtime_put(dwc->dev);
>>>>>
>>>>> Why the put here?
>>>>>
>>>>
>>>> To balance the get() called when setting the pending_events flag in dwc3_check_event_buf()
>>>>
>>>>       if (pm_runtime_suspended(dwc->dev)) {
>>>>           pm_runtime_get(dwc->dev);
>>>>           disable_irq_nosync(dwc->irq_gadget);
>>>>           dwc->pending_events = true;
>>>>           return IRQ_HANDLED;
>>>>       }
>>>>
>>>
>>> No this wrong. We want the device to be active from now on.
>>>
>>> runtime suspended->interrupt->pm_runtime_get->runtime_resume->process_pending_events->USB gadget resumed
>>>
>>> Only on next USB suspend you want to do the pm_runtime_put like you are doing it
>>> in dwc3_gadget_suspend_interrupt() by pm_request_autosuspend()
>>>
>>
>> That would break/block dwc3 runtime suspend during DISCONNECT case in below scenario
>>
>> runtime suspended->interrupt->pm_runtime_get (runtime usage count is 1)->runtime_resume->process_pending_events->USB gadget resumed -> USB disconnect (autosuspend blocked due to runtime usage count being 1 due to unbalanced get() ).
>>
>> The idea here is to balance the get() that was requested for processing the pending events, after processing those events. (like how we balance get() of ep_queue through put() in ep_dequeue)
>>
>> Also pm_request_autosuspend() doesnt decrement the usage count, it only requests for autosuspend.
> 
> Ah, indeed.
> 
>>
>> But I think better approach in terms of ordering is below
> 
> ok, but should dwc->pending_events be set before calling pm_runtime_get() in dwc3_check_event_buf()?

Yes that would be a better approach (just in case if there is any race 
between dwc3_check_event_buf() and the resume() path).

> Can we add a comment there that the get will be balanced out in dwc3_gadget_process_pending_events()?

Sure.
> 
>>
>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>   {
>>       if (dwc->pending_events) {
>>           dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>> +        /*
>> +         * We have only stored the pending events as part
>> +         * of dwc3_interrupt() above, but those events are
>> +         * not yet handled. So explicitly invoke the
>> +         * interrupt handler for handling those events.
>> +         */
>> +        dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>           dwc->pending_events = false;
>>           enable_irq(dwc->irq_gadget);
>> +        pm_runtime_put(dwc->dev);
> 
> We could do the put right after dwc3_thread_interrupt().

Ack.

> 
>>
>>       }
>>   }
> 
> I think this fix should be an independent patch
> as this fixes an issue that existed prior to this series?
> also need to -cc stable?
> 
Agree. Both dwc3_thread_interrupt() and pm_runtime_put() above are 
addressing an issue that existed prior. I will submit a separate patch 
for this modification.

Thanks
Elson