linux-kernel - Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <CX0GOP07I40N.198G7LJ0HYDBG@tleb-bootlin-xps13-01>
Date:   Thu, 16 Nov 2023 19:56:22 +0100
From:   Théo Lebrun <theo.lebrun@...tlin.com>
To:     "Roger Quadros" <rogerq@...nel.org>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        "Rob Herring" <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        "Conor Dooley" <conor+dt@...nel.org>,
        "Peter Chen" <peter.chen@...nel.org>,
        "Pawel Laszczak" <pawell@...ence.com>,
        "Nishanth Menon" <nm@...com>,
        "Vignesh Raghavendra" <vigneshr@...com>,
        "Tero Kristo" <kristo@...nel.org>
Cc:     <linux-usb@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        Grégory Clement <gregory.clement@...tlin.com>,
        "Thomas Petazzoni" <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for
 J7200

Hello Roger,

On Thu Nov 16, 2023 at 1:40 PM CET, Roger Quadros wrote:
> On 15/11/2023 17:02, Théo Lebrun wrote:
> > On Wed Nov 15, 2023 at 12:37 PM CET, Roger Quadros wrote:
> >> On 13/11/2023 16:26, Théo Lebrun wrote:
> >>> Hardware initialisation is only done at probe. The J7200 USB controller
> >>> is reset at resume because of power-domains toggling off & on. We
> >>> therefore (1) toggle PM runtime at suspend/resume & (2) reconfigure the
> >>> hardware at resume.
> >>
> >> at probe we are doing a pm_runtime_get() and never doing a put thus
> >> preventing any runtime PM.
> > 
> > Indeed. The get() from probe/resume are in symmetry with the put() from
> > suspend. Is this wrong in some manner?
> > 
> >>> index c331bcd2faeb..50b38c4b9c87 100644
> >>> --- a/drivers/usb/cdns3/cdns3-ti.c
> >>> +++ b/drivers/usb/cdns3/cdns3-ti.c
> >>> @@ -197,6 +197,50 @@ static int cdns_ti_probe(struct platform_device *pdev)
> >>>  	return error;
> >>>  }
> >>>  
> >>> +#ifdef CONFIG_PM
> >>> +
> >>> +static int cdns_ti_suspend(struct device *dev)
> >>> +{
> >>> +	struct cdns_ti *data = dev_get_drvdata(dev);
> >>> +
> >>> +	if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb"))
> >>> +		return 0;
> >>> +
> >>> +	pm_runtime_put_sync(data->dev);
> >>> +
> >>> +	return 0;
> >>
> >> You might want to check suspend/resume ops in cdns3-plat and
> >> do something similar here.
> > 
> > I'm unsure what you are referring to specifically in cdns3-plat?
>
> What I meant is, calling pm_runtime_get/put() from system suspend/resume
> hooks doesn't seem right.
>
> How about using something like pm_runtime_forbid(dev) on devices which
> loose USB context on runtime suspend e.g. J7200.
> So at probe we can get rid of the pm_runtime_get_sync() call.

What is the goal of enabling PM runtime to then block (ie forbid) it in
its enabled state until system suspend?

Thinking some more about it and having read parts of the genpd source,
it's unclear to me why there even is some PM runtime calls in this
driver. No runtime_suspend/runtime_resume callbacks are registered.
Also, power-domains work as expected without any PM runtime calls.

The power domain is turned on when attached to a device
(see genpd_dev_pm_attach). It gets turned off automatically at
suspend_noirq (taking into account the many things that make genpd
complex: multiple devices per PD, subdomains, flags to customise the
behavior, etc.). Removing calls to PM runtime at probe keeps the driver
working.

So my new proposal would be: remove all all PM runtime calls from this
driver. Anything wrong with this approach?

Only possible reason I see for having PM runtime in this wrapper driver
would be cut the full power-domain when USB isn't used, with some PM
runtime interaction with the children node. But that cannot work
currently as we don't register a runtime_resume to init the hardware,
so this cannot be the current expected behavior.

> e.g.
>
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
>         if (cnds_ti->can_loose_context)
>                 pm_runtime_forbid(dev);
>
>         pm_runtime_set_autosuspend_delay(dev, CNDS_TI_AUTOSUSPEND_DELAY);	/* could be 20ms? */

Why mention autosuspend in this driver? This will turn the device off in
CNDS_TI_AUTOSUSPEND_DELAY then nothing enables it back using
pm_runtime_get. We have nothing to reconfigure the device, ie no
runtime_resume, so we must not go into runtime suspend.

>         pm_runtime_mark_last_busy(dev);
>         pm_runtime_use_autosuspend(dev);
>
> You will need to modify the suspend/resume handlers accordingly.
> https://docs.kernel.org/power/runtime_pm.html#runtime-pm-and-system-sleep
>
> What I'm not sure of is if there are any TI platforms that retain USB context
> on power domain off. Let me get back on this. Till then we can assume that
> all platforms loose USB context on power domain off.

Good question indeed! Thanks for looking into it. From what I see all 5
DT nodes which use this driver in upstream devicetrees have a
power-domain configured. So if the behavior is the same on all three TI
platforms (which would be the logical thing to assume) it would make
sense that all controllers lose power at suspend.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com