linux-kernel - Re: [PATCH v1 3/3] PM: sleep: Use DPM_FLAG_SMART

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: <CAPDyKFqshaVNzHPe0KL3HRTpiuzyKVJ-LuDsaAne5PawFLMJow@mail.gmail.com>
Date: Tue, 18 Feb 2025 15:39:47 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Alan Stern <stern@...land.harvard.edu>, 
	Bjorn Helgaas <helgaas@...nel.org>, Linux PCI <linux-pci@...r.kernel.org>, 
	Johan Hovold <johan@...nel.org>, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	Jon Hunter <jonathanh@...dia.com>, Linux ACPI <linux-acpi@...r.kernel.org>, 
	Mika Westerberg <mika.westerberg@...ux.intel.com>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Saravana Kannan <saravanak@...gle.com>
Subject: Re: [PATCH v1 3/3] PM: sleep: Use DPM_FLAG_SMART_SUSPEND conditionally

[...]

> > >
> > > +static void device_prepare_smart_suspend(struct device *dev)
> > > +{
> > > +       struct device_link *link;
> > > +       int idx;
> > > +
> > > +       /*
> > > +        * The "smart suspend" feature is enabled for devices whose drivers ask
> > > +        * for it and for devices without PM callbacks unless runtime PM is
> > > +        * disabled and enabling it is blocked for them.
> > > +        *
> > > +        * However, if "smart suspend" is not enabled for the device's parent
> > > +        * or any of its suppliers that take runtime PM into account, it cannot
> > > +        * be enabled for the device either.
> > > +        */
> > > +       dev->power.smart_suspend = (dev->power.no_pm_callbacks ||
> > > +               dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) &&
> > > +               !pm_runtime_blocked(dev);
> > > +
> > > +       if (!dev->power.smart_suspend)
> > > +               return;
> > > +
> > > +       if (dev->parent && !pm_runtime_blocked(dev->parent) &&
> > > +           !dev->parent->power.ignore_children && !dev->parent->power.smart_suspend) {
> > > +               dev->power.smart_suspend = false;
> > > +               return;
> > > +       }
> > > +
> > > +       idx = device_links_read_lock();
> > > +
> > > +       list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) {
> > > +               if (!(link->flags | DL_FLAG_PM_RUNTIME))
> > > +                       continue;
> > > +
> > > +               if (!pm_runtime_blocked(link->supplier) &&
> > > +                   !link->supplier->power.smart_suspend) {
> >
> > This requires device_prepare() for all suppliers to be run before its
> > consumer. Is that always the case?
>
> Yes, it is by design.

Okay! I was worried that fw_devlink could mess this up.

>
> > > +                       dev->power.smart_suspend = false;
> > > +                       break;
> > > +               }
> > > +       }
> > > +
> > > +       device_links_read_unlock(idx);
> >
> > From an execution overhead point of view, did you check if the above
> > code had some measurable impact on the latency for dpm_prepare()?
>
> It didn't on my systems.
>
> For the vast majority of devices the overhead is just checking
> power.no_pm_callbacks and DPM_FLAG_SMART_SUSPEND.  For some,
> pm_runtime_blocked() needs to be called which requires grabbing a
> spinlock and there are only a few with power.smart_suspend set to
> start with.
>
> I'm wondering why you didn't have this concern regarding other changes
> that involved walking suppliers or consumers, though.

Well, the concern is mostly generic from my side. When introducing
code that potentially could impact latency during system
suspend/resume, we should at least check it.

That said, I think the approach makes sense, so no objections from my side!

Feel free to add:
Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>

Kind regards
Uffe