| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 25 Oct 2023 15:21:15 -0700
From: Elson Serrao <quic_eserrao@...cinc.com>
To: Roger Quadros <rogerq@...nel.org>, <gregkh@...uxfoundation.org>,
<Thinh.Nguyen@...opsys.com>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<devicetree@...r.kernel.org>
CC: <linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>
Subject: Re: [PATCH v4 3/3] usb: dwc3: Modify runtime pm ops to handle bus
suspend
On 10/25/2023 1:02 AM, Roger Quadros wrote:
>
>
> On 24/10/2023 21:41, Elson Serrao wrote:
>>
>>
>> On 10/24/2023 3:14 AM, Roger Quadros wrote:
>>> Hi Elson,
>>>
>>> On 14/08/2023 21:50, Elson Roy Serrao wrote:
>>>> The current implementation blocks the runtime pm operations when cable
>>>> is connected. This would block dwc3 to enter a low power state during
>>>> bus suspend scenario. Modify the runtime pm ops to handle bus suspend
>>>> case for such platforms where the controller low power mode entry/exit
>>>> is handled by the glue driver. This enablement is controlled through a
>>>> dt property and platforms capable of detecting bus resume can benefit
>>>> from this feature. Also modify the remote wakeup operations to trigger
>>>> runtime resume before sending wakeup signal.
>>>>
>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@...cinc.com>
>>>> ---
>>>> drivers/usb/dwc3/core.c | 28 ++++++++++++++++++++++++++--
>>>> drivers/usb/dwc3/core.h | 3 +++
>>>> drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++-------
>>>> 3 files changed, 54 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 9c6bf054f15d..9bfd9bb18caf 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>> dwc->dis_split_quirk = device_property_read_bool(dev,
>>>> "snps,dis-split-quirk");
>>>> + dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev,
>>>> + "snps,runtime-suspend-on-usb-suspend");
>>>> +
>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>>> dwc->tx_de_emphasis = tx_de_emphasis;
>>>> @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>>> switch (dwc->current_dr_role) {
>>>> case DWC3_GCTL_PRTCAP_DEVICE:
>>>> + /* runtime resume on bus resume scenario */
>>>> + if (PMSG_IS_AUTO(msg) && dwc->connected)
>>>> + break;
>>>> ret = dwc3_core_init_for_resume(dwc);
>>>> if (ret)
>>>> return ret;
>>>> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
>>>> {
>>>> switch (dwc->current_dr_role) {
>>>> case DWC3_GCTL_PRTCAP_DEVICE:
>>>> - if (dwc->connected)
>>>> + if (dwc->connected) {
>>>> + /* bus suspend scenario */
>>>> + if (dwc->runtime_suspend_on_usb_suspend &&
>>>> + dwc->suspended)
>>>
>>> If dwc is already suspended why do we return -EBUSY?
>>> Should this be !dwc->suspended?
>>>
>>
>> Hi Roger
>>
>> Thank you for reviewing.
>> If dwc->suspended is true (i.e suspend event due to U3/L2 is received), I am actually breaking from this switch statement and returning 0.
>
> Of course. I missed the break :)
>
>>
>>>> + break;
>>>> return -EBUSY;
>>>> + }
>>>> break;
>>>> case DWC3_GCTL_PRTCAP_HOST:
>>>> default:
>>>> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev)
>>>> struct dwc3 *dwc = dev_get_drvdata(dev);
>>>> int ret;
>>>> - if (dwc3_runtime_checks(dwc))
>>>> + ret = dwc3_runtime_checks(dwc);
>>>> + if (ret)
>>>> return -EBUSY;
>>>> + switch (dwc->current_dr_role) {
>>>> + case DWC3_GCTL_PRTCAP_DEVICE:
>>>> + /* bus suspend case */
>>>> + if (!ret && dwc->connected)
>>>
>>> No need to check !ret again as it will never happen because
>>> we are returning -EBUSY earlier if (ret);
>>>
>> Thanks for this catch. I will remove !ret check in v5.
>>
>>>> + return 0;
>>>> + break;
>>>> + case DWC3_GCTL_PRTCAP_HOST:
>>>> + default:
>>>> + /* do nothing */
>>>> + break;
>>>> + }
>>>> +
>>>
>>> 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).
Thanks
Elson
>
>> Will rename this patch to "Modify pm ops to handle bus suspend" since this is now not limited to only runtime suspend/resume. Will also rename dwc->runtime_suspend_on_usb_suspend to dwc->delegate_wakeup_interrupt based on earlier feedback.
>>
>> I am still working on a clean way to enable/disable this feature (i.e set dwc->delegate_wakeup_interrupt flag) from the glue driver based on Thinh's feedback .
>> I will accommodate above feedback as well and upload v5.
>>
>> Thanks
>> Elson
>
Powered by blists - more mailing lists