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] [day] [month] [year] [list]
Message-ID: <CAMz4kuJMmRuS2DD8vpM3FuBs9X3s=WsPeR+dr3U1MQyvBAwcxw@mail.gmail.com>
Date:   Fri, 9 Dec 2016 11:23:44 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Felipe Balbi <balbi@...nel.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,

On 9 December 2016 at 01:52, Felipe Balbi <balbi@...nel.org> wrote:
>
> 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.

Maybe you misunderstood my meaning and I should make it clear.

If dwc3 is just working on peripheral mode, then no need to initialize
the host (xhci) things, which means it is invalid to runtime
suspend/resume xHCI device and we can just runtime suspend/resume the
gadget.

If dwc3 is working on OTG mode, which will create and add the USB hcd
for host side. Then if we want to suspend dwc3, we need to suspend
xHCI device manually though now dwc3 acts as peripheral. The USB
device (bus) of host side will runtime suspend automatically if there
are no slave attached (by
usb_runtime_suspend()--->usb_suspend_both()--->usb_suspend_interface()--->usb_suspend_device()--->generic_suspend()--->hcd_bus_suspend()--->xhci_bus_suspend()),
but we should not suspend xHCI device (issuing xhci_suspend()) now,
since some controllers did not implement the runtime PM to control
xHCI device's runtime state, which will cause controllers can not
resume xHCI device (issuing xhci_resume()) if the xHCI device suspend
automatically by its child device. Thus we should get the runtime
count for xHCI device in xhci_plat_probe() in case xHCI device will
also suspend automatically by its child device. According to that, for
xHCI device's parent: dwc3 device, we should put the xHCI device's
runtime count to suspend xHCI device manually. Hope I make it clear.

-- 
Baolin.wang
Best Regards