linux-kernel - Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM

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]

Message-ID: <20160826221134.GA10849@localhost>
Date:   Fri, 26 Aug 2016 15:11:36 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Robert Foss <robert.foss@...labora.com>
Cc:     mathias.nyman@...el.com, gregkh@...uxfoundation.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Julius Werner <jwerner@...omium.org>,
        Andrew Bresticker <abrestic@...omium.org>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Baolin Wang <baolin.wang@...aro.org>, zyx@...k-chips.com,
        wulf@...k-chips.com
Subject: Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM

Hi,

On Wed, Aug 24, 2016 at 04:48:01PM -0400, Robert Foss wrote:
> On 2016-08-22 11:23 PM, Brian Norris wrote:
> >+ others
> >
> >Hi Robert and Felipe,
> >
> >I have a few questions for one or both of you. I'm not really an expert
> >on runtime PM, so please take my questions with a grain of salt.
> >
> >On Wed, Aug 10, 2016 at 04:32:15PM -0400, robert.foss@...labora.com wrote:
> >>From: Robert Foss <robert.foss@...labora.com>
> >>
> >>Enable runtime PM for the xhci-plat device so that the parent device
> >>may implement runtime PM.
> >>
> >>Signed-off-by: Robert Foss <robert.foss@...labora.com>
> >>
> >>Tested-by: Robert Foss <robert.foss@...labora.com>
> >>---
> >> drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++--
> >> 1 file changed, 27 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> >>index ed56bf9..ba4efe7 100644
> >>--- a/drivers/usb/host/xhci-plat.c
> >>+++ b/drivers/usb/host/xhci-plat.c
> >>@@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >> 	if (ret)
> >> 		goto dealloc_usb2_hcd;
> >>
> >>+	pm_runtime_set_active(&pdev->dev);
> >>+	pm_runtime_enable(&pdev->dev);
> >>+
> >
> >How does it help to enable PM runtime like this, if you don't have any
> >kind of runtime_{suspend,resume}() callbacks?
> 
> Andrew, I think you understand the inner workings of this code
> better than me, maybe you could give a short summary?

I believe Andrew is fairly busy, and I already talked with him a bit
about this. This is needed (as per your (or his?) commit message) only
if you have a parent device that wants to implement runtime PM. Now
depending on what you want to do with "runtime PM", this might mean
the parent device has to suspend xhci_{suspend,resume}() on behalf of
xhci-plat.c. Not very nice layering IMO, but it has been done before...

So I guess this comes down to "what does a parent device/driver want to
do"? If that's (e.g.) just putting some PHY into a slightly lower power
mode, then maybe it's fine for xhci-plat not to do anything else. But if
you actually want to completely power down the parent bus, reset an
accompanying dual-role/OTG controller, etc., then this really isn't
sufficient, AFAICT. But maybe that's just an indictment of the poor
structure of dwc3's host-mode support, more than it is an indictment of
this patch.

Brian