linux-kernel - Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 08 Dec 2016 19:52:56 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Baolin Wang <baolin.wang@...aro.org>
Cc:     mathias.nyman@...el.com, Greg KH <gregkh@...uxfoundation.org>,
        Mark Brown <broonie@...nel.org>,
        USB <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume


Hi,

Baolin Wang <baolin.wang@...aro.org> writes:
>> Baolin Wang <baolin.wang@...aro.org> writes:
>>>>> On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@...aro.org> wrote:
>>>>>> For some mobile devices with strict power management, we also want to suspend
>>>>>> the host when the slave is detached for power saving. Thus we add the host
>>>>>> suspend/resume functions to support this requirement.
>>>>>>
>>>>>> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>
>>>>>> ---
>>>>>> Changes since v3:
>>>>>>  - No updates.
>>>>>>
>>>>>> Changes since v2:
>>>>>>  - Remove pm_children_suspended() and other unused macros.
>>>>>>
>>>>>>  Changes since v1:
>>>>>>    - Add pm_runtime.h head file to avoid kbuild error.
>>>>>> ---
>>>>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>>>>  drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
>>>>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>>>>  drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
>>>>>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>>>> index a45b4f1..47bb2f3 100644
>>>>>> --- a/drivers/usb/dwc3/Kconfig
>>>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>>>> @@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
>>>>>>
>>>>>>  endchoice
>>>>>>
>>>>>> +config USB_DWC3_HOST_SUSPEND
>>>>>> +       bool "Choose if the DWC3 host (xhci) can be suspend/resume"
>>>>>> +       depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>>>
>>>> why do you need another Kconfig for this? Just enable it already :-p
>>>
>>> I assume some platforms may do not need this feature. But okay, I can
>>> remove this kconfig and enable it.
>>
>> thanks :-)
>>
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 9a4a5e4..7ad4bc3 100644
>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>>>         pm_runtime_use_autosuspend(dev);
>>>>>>         pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>>>>>>         pm_runtime_enable(dev);
>>>>>> +       pm_suspend_ignore_children(dev, true);
>>>>
>>>> why do you need this?
>>>
>>> Since the dwc3 device is the parent deive of xhci device, if we want
>>> to suspend dwc3 device, we need to suspend child device (xhci device)
>>> manually by issuing pm_runtime_put_sync() in dwc3_host_suspend(). In
>>> pm_runtime_put_sync(), it will check if the children (xhci device) of
>>> dwc3 device have been in suspend state, if not we will suspend dwc3
>>> device failed.
>>>
>>> We get/put the child device manually in parent device's runtime
>>> callback, we need to ignore the child device's runtime state, or it
>>> will failed due to the dependency.
>>
>> I see. Good explanation :-)
>>
>> There's one thing though, if you want to runtime suspend the gadget and
>> dwc3 is working on peripheral mode, host side (XHCI) should already be
>> runtime suspended because there's nothing attached to it. Why isn't it
>> runtime suspended?
>
> Since we have get the runtime count for xHCI device in
> xhci_plat_probe(), in case it will suspend automatically if some
> controllers do not want xHCI enters runtime suspend automatically. So
> the parent device (dwc3 device) need to get/put the xHCI device's
> runtime count  to resume/suspend xHCI.

IMHO, that's a bug in xhci-plat, not something dwc3 should work around.

-- 
balbi