| 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: Thu, 18 Aug 2016 20:52:11 +0800
From: Baolin Wang <baolin.wang@...aro.org>
To: Felipe Balbi <balbi@...nel.org>
Cc: Greg KH <gregkh@...uxfoundation.org>, mathias.nyman@...el.com,
USB <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume
Hi Felipe,
On 18 August 2016 at 20:25, Felipe Balbi <balbi@...nel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@...aro.org> writes:
>> Hi Felipe,
>>
>> On 18 August 2016 at 15:33, Felipe Balbi <balbi@...nel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@...aro.org> writes:
>>>> 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, and
>>>> we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
>>>> suspending the xhci.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>
>>>> ---
>>>> drivers/usb/dwc3/Kconfig | 7 +++++++
>>>> drivers/usb/dwc3/core.c | 25 ++++++++++++++++++++++++-
>>>> drivers/usb/dwc3/core.h | 15 +++++++++++++++
>>>> drivers/usb/dwc3/host.c | 32 ++++++++++++++++++++++++++++++++
>>>> 4 files changed, 78 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>> index a64ce1c..725d2bd 100644
>>>> --- a/drivers/usb/dwc3/Kconfig
>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>> @@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
>>>> This is the default mode of working of DWC3 controller where
>>>> both host and gadget features are enabled.
>>>>
>>>> +config USB_DWC3_HOST_SUSPEND
>>>> + bool "Choose if the host (xhci) can be suspend/resume"
>>>> + depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>>> + help
>>>> + We can suspend the host when the slave is detached for power saving,
>>>> + and resume the host when one slave is attached.
>>>> +
>>>> endchoice
>>>>
>>>> comment "Platform Glue Driver Support"
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 1485480..5140b4d 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
>>>> static int dwc3_suspend_common(struct dwc3 *dwc)
>>>> {
>>>> unsigned long flags;
>>>> + int ret;
>>>>
>>>> switch (dwc->dr_mode) {
>>>> case USB_DR_MODE_PERIPHERAL:
>>>> + spin_lock_irqsave(&dwc->lock, flags);
>>>> + dwc3_gadget_suspend(dwc);
>>>> + spin_unlock_irqrestore(&dwc->lock, flags);
>>>> + break;
>>>> case USB_DR_MODE_OTG:
>>>> + ret = dwc3_host_suspend(dwc);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> spin_lock_irqsave(&dwc->lock, flags);
>>>> dwc3_gadget_suspend(dwc);
>>>> spin_unlock_irqrestore(&dwc->lock, flags);
>>>> break;
>>>> case USB_DR_MODE_HOST:
>>>> + ret = dwc3_host_suspend(dwc);
>>>> + if (ret)
>>>> + return ret;
>>>> default:
>>>> /* do nothing */
>>>> break;
>>>> @@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>>>>
>>>> switch (dwc->dr_mode) {
>>>> case USB_DR_MODE_PERIPHERAL:
>>>> + spin_lock_irqsave(&dwc->lock, flags);
>>>> + dwc3_gadget_resume(dwc);
>>>> + spin_unlock_irqrestore(&dwc->lock, flags);
>>>> + break;
>>>> case USB_DR_MODE_OTG:
>>>> + ret = dwc3_host_resume(dwc);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> spin_lock_irqsave(&dwc->lock, flags);
>>>> dwc3_gadget_resume(dwc);
>>>> spin_unlock_irqrestore(&dwc->lock, flags);
>>>> - /* FALLTHROUGH */
>>>> + break;
>>>> case USB_DR_MODE_HOST:
>>>> + ret = dwc3_host_resume(dwc);
>>>> + if (ret)
>>>> + return ret;
>>>> default:
>>>> /* do nothing */
>>>> break;
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 45d6de5..0ba203e 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>>> { }
>>>> #endif
>>>>
>>>> +#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
>>>> +int dwc3_host_suspend(struct dwc3 *dwc);
>>>> +int dwc3_host_resume(struct dwc3 *dwc);
>>>> +#else
>>>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static inline int dwc3_host_resume(struct dwc3 *dwc)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>> #endif /* __DRIVERS_USB_DWC3_CORE_H */
>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>>> index 2e960ed..2ec3eff 100644
>>>> --- a/drivers/usb/dwc3/host.c
>>>> +++ b/drivers/usb/dwc3/host.c
>>>> @@ -17,8 +17,11 @@
>>>>
>>>> #include <linux/platform_device.h>
>>>> #include <linux/usb/xhci_pdriver.h>
>>>> +#include <linux/usb.h>
>>>> +#include <linux/usb/hcd.h>
>>>>
>>>> #include "core.h"
>>>> +#include "../host/xhci.h"
>>>>
>>>> int dwc3_host_init(struct dwc3 *dwc)
>>>> {
>>>> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>> memset(&pdata, 0, sizeof(pdata));
>>>>
>>>> pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
>>>> + /* dwc3 controller need an extraordinary delay when suspending xhci. */
>>>> + pdata.usb3_slow_suspend = 1;
>>>>
>>>> ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>>>> if (ret) {
>>>> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>>> dev_name(&dwc->xhci->dev));
>>>> platform_device_unregister(dwc->xhci);
>>>> }
>>>> +
>>>> +int dwc3_host_suspend(struct dwc3 *dwc)
>>>> +{
>>>> + struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>> + int ret, cnt = 20;
>>>> +
>>>> +try_again:
>>>> + /* We should wait for xhci bus has been into suspend mode firstly. */
>>>> + ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
>>>> + if (ret && --cnt > 0) {
>>>> + dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
>>>> + ret);
>>>> + msleep(200);
>>>> + goto try_again;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +int dwc3_host_resume(struct dwc3 *dwc)
>>>> +{
>>>> + struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>> +
>>>> + return xhci_resume(xhci, 0);
>>>> +}
>>>
>>> This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
>>> xhci_supend() has already been called. Why isn't it called for you?
>>
>> xhci-plat.c did not set the runtime PM callbacks, how can we issued
>> xhci_supend() by runtime PM from dwc3?
>
> wouldn't it be nicer to just initialize PM runtime callbacks from
> xhci-plat? It just needs to be added and verified, right?
Yes, you are right. Alan and Peter also suggested me to implement the
runtime PM callbacks for xhci-plat. I am also testing this and I will
send out the patch if it works well. So please ignore this patch.
Thanks for your comments.
--
Baolin.wang
Best Regards
Powered by blists - more mailing lists