diff mbox series

[01/12] PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags

Message ID 1998650.Q52BuGQTI4@aspire.rjw.lan
State Not Applicable
Headers show
Series PM / sleep: Driver flags for system suspend/resume | expand

Commit Message

Rafael J. Wysocki Oct. 16, 2017, 1:29 a.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The motivation for this change is to provide a way to work around
a problem with the direct-complete mechanism used for avoiding
system suspend/resume handling for devices in runtime suspend.

The problem is that some middle layer code (the PCI bus type and
the ACPI PM domain in particular) returns positive values from its
system suspend ->prepare callbacks regardless of whether the driver's
->prepare returns a positive value or 0, which effectively prevents
drivers from being able to control the direct-complete feature.
Some drivers need that control, however, and the PCI bus type has
grown its own flag to deal with this issue, but since it is not
limited to PCI, it is better to address it by adding driver flags at
the core level.

To that end, add a driver_flags field to struct dev_pm_info for flags
that can be set by device drivers at the probe time to inform the PM
core and/or bus types, PM domains and so on on the capabilities and/or
preferences of device drivers.  Also add two static inline helpers
for setting that field and testing it against a given set of flags
and make the driver core clear it automatically on driver remove
and probe failures.

Define and document two PM driver flags related to the direct-
complete feature: NEVER_SKIP and SMART_PREPARE that can be used,
respectively, to indicate to the PM core that the direct-complete
mechanism should never be used for the device and to inform the
middle layer code (bus types, PM domains etc) that it can only
request the PM core to use the direct-complete mechanism for
the device (by returning a positive value from its ->prepare
callback) if it also has been requested by the driver.

While at it, make the core check pm_runtime_suspended() when
setting power.direct_complete so that it doesn't need to be
checked by ->prepare callbacks.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/driver-api/pm/devices.rst |   14 ++++++++++++++
 Documentation/power/pci.txt             |   19 +++++++++++++++++++
 drivers/acpi/device_pm.c                |    3 +++
 drivers/base/dd.c                       |    2 ++
 drivers/base/power/main.c               |    4 +++-
 drivers/pci/pci-driver.c                |    5 ++++-
 include/linux/device.h                  |   10 ++++++++++
 include/linux/pm.h                      |   20 ++++++++++++++++++++
 8 files changed, 75 insertions(+), 2 deletions(-)

Comments

Lukas Wunner Oct. 16, 2017, 5:34 a.m. UTC | #1 
On Mon, Oct 16, 2017 at 03:29:02AM +0200, Rafael J. Wysocki wrote:
> +	:c:func:`dev_pm_set_driver_flags` helper function.]  If the first of
> +	tese flags is set, the PM core will not apply the direct-complete
        ^
	these

> +	proceudre described above to the given device and, consequenty, to any
        ^
        procedure

Lukas
gregkh@linuxfoundation.org Oct. 16, 2017, 6:28 a.m. UTC | #2 
On Mon, Oct 16, 2017 at 03:29:02AM +0200, Rafael J. Wysocki wrote:
>  struct dev_pm_info {
>  	pm_message_t		power_state;
>  	unsigned int		can_wakeup:1;
> @@ -561,6 +580,7 @@ struct dev_pm_info {
>  	bool			is_late_suspended:1;
>  	bool			early_init:1;	/* Owned by the PM core */
>  	bool			direct_complete:1;	/* Owned by the PM core */
> +	unsigned int		driver_flags;

Minor nit, u32 or u64?

thanks,

greg k-h
gregkh@linuxfoundation.org Oct. 16, 2017, 6:31 a.m. UTC | #3 
On Mon, Oct 16, 2017 at 03:29:02AM +0200, Rafael J. Wysocki wrote:
> +static inline void dev_pm_set_driver_flags(struct device *dev, unsigned int flags)
> +{
> +	dev->power.driver_flags = flags;
> +}

Should this function just set the specific bit?  Or is it going to be ok
to set the whole value, meaning you aren't going to care about turning
on and off specific flags over the lifetime of the driver/device, you
are just going to set them once and then just test them as needed?

thanks,

greg k-h
Alan Stern Oct. 16, 2017, 8:16 p.m. UTC | #4 
On Mon, 16 Oct 2017, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The motivation for this change is to provide a way to work around
> a problem with the direct-complete mechanism used for avoiding
> system suspend/resume handling for devices in runtime suspend.
> 
> The problem is that some middle layer code (the PCI bus type and
> the ACPI PM domain in particular) returns positive values from its
> system suspend ->prepare callbacks regardless of whether the driver's
> ->prepare returns a positive value or 0, which effectively prevents
> drivers from being able to control the direct-complete feature.
> Some drivers need that control, however, and the PCI bus type has
> grown its own flag to deal with this issue, but since it is not
> limited to PCI, it is better to address it by adding driver flags at
> the core level.

I'm curious: Why does the PCI bus type (and others) do this?  Why 
doesn't it do what the driver says to do?

Alan Stern
Rafael J. Wysocki Oct. 16, 2017, 10:03 p.m. UTC | #5 
On Monday, October 16, 2017 7:34:52 AM CEST Lukas Wunner wrote:
> On Mon, Oct 16, 2017 at 03:29:02AM +0200, Rafael J. Wysocki wrote:
> > +	:c:func:`dev_pm_set_driver_flags` helper function.]  If the first of
> > +	tese flags is set, the PM core will not apply the direct-complete
>         ^
> 	these
> 
> > +	proceudre described above to the given device and, consequenty, to any
>         ^
>         procedure
> 

Thanks!
Rafael J. Wysocki Oct. 16, 2017, 10:05 p.m. UTC | #6 
On Monday, October 16, 2017 8:28:52 AM CEST Greg Kroah-Hartman wrote:
> On Mon, Oct 16, 2017 at 03:29:02AM +0200, Rafael J. Wysocki wrote:
> >  struct dev_pm_info {
> >  	pm_message_t		power_state;
> >  	unsigned int		can_wakeup:1;
> > @@ -561,6 +580,7 @@ struct dev_pm_info {
> >  	bool			is_late_suspended:1;
> >  	bool			early_init:1;	/* Owned by the PM core */
> >  	bool			direct_complete:1;	/* Owned by the PM core */
> > +	unsigned int		driver_flags;
> 
> Minor nit, u32 or u64?

u32 I think, will update.

BTW, there's a mess in this struct overall and I'd like all of the bit fileds
to be the same type (and that shouldn't be bool IMO :-)).

Do you prefer u32 or unsinged int?

Thanks,
Rafael
Rafael J. Wysocki Oct. 16, 2017, 10:07 p.m. UTC | #7 
On Monday, October 16, 2017 8:31:22 AM CEST Greg Kroah-Hartman wrote:
> On Mon, Oct 16, 2017 at 03:29:02AM +0200, Rafael J. Wysocki wrote:
> > +static inline void dev_pm_set_driver_flags(struct device *dev, unsigned int flags)
> > +{
> > +	dev->power.driver_flags = flags;
> > +}
> 
> Should this function just set the specific bit?  Or is it going to be ok
> to set the whole value, meaning you aren't going to care about turning
> on and off specific flags over the lifetime of the driver/device, you
> are just going to set them once and then just test them as needed?

The idea is to set them once and they should not be touched again until
the driver (or device) goes away, so that would be the whole value at once
(and one of the i2c-designware-platdrv patches actually sets multiple flags
in one go).

Thanks,
Rafael
Rafael J. Wysocki Oct. 16, 2017, 10:11 p.m. UTC | #8 
On Monday, October 16, 2017 10:16:15 PM CEST Alan Stern wrote:
> On Mon, 16 Oct 2017, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > The motivation for this change is to provide a way to work around
> > a problem with the direct-complete mechanism used for avoiding
> > system suspend/resume handling for devices in runtime suspend.
> > 
> > The problem is that some middle layer code (the PCI bus type and
> > the ACPI PM domain in particular) returns positive values from its
> > system suspend ->prepare callbacks regardless of whether the driver's
> > ->prepare returns a positive value or 0, which effectively prevents
> > drivers from being able to control the direct-complete feature.
> > Some drivers need that control, however, and the PCI bus type has
> > grown its own flag to deal with this issue, but since it is not
> > limited to PCI, it is better to address it by adding driver flags at
> > the core level.
> 
> I'm curious: Why does the PCI bus type (and others) do this?  Why 
> doesn't it do what the driver says to do?

Well, the idea was that it might work for the existing drivers without the
need to modify them (and they would have had to be modified had the driver's
->prepare return value been required to be taken into account).

It actually does work for them in general, although with some notable
exceptions.

Thanks,
Rafael
gregkh@linuxfoundation.org Oct. 17, 2017, 7:15 a.m. UTC | #9 
On Tue, Oct 17, 2017 at 12:05:11AM +0200, Rafael J. Wysocki wrote:
> On Monday, October 16, 2017 8:28:52 AM CEST Greg Kroah-Hartman wrote:
> > On Mon, Oct 16, 2017 at 03:29:02AM +0200, Rafael J. Wysocki wrote:
> > >  struct dev_pm_info {
> > >  	pm_message_t		power_state;
> > >  	unsigned int		can_wakeup:1;
> > > @@ -561,6 +580,7 @@ struct dev_pm_info {
> > >  	bool			is_late_suspended:1;
> > >  	bool			early_init:1;	/* Owned by the PM core */
> > >  	bool			direct_complete:1;	/* Owned by the PM core */
> > > +	unsigned int		driver_flags;
> > 
> > Minor nit, u32 or u64?
> 
> u32 I think, will update.
> 
> BTW, there's a mess in this struct overall and I'd like all of the bit fileds
> to be the same type (and that shouldn't be bool IMO :-)).
> 
> Do you prefer u32 or unsinged int?

I always prefer an explicit size for variables, unless it's a "generic
loop" type thing.  So I'll always say "u32" for this.

And cleaning up the structure would be great, it's grown over time in
odd ways as you point out.

thanks,

greg k-h
gregkh@linuxfoundation.org Oct. 17, 2017, 1:26 p.m. UTC | #10 
On Tue, Oct 17, 2017 at 12:07:37AM +0200, Rafael J. Wysocki wrote:
> On Monday, October 16, 2017 8:31:22 AM CEST Greg Kroah-Hartman wrote:
> > On Mon, Oct 16, 2017 at 03:29:02AM +0200, Rafael J. Wysocki wrote:
> > > +static inline void dev_pm_set_driver_flags(struct device *dev, unsigned int flags)
> > > +{
> > > +	dev->power.driver_flags = flags;
> > > +}
> > 
> > Should this function just set the specific bit?  Or is it going to be ok
> > to set the whole value, meaning you aren't going to care about turning
> > on and off specific flags over the lifetime of the driver/device, you
> > are just going to set them once and then just test them as needed?
> 
> The idea is to set them once and they should not be touched again until
> the driver (or device) goes away, so that would be the whole value at once
> (and one of the i2c-designware-platdrv patches actually sets multiple flags
> in one go).

Ok, thanks.
Rafael J. Wysocki Oct. 17, 2017, 3:26 p.m. UTC | #11 
On Tuesday, October 17, 2017 9:15:43 AM CEST Greg Kroah-Hartman wrote:
> On Tue, Oct 17, 2017 at 12:05:11AM +0200, Rafael J. Wysocki wrote:
> > On Monday, October 16, 2017 8:28:52 AM CEST Greg Kroah-Hartman wrote:
> > > On Mon, Oct 16, 2017 at 03:29:02AM +0200, Rafael J. Wysocki wrote:
> > > >  struct dev_pm_info {
> > > >  	pm_message_t		power_state;
> > > >  	unsigned int		can_wakeup:1;
> > > > @@ -561,6 +580,7 @@ struct dev_pm_info {
> > > >  	bool			is_late_suspended:1;
> > > >  	bool			early_init:1;	/* Owned by the PM core */
> > > >  	bool			direct_complete:1;	/* Owned by the PM core */
> > > > +	unsigned int		driver_flags;
> > > 
> > > Minor nit, u32 or u64?
> > 
> > u32 I think, will update.
> > 
> > BTW, there's a mess in this struct overall and I'd like all of the bit fileds
> > to be the same type (and that shouldn't be bool IMO :-)).
> > 
> > Do you prefer u32 or unsinged int?
> 
> I always prefer an explicit size for variables, unless it's a "generic
> loop" type thing.  So I'll always say "u32" for this.
> 
> And cleaning up the structure would be great, it's grown over time in
> odd ways as you point out.

OK, but that will be separate from this work.

Thanks,
Rafael
gregkh@linuxfoundation.org Oct. 18, 2017, 6:56 a.m. UTC | #12 
On Tue, Oct 17, 2017 at 05:26:20PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, October 17, 2017 9:15:43 AM CEST Greg Kroah-Hartman wrote:
> > On Tue, Oct 17, 2017 at 12:05:11AM +0200, Rafael J. Wysocki wrote:
> > > On Monday, October 16, 2017 8:28:52 AM CEST Greg Kroah-Hartman wrote:
> > > > On Mon, Oct 16, 2017 at 03:29:02AM +0200, Rafael J. Wysocki wrote:
> > > > >  struct dev_pm_info {
> > > > >  	pm_message_t		power_state;
> > > > >  	unsigned int		can_wakeup:1;
> > > > > @@ -561,6 +580,7 @@ struct dev_pm_info {
> > > > >  	bool			is_late_suspended:1;
> > > > >  	bool			early_init:1;	/* Owned by the PM core */
> > > > >  	bool			direct_complete:1;	/* Owned by the PM core */
> > > > > +	unsigned int		driver_flags;
> > > > 
> > > > Minor nit, u32 or u64?
> > > 
> > > u32 I think, will update.
> > > 
> > > BTW, there's a mess in this struct overall and I'd like all of the bit fileds
> > > to be the same type (and that shouldn't be bool IMO :-)).
> > > 
> > > Do you prefer u32 or unsinged int?
> > 
> > I always prefer an explicit size for variables, unless it's a "generic
> > loop" type thing.  So I'll always say "u32" for this.
> > 
> > And cleaning up the structure would be great, it's grown over time in
> > odd ways as you point out.
> 
> OK, but that will be separate from this work.

Of course :)
diff mbox series

Patch

Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -1070,6 +1070,16 @@  static inline void dev_pm_syscore_device
 #endif
 }
 
+static inline void dev_pm_set_driver_flags(struct device *dev, unsigned int flags)
+{
+	dev->power.driver_flags = flags;
+}
+
+static inline bool dev_pm_test_driver_flags(struct device *dev, unsigned int flags)
+{
+	return !!(dev->power.driver_flags & flags);
+}
+
 static inline void device_lock(struct device *dev)
 {
 	mutex_lock(&dev->mutex);
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -550,6 +550,25 @@  struct pm_subsys_data {
 #endif
 };
 
+/*
+ * Driver flags to control system suspend/resume behavior.
+ *
+ * These flags can be set by device drivers at the probe time.  They need not be
+ * cleared by the drivers as the driver core will take care of that.
+ *
+ * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device.
+ * SMART_PREPARE: Check the return value of the driver's ->prepare callback.
+ *
+ * Setting SMART_PREPARE instructs bus types and PM domains which may want
+ * system suspend/resume callbacks to be skipped for the device to return 0 from
+ * their ->prepare callbacks if the driver's ->prepare callback returns 0 (in
+ * other words, the system suspend/resume callbacks can only be skipped for the
+ * device if its driver doesn't object against that).  This flag has no effect
+ * if NEVER_SKIP is set.
+ */
+#define DPM_FLAG_NEVER_SKIP	BIT(0)
+#define DPM_FLAG_SMART_PREPARE	BIT(1)
+
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned int		can_wakeup:1;
@@ -561,6 +580,7 @@  struct dev_pm_info {
 	bool			is_late_suspended:1;
 	bool			early_init:1;	/* Owned by the PM core */
 	bool			direct_complete:1;	/* Owned by the PM core */
+	unsigned int		driver_flags;
 	spinlock_t		lock;
 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
Index: linux-pm/drivers/base/dd.c
===================================================================
--- linux-pm.orig/drivers/base/dd.c
+++ linux-pm/drivers/base/dd.c
@@ -464,6 +464,7 @@  pinctrl_bind_failed:
 	if (dev->pm_domain && dev->pm_domain->dismiss)
 		dev->pm_domain->dismiss(dev);
 	pm_runtime_reinit(dev);
+	dev_pm_set_driver_flags(dev, 0);
 
 	switch (ret) {
 	case -EPROBE_DEFER:
@@ -869,6 +870,7 @@  static void __device_release_driver(stru
 		if (dev->pm_domain && dev->pm_domain->dismiss)
 			dev->pm_domain->dismiss(dev);
 		pm_runtime_reinit(dev);
+		dev_pm_set_driver_flags(dev, 0);
 
 		klist_remove(&dev->p->knode_driver);
 		device_pm_check_callbacks(dev);
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1700,7 +1700,9 @@  unlock:
 	 * applies to suspend transitions, however.
 	 */
 	spin_lock_irq(&dev->power.lock);
-	dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND;
+	dev->power.direct_complete = state.event == PM_EVENT_SUSPEND &&
+		pm_runtime_suspended(dev) && ret > 0 &&
+		!dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP);
 	spin_unlock_irq(&dev->power.lock);
 	return 0;
 }
Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -682,8 +682,11 @@  static int pci_pm_prepare(struct device
 
 	if (drv && drv->pm && drv->pm->prepare) {
 		int error = drv->pm->prepare(dev);
-		if (error)
+		if (error < 0)
 			return error;
+
+		if (!error && dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_PREPARE))
+			return 0;
 	}
 	return pci_dev_keep_suspended(to_pci_dev(dev));
 }
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -965,6 +965,9 @@  int acpi_subsys_prepare(struct device *d
 	if (ret < 0)
 		return ret;
 
+	if (!ret && dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_PREPARE))
+		return 0;
+
 	if (!adev || !pm_runtime_suspended(dev))
 		return 0;
 
Index: linux-pm/Documentation/driver-api/pm/devices.rst
===================================================================
--- linux-pm.orig/Documentation/driver-api/pm/devices.rst
+++ linux-pm/Documentation/driver-api/pm/devices.rst
@@ -354,6 +354,20 @@  the phases are: ``prepare``, ``suspend``
 	is because all such devices are initially set to runtime-suspended with
 	runtime PM disabled.
 
+	This feature also can be controlled by device drivers by using the
+	``DPM_FLAG_NEVER_SKIP`` and ``DPM_FLAG_SMART_PREPARE`` driver power
+	management flags.  [Typically, they are set at the time the driver is
+	probed against the device in question by passing them to the
+	:c:func:`dev_pm_set_driver_flags` helper function.]  If the first of
+	tese flags is set, the PM core will not apply the direct-complete
+	proceudre described above to the given device and, consequenty, to any
+	of its ancestors.  The second flag, when set, informs the middle layer
+	code (bus types, device types, PM domains, classes) that it should take
+	the return value of the ``->prepare`` callback provided by the driver
+	into account and it may only return a positive value from its own
+	``->prepare`` callback if the driver's one also has returned a positive
+	value.
+
     2.	The ``->suspend`` methods should quiesce the device to stop it from
 	performing I/O.  They also may save the device registers and put it into
 	the appropriate low-power state, depending on the bus type the device is
Index: linux-pm/Documentation/power/pci.txt
===================================================================
--- linux-pm.orig/Documentation/power/pci.txt
+++ linux-pm/Documentation/power/pci.txt
@@ -961,6 +961,25 @@  dev_pm_ops to indicate that one suspend
 .suspend(), .freeze(), and .poweroff() members and one resume routine is to
 be pointed to by the .resume(), .thaw(), and .restore() members.
 
+3.1.19. Driver Flags for Power Management
+
+The PM core allows device drivers to set flags that influence the handling of
+power management for the devices by the core itself and by middle layer code
+including the PCI bus type.  The flags should be set once at the driver probe
+time with the help of the dev_pm_set_driver_flags() function and they should not
+be updated directly afterwards.
+
+The DPM_FLAG_NEVER_SKIP flag prevents the PM core from using the direct-complete
+mechanism allowing device suspend/resume callbacks to be skipped if the device
+is in runtime suspend when the system suspend starts.  That also affects all of
+the ancestors of the device, so this flag should only be used if absolutely
+necessary.
+
+The DPM_FLAG_SMART_PREPARE flag instructs the PCI bus type to only return a
+positive value from pci_pm_prepare() if the ->prepare callback provided by the
+driver of the device returns a positive value.  That allows the driver to opt
+out from using the direct-complete mechanism dynamically.
+
 3.2. Device Runtime Power Management
 ------------------------------------
 In addition to providing device power management callbacks PCI device drivers