Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <ceb0f48f-8db9-40ae-769a-08e36373b922@quicinc.com>
Date:   Thu, 26 Oct 2023 17:07:49 -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 v4 3/3] usb: dwc3: Modify runtime pm ops to handle bus
 suspend
Content-Language: en-US
To:     Roger Quadros <rogerq@kernel.org>, <gregkh@linuxfoundation.org>,
        <Thinh.Nguyen@synopsys.com>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <devicetree@vger.kernel.org>
CC:     <linux-kernel@vger.kernel.org>, <linux-usb@vger.kernel.org>
References: <20230814185043.9252-1-quic_eserrao@quicinc.com>
 <20230814185043.9252-4-quic_eserrao@quicinc.com>
 <9be9fae5-f6f2-42fe-bd81-78ab50aafa06@kernel.org>
 <cd294a89-33e7-0569-81b3-df77a255f061@quicinc.com>
 <0dee3bec-d49f-4808-a2f8-7a4205303e1f@kernel.org>
 <c7fc7bc2-1a84-e6b5-5198-1b8cc602d738@quicinc.com>
 <bd74947f-8827-4539-a590-9c53d5ddd02d@kernel.org>
From:   Elson Serrao <quic_eserrao@quicinc.com>
In-Reply-To: <bd74947f-8827-4539-a590-9c53d5ddd02d@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


>>>>>
>>>>> While this takes care of runtime suspend case, what about system_suspend?
>>>>> Should this check be moved to dwc3_suspend_common() instead?
>>>>>
>>>>
>>>> Sure I can move these checks to dwc3_suspend_common to make it generic.
>>>
>>> Before you do that let's first decide how we want the gadget driver to behave
>>> in system_suspend case.
>>>
>>> Current behavior is to Disconnect from the Host.
>>>
>>> Earlier I was thinking on the lines that we prevent system suspend if
>>> we are not already in USB suspend. But I'm not sure if that is the right
>>> thing to do anymore. Mainly because, system suspend is a result of user
>>> request and it may not be nice to not to meet his/her request.
>>
>> Agree. Irrespective of whether USB is suspended or not it is better to honor the system suspend request from user.
>>
>>> Maybe best to leave this policy handling to user space?
>>> i.e. if user wants USB gadget operation to be alive, he will not issue
>>> system suspend?
>>>
>>
>> Sure. So below two cases
>>
>> Case1: User doesn't care if gadget operation is alive and triggers system suspend irrespective of USB suspend. Like you mentioned, current behavior already takes care of this and initiates a DISCONNECT
>>
>> Case2:  User wants gadget to stay alive and hence can trigger system suspend only when USB is suspended (there are already user space hooks that read cdev->suspended bit to tell whether USB is suspended or not for user to decide). Attempts to request system suspend when USB is not suspended, would result in a DISCONNECT.
>>
>> For supporting Case2 from gadget driver point of view, we need to extend this series by having relevant checks in suspend_common()
>>
>> Also, is it better to provide separate flags to control the gadget driver behavior for runtime suspend Vs system suspend when USB is suspended ? For example, what if we want to enable bus suspend handling for runtime suspend only and not for system suspend (Case1).
> 
> But you mentioned that for Case1, USB gadget would disconnect from Host. So USB will be in disconnected state and USB controller can be fully de-activated? Except maybe wakeup handling to bring system out of suspend on a USB plug/unplug event?
> Why do we need separate flags for?
> 

Sorry let me clarify. This is in reference to deciding how we want the 
dwc3 driver to behave in system_suspend case.

One option is to continue with the existing behavior where USB gadget 
would disconnect from Host irrespective of bus suspend state. We dont 
need any modification in this case and we can leave this series limited 
to runtime suspend only.

Second option is to stay connected IF we are in bus suspend state 
(U3/L2) otherwise DISCONNECT IF we are not in bus suspend state. The 
main motivation is to preserve the ongoing usb session
without going through a re-enumeration (ofcourse true only if we are in 
bus suspend state). This would need relevant checks in suspend_common().

Which option do you think is more suitable? IMO option2 is better. For 
example if we are in a scenario where there is a network session (over 
USB) open between Host and the device and usb bus is suspended due to 
data inactivity. Option2 would preserve the session whereas Option1 we 
would terminate this session when a system_suspend happens.

Thanks
Elson