[PATCH 0/2] ACPI / platform: Put devices enumerated via ACPI into D0 before probing drivers

[PATCH 0/2] ACPI / platform: Put devices enumerated via ACPI into D0 before probing drivers

[Rafael J. Wysocki]

Hi All,

The following two patches are meant to work around a problem where some
devices may be in the ACPI D3cold power state (power off) before their
drivers' .probe() routines are called, but those routines assume devices
to be operational.  They also allow ACPI PM to be used going forward with
the devices in question.

[1/2] - Allow ACPI PM attach/detach routines to change device power states.
[2/2] - Call the ACPI PM attach/detach routines in the platform probe and
        remove/shutdown callbacks (and change power states of devices as
        necessary).

The patches are on top of the current linux-next branch of the linux-pm.git
tree.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make it possible to ask the routines used for adding/removing devices
to/from the general ACPI PM domain, acpi_dev_pm_attach() and
acpi_dev_pm_detach(), respectively, to change the power states of
devices so that they are put into the full-power state automatically
by acpi_dev_pm_attach() and into the lowest-power state available
automatically by acpi_dev_pm_detach().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   28 ++++++++++++++++++++++++----
 include/linux/acpi.h     |   11 +++++++----
 2 files changed, 31 insertions(+), 8 deletions(-)

Index: linux/drivers/acpi/device_pm.c
===================================================================
--- linux.orig/drivers/acpi/device_pm.c
+++ linux/drivers/acpi/device_pm.c
@@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
 /**
  * acpi_dev_pm_attach - Prepare device for ACPI power management.
  * @dev: Device to prepare.
+ * @power_on: Whether or not to power on the device.
  *
  * If @dev has a valid ACPI handle that has a valid struct acpi_device object
  * attached to it, install a wakeup notification handler for the device and
- * add it to the general ACPI PM domain.
+ * add it to the general ACPI PM domain.  If @power_on is set, the device will
+ * be put into the ACPI D0 state before the function returns.
  *
  * This assumes that the @dev's bus type uses generic power management callbacks
  * (or doesn't use any power management callbacks at all).
@@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
  * Callers must ensure proper synchronization of this function with power
  * management callbacks.
  */
-int acpi_dev_pm_attach(struct device *dev)
+int acpi_dev_pm_attach(struct device *dev, bool power_on)
 {
 	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
 
@@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
 
 	acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
 	dev->pm_domain = &acpi_general_pm_domain;
+	if (power_on) {
+		acpi_dev_pm_full_power(adev);
+		__acpi_device_run_wake(adev, false);
+	}
 	return 0;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
@@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
 /**
  * acpi_dev_pm_detach - Remove ACPI power management from the device.
  * @dev: Device to take care of.
+ * @power_off: Whether or not to try to remove power from the device.
  *
  * Remove the device from the general ACPI PM domain and remove its wakeup
- * notifier.
+ * notifier.  If @power_off is set, additionally remove power from the device if
+ * possible.
  *
  * Callers must ensure proper synchronization of this function with power
  * management callbacks.
  */
-void acpi_dev_pm_detach(struct device *dev)
+void acpi_dev_pm_detach(struct device *dev, bool power_off)
 {
 	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
 
 	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
 		dev->pm_domain = NULL;
 		acpi_remove_pm_notifier(adev, acpi_wakeup_device);
+		if (power_off) {
+			/*
+			 * If the device's PM QoS resume latency limit or flags
+			 * have been exposed to user space, they have to be
+			 * hidden at this point, so that they don't affect the
+			 * choice of the low-power state to put the device into.
+			 */
+			dev_pm_qos_hide_latency_limit(dev);
+			dev_pm_qos_hide_flags(dev);
+			__acpi_device_run_wake(adev, false);
+			acpi_dev_pm_low_power(dev, adev, ACPI_STATE_S0);
+		}
 	}
 }
 EXPORT_SYMBOL_GPL(acpi_dev_pm_detach);
Index: linux/include/linux/acpi.h
===================================================================
--- linux.orig/include/linux/acpi.h
+++ linux/include/linux/acpi.h
@@ -510,11 +510,14 @@ static inline int acpi_subsys_resume_ear
 #endif
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_PM)
-int acpi_dev_pm_attach(struct device *dev);
-int acpi_dev_pm_detach(struct device *dev);
+int acpi_dev_pm_attach(struct device *dev, bool power_on);
+int acpi_dev_pm_detach(struct device *dev, bool power_off);
 #else
-static inline int acpi_dev_pm_attach(struct device *dev) { return -ENODEV; }
-static inline void acpi_dev_pm_detach(struct device *dev) {}
+static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
+{
+	return -ENODEV;
+}
+static inline void acpi_dev_pm_detach(struct device *dev, bool power_off) {}
 #endif
 
 #ifdef CONFIG_ACPI

Re: [PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states

[Huang Ying]

On Sun, 2012-11-25 at 15:55 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make it possible to ask the routines used for adding/removing devices
> to/from the general ACPI PM domain, acpi_dev_pm_attach() and
> acpi_dev_pm_detach(), respectively, to change the power states of
> devices so that they are put into the full-power state automatically
> by acpi_dev_pm_attach() and into the lowest-power state available
> automatically by acpi_dev_pm_detach().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/device_pm.c |   28 ++++++++++++++++++++++++----
>  include/linux/acpi.h     |   11 +++++++----
>  2 files changed, 31 insertions(+), 8 deletions(-)
> 
> Index: linux/drivers/acpi/device_pm.c
> ===================================================================
> --- linux.orig/drivers/acpi/device_pm.c
> +++ linux/drivers/acpi/device_pm.c
> @@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
>  /**
>   * acpi_dev_pm_attach - Prepare device for ACPI power management.
>   * @dev: Device to prepare.
> + * @power_on: Whether or not to power on the device.
>   *
>   * If @dev has a valid ACPI handle that has a valid struct acpi_device object
>   * attached to it, install a wakeup notification handler for the device and
> - * add it to the general ACPI PM domain.
> + * add it to the general ACPI PM domain.  If @power_on is set, the device will
> + * be put into the ACPI D0 state before the function returns.
>   *
>   * This assumes that the @dev's bus type uses generic power management callbacks
>   * (or doesn't use any power management callbacks at all).
> @@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
>   * Callers must ensure proper synchronization of this function with power
>   * management callbacks.
>   */
> -int acpi_dev_pm_attach(struct device *dev)
> +int acpi_dev_pm_attach(struct device *dev, bool power_on)
>  {
>  	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
>  
> @@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
>  
>  	acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
>  	dev->pm_domain = &acpi_general_pm_domain;
> +	if (power_on) {
> +		acpi_dev_pm_full_power(adev);
> +		__acpi_device_run_wake(adev, false);
> +	}
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> @@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
>  /**
>   * acpi_dev_pm_detach - Remove ACPI power management from the device.
>   * @dev: Device to take care of.
> + * @power_off: Whether or not to try to remove power from the device.
>   *
>   * Remove the device from the general ACPI PM domain and remove its wakeup
> - * notifier.
> + * notifier.  If @power_off is set, additionally remove power from the device if
> + * possible.
>   *
>   * Callers must ensure proper synchronization of this function with power
>   * management callbacks.
>   */
> -void acpi_dev_pm_detach(struct device *dev)
> +void acpi_dev_pm_detach(struct device *dev, bool power_off)
>  {
>  	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
>  
>  	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
>  		dev->pm_domain = NULL;
>  		acpi_remove_pm_notifier(adev, acpi_wakeup_device);
> +		if (power_off) {
> +			/*
> +			 * If the device's PM QoS resume latency limit or flags
> +			 * have been exposed to user space, they have to be
> +			 * hidden at this point, so that they don't affect the
> +			 * choice of the low-power state to put the device into.
> +			 */
> +			dev_pm_qos_hide_latency_limit(dev);
> +			dev_pm_qos_hide_flags(dev);

NO_POWER_OFF flag is ignored here.  Is it possible for some device (or
corresponding ACPI method) has broken D3cold implementation, so that the
user need a way to disable it?

Best Regards,
Huang Ying

> +			__acpi_device_run_wake(adev, false);
> +			acpi_dev_pm_low_power(dev, adev, ACPI_STATE_S0);
> +		}
>  	}
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_pm_detach);
> Index: linux/include/linux/acpi.h
> ===================================================================
> --- linux.orig/include/linux/acpi.h
> +++ linux/include/linux/acpi.h
> @@ -510,11 +510,14 @@ static inline int acpi_subsys_resume_ear
>  #endif
>  
>  #if defined(CONFIG_ACPI) && defined(CONFIG_PM)
> -int acpi_dev_pm_attach(struct device *dev);
> -int acpi_dev_pm_detach(struct device *dev);
> +int acpi_dev_pm_attach(struct device *dev, bool power_on);
> +int acpi_dev_pm_detach(struct device *dev, bool power_off);
>  #else
> -static inline int acpi_dev_pm_attach(struct device *dev) { return -ENODEV; }
> -static inline void acpi_dev_pm_detach(struct device *dev) {}
> +static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
> +{
> +	return -ENODEV;
> +}
> +static inline void acpi_dev_pm_detach(struct device *dev, bool power_off) {}
>  #endif
>  
>  #ifdef CONFIG_ACPI
> 

Re: [PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states

[Rafael J. Wysocki]

On Monday, November 26, 2012 08:43:10 AM Huang Ying wrote:
> On Sun, 2012-11-25 at 15:55 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Make it possible to ask the routines used for adding/removing devices
> > to/from the general ACPI PM domain, acpi_dev_pm_attach() and
> > acpi_dev_pm_detach(), respectively, to change the power states of
> > devices so that they are put into the full-power state automatically
> > by acpi_dev_pm_attach() and into the lowest-power state available
> > automatically by acpi_dev_pm_detach().
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/device_pm.c |   28 ++++++++++++++++++++++++----
> >  include/linux/acpi.h     |   11 +++++++----
> >  2 files changed, 31 insertions(+), 8 deletions(-)
> > 
> > Index: linux/drivers/acpi/device_pm.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/device_pm.c
> > +++ linux/drivers/acpi/device_pm.c
> > @@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
> >  /**
> >   * acpi_dev_pm_attach - Prepare device for ACPI power management.
> >   * @dev: Device to prepare.
> > + * @power_on: Whether or not to power on the device.
> >   *
> >   * If @dev has a valid ACPI handle that has a valid struct acpi_device object
> >   * attached to it, install a wakeup notification handler for the device and
> > - * add it to the general ACPI PM domain.
> > + * add it to the general ACPI PM domain.  If @power_on is set, the device will
> > + * be put into the ACPI D0 state before the function returns.
> >   *
> >   * This assumes that the @dev's bus type uses generic power management callbacks
> >   * (or doesn't use any power management callbacks at all).
> > @@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
> >   * Callers must ensure proper synchronization of this function with power
> >   * management callbacks.
> >   */
> > -int acpi_dev_pm_attach(struct device *dev)
> > +int acpi_dev_pm_attach(struct device *dev, bool power_on)
> >  {
> >  	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> >  
> > @@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
> >  
> >  	acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
> >  	dev->pm_domain = &acpi_general_pm_domain;
> > +	if (power_on) {
> > +		acpi_dev_pm_full_power(adev);
> > +		__acpi_device_run_wake(adev, false);
> > +	}
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > @@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> >  /**
> >   * acpi_dev_pm_detach - Remove ACPI power management from the device.
> >   * @dev: Device to take care of.
> > + * @power_off: Whether or not to try to remove power from the device.
> >   *
> >   * Remove the device from the general ACPI PM domain and remove its wakeup
> > - * notifier.
> > + * notifier.  If @power_off is set, additionally remove power from the device if
> > + * possible.
> >   *
> >   * Callers must ensure proper synchronization of this function with power
> >   * management callbacks.
> >   */
> > -void acpi_dev_pm_detach(struct device *dev)
> > +void acpi_dev_pm_detach(struct device *dev, bool power_off)
> >  {
> >  	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> >  
> >  	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> >  		dev->pm_domain = NULL;
> >  		acpi_remove_pm_notifier(adev, acpi_wakeup_device);
> > +		if (power_off) {
> > +			/*
> > +			 * If the device's PM QoS resume latency limit or flags
> > +			 * have been exposed to user space, they have to be
> > +			 * hidden at this point, so that they don't affect the
> > +			 * choice of the low-power state to put the device into.
> > +			 */
> > +			dev_pm_qos_hide_latency_limit(dev);
> > +			dev_pm_qos_hide_flags(dev);
> 
> NO_POWER_OFF flag is ignored here.  Is it possible for some device (or
> corresponding ACPI method) has broken D3cold implementation, so that the
> user need a way to disable it?

We are removing the driver here and we haven't exposed the flags, have we?

If the driver had exposed them and then left them behind, we need to clean up
after it and that's why I added those two lines.

In the future we may decide that the flags need to be exposed by the core,
because of the broken hardware, for example, before the driver's .probe()
routine is run and in that case we obviously won't be removing them here any
more.  For now, however, I think it's better to hide them here.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states

[Huang Ying]

On Mon, 2012-11-26 at 02:00 +0100, Rafael J. Wysocki wrote:
> On Monday, November 26, 2012 08:43:10 AM Huang Ying wrote:
> > On Sun, 2012-11-25 at 15:55 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > Make it possible to ask the routines used for adding/removing devices
> > > to/from the general ACPI PM domain, acpi_dev_pm_attach() and
> > > acpi_dev_pm_detach(), respectively, to change the power states of
> > > devices so that they are put into the full-power state automatically
> > > by acpi_dev_pm_attach() and into the lowest-power state available
> > > automatically by acpi_dev_pm_detach().
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/acpi/device_pm.c |   28 ++++++++++++++++++++++++----
> > >  include/linux/acpi.h     |   11 +++++++----
> > >  2 files changed, 31 insertions(+), 8 deletions(-)
> > > 
> > > Index: linux/drivers/acpi/device_pm.c
> > > ===================================================================
> > > --- linux.orig/drivers/acpi/device_pm.c
> > > +++ linux/drivers/acpi/device_pm.c
> > > @@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
> > >  /**
> > >   * acpi_dev_pm_attach - Prepare device for ACPI power management.
> > >   * @dev: Device to prepare.
> > > + * @power_on: Whether or not to power on the device.
> > >   *
> > >   * If @dev has a valid ACPI handle that has a valid struct acpi_device object
> > >   * attached to it, install a wakeup notification handler for the device and
> > > - * add it to the general ACPI PM domain.
> > > + * add it to the general ACPI PM domain.  If @power_on is set, the device will
> > > + * be put into the ACPI D0 state before the function returns.
> > >   *
> > >   * This assumes that the @dev's bus type uses generic power management callbacks
> > >   * (or doesn't use any power management callbacks at all).
> > > @@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
> > >   * Callers must ensure proper synchronization of this function with power
> > >   * management callbacks.
> > >   */
> > > -int acpi_dev_pm_attach(struct device *dev)
> > > +int acpi_dev_pm_attach(struct device *dev, bool power_on)
> > >  {
> > >  	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > >  
> > > @@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
> > >  
> > >  	acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
> > >  	dev->pm_domain = &acpi_general_pm_domain;
> > > +	if (power_on) {
> > > +		acpi_dev_pm_full_power(adev);
> > > +		__acpi_device_run_wake(adev, false);
> > > +	}
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > @@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > >  /**
> > >   * acpi_dev_pm_detach - Remove ACPI power management from the device.
> > >   * @dev: Device to take care of.
> > > + * @power_off: Whether or not to try to remove power from the device.
> > >   *
> > >   * Remove the device from the general ACPI PM domain and remove its wakeup
> > > - * notifier.
> > > + * notifier.  If @power_off is set, additionally remove power from the device if
> > > + * possible.
> > >   *
> > >   * Callers must ensure proper synchronization of this function with power
> > >   * management callbacks.
> > >   */
> > > -void acpi_dev_pm_detach(struct device *dev)
> > > +void acpi_dev_pm_detach(struct device *dev, bool power_off)
> > >  {
> > >  	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > >  
> > >  	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> > >  		dev->pm_domain = NULL;
> > >  		acpi_remove_pm_notifier(adev, acpi_wakeup_device);
> > > +		if (power_off) {
> > > +			/*
> > > +			 * If the device's PM QoS resume latency limit or flags
> > > +			 * have been exposed to user space, they have to be
> > > +			 * hidden at this point, so that they don't affect the
> > > +			 * choice of the low-power state to put the device into.
> > > +			 */
> > > +			dev_pm_qos_hide_latency_limit(dev);
> > > +			dev_pm_qos_hide_flags(dev);
> > 
> > NO_POWER_OFF flag is ignored here.  Is it possible for some device (or
> > corresponding ACPI method) has broken D3cold implementation, so that the
> > user need a way to disable it?
> 
> We are removing the driver here and we haven't exposed the flags, have we?

IMHO, the flag may be exposed by the bus instead of device driver.
Because some bus can determine whether D3cold is supported by the
device.  If the sysfs file for the flag is exposed by the bus, user set
it and we silently ignore it, will it cause confusing for the user?

Best Regards,
Huang Ying

> If the driver had exposed them and then left them behind, we need to clean up
> after it and that's why I added those two lines.
> 
> In the future we may decide that the flags need to be exposed by the core,
> because of the broken hardware, for example, before the driver's .probe()
> routine is run and in that case we obviously won't be removing them here any
> more.  For now, however, I think it's better to hide them here.
> 
> Thanks,
> Rafael
> 
> 

Re: [PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states

[Rafael J. Wysocki]

On Monday, November 26, 2012 09:07:27 AM Huang Ying wrote:
> On Mon, 2012-11-26 at 02:00 +0100, Rafael J. Wysocki wrote:
> > On Monday, November 26, 2012 08:43:10 AM Huang Ying wrote:
> > > On Sun, 2012-11-25 at 15:55 +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > Make it possible to ask the routines used for adding/removing devices
> > > > to/from the general ACPI PM domain, acpi_dev_pm_attach() and
> > > > acpi_dev_pm_detach(), respectively, to change the power states of
> > > > devices so that they are put into the full-power state automatically
> > > > by acpi_dev_pm_attach() and into the lowest-power state available
> > > > automatically by acpi_dev_pm_detach().
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >  drivers/acpi/device_pm.c |   28 ++++++++++++++++++++++++----
> > > >  include/linux/acpi.h     |   11 +++++++----
> > > >  2 files changed, 31 insertions(+), 8 deletions(-)
> > > > 
> > > > Index: linux/drivers/acpi/device_pm.c
> > > > ===================================================================
> > > > --- linux.orig/drivers/acpi/device_pm.c
> > > > +++ linux/drivers/acpi/device_pm.c
> > > > @@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
> > > >  /**
> > > >   * acpi_dev_pm_attach - Prepare device for ACPI power management.
> > > >   * @dev: Device to prepare.
> > > > + * @power_on: Whether or not to power on the device.
> > > >   *
> > > >   * If @dev has a valid ACPI handle that has a valid struct acpi_device object
> > > >   * attached to it, install a wakeup notification handler for the device and
> > > > - * add it to the general ACPI PM domain.
> > > > + * add it to the general ACPI PM domain.  If @power_on is set, the device will
> > > > + * be put into the ACPI D0 state before the function returns.
> > > >   *
> > > >   * This assumes that the @dev's bus type uses generic power management callbacks
> > > >   * (or doesn't use any power management callbacks at all).
> > > > @@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
> > > >   * Callers must ensure proper synchronization of this function with power
> > > >   * management callbacks.
> > > >   */
> > > > -int acpi_dev_pm_attach(struct device *dev)
> > > > +int acpi_dev_pm_attach(struct device *dev, bool power_on)
> > > >  {
> > > >  	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > > >  
> > > > @@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
> > > >  
> > > >  	acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
> > > >  	dev->pm_domain = &acpi_general_pm_domain;
> > > > +	if (power_on) {
> > > > +		acpi_dev_pm_full_power(adev);
> > > > +		__acpi_device_run_wake(adev, false);
> > > > +	}
> > > >  	return 0;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > > @@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > >  /**
> > > >   * acpi_dev_pm_detach - Remove ACPI power management from the device.
> > > >   * @dev: Device to take care of.
> > > > + * @power_off: Whether or not to try to remove power from the device.
> > > >   *
> > > >   * Remove the device from the general ACPI PM domain and remove its wakeup
> > > > - * notifier.
> > > > + * notifier.  If @power_off is set, additionally remove power from the device if
> > > > + * possible.
> > > >   *
> > > >   * Callers must ensure proper synchronization of this function with power
> > > >   * management callbacks.
> > > >   */
> > > > -void acpi_dev_pm_detach(struct device *dev)
> > > > +void acpi_dev_pm_detach(struct device *dev, bool power_off)
> > > >  {
> > > >  	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > > >  
> > > >  	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> > > >  		dev->pm_domain = NULL;
> > > >  		acpi_remove_pm_notifier(adev, acpi_wakeup_device);
> > > > +		if (power_off) {
> > > > +			/*
> > > > +			 * If the device's PM QoS resume latency limit or flags
> > > > +			 * have been exposed to user space, they have to be
> > > > +			 * hidden at this point, so that they don't affect the
> > > > +			 * choice of the low-power state to put the device into.
> > > > +			 */
> > > > +			dev_pm_qos_hide_latency_limit(dev);
> > > > +			dev_pm_qos_hide_flags(dev);
> > > 
> > > NO_POWER_OFF flag is ignored here.  Is it possible for some device (or
> > > corresponding ACPI method) has broken D3cold implementation, so that the
> > > user need a way to disable it?
> > 
> > We are removing the driver here and we haven't exposed the flags, have we?
> 
> IMHO, the flag may be exposed by the bus instead of device driver.
> Because some bus can determine whether D3cold is supported by the
> device.  If the sysfs file for the flag is exposed by the bus, user set
> it and we silently ignore it, will it cause confusing for the user?

OK, put it in a different way: If the entity that calls acpi_dev_pm_attach()
exposes the flags _before_ calling it, then acpi_dev_pm_detach() should not
hide the flags.  Otherwise, it should hide them.  Currently, the only user
of acpi_dev_pm_attach() (that will be the platform bus type) will not expose
the flags before calling that routine, so it is OK for acpi_dev_pm_detach()
to remove them.

Is there anything I'm missing here?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/2] ACPI / PM: Allow attach/detach routines to change device power states

[Huang Ying]

On Mon, 2012-11-26 at 02:16 +0100, Rafael J. Wysocki wrote:
> On Monday, November 26, 2012 09:07:27 AM Huang Ying wrote:
> > On Mon, 2012-11-26 at 02:00 +0100, Rafael J. Wysocki wrote:
> > > On Monday, November 26, 2012 08:43:10 AM Huang Ying wrote:
> > > > On Sun, 2012-11-25 at 15:55 +0100, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > 
> > > > > Make it possible to ask the routines used for adding/removing devices
> > > > > to/from the general ACPI PM domain, acpi_dev_pm_attach() and
> > > > > acpi_dev_pm_detach(), respectively, to change the power states of
> > > > > devices so that they are put into the full-power state automatically
> > > > > by acpi_dev_pm_attach() and into the lowest-power state available
> > > > > automatically by acpi_dev_pm_detach().
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > ---
> > > > >  drivers/acpi/device_pm.c |   28 ++++++++++++++++++++++++----
> > > > >  include/linux/acpi.h     |   11 +++++++----
> > > > >  2 files changed, 31 insertions(+), 8 deletions(-)
> > > > > 
> > > > > Index: linux/drivers/acpi/device_pm.c
> > > > > ===================================================================
> > > > > --- linux.orig/drivers/acpi/device_pm.c
> > > > > +++ linux/drivers/acpi/device_pm.c
> > > > > @@ -599,10 +599,12 @@ static struct dev_pm_domain acpi_general
> > > > >  /**
> > > > >   * acpi_dev_pm_attach - Prepare device for ACPI power management.
> > > > >   * @dev: Device to prepare.
> > > > > + * @power_on: Whether or not to power on the device.
> > > > >   *
> > > > >   * If @dev has a valid ACPI handle that has a valid struct acpi_device object
> > > > >   * attached to it, install a wakeup notification handler for the device and
> > > > > - * add it to the general ACPI PM domain.
> > > > > + * add it to the general ACPI PM domain.  If @power_on is set, the device will
> > > > > + * be put into the ACPI D0 state before the function returns.
> > > > >   *
> > > > >   * This assumes that the @dev's bus type uses generic power management callbacks
> > > > >   * (or doesn't use any power management callbacks at all).
> > > > > @@ -610,7 +612,7 @@ static struct dev_pm_domain acpi_general
> > > > >   * Callers must ensure proper synchronization of this function with power
> > > > >   * management callbacks.
> > > > >   */
> > > > > -int acpi_dev_pm_attach(struct device *dev)
> > > > > +int acpi_dev_pm_attach(struct device *dev, bool power_on)
> > > > >  {
> > > > >  	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > > > >  
> > > > > @@ -622,6 +624,10 @@ int acpi_dev_pm_attach(struct device *de
> > > > >  
> > > > >  	acpi_add_pm_notifier(adev, acpi_wakeup_device, dev);
> > > > >  	dev->pm_domain = &acpi_general_pm_domain;
> > > > > +	if (power_on) {
> > > > > +		acpi_dev_pm_full_power(adev);
> > > > > +		__acpi_device_run_wake(adev, false);
> > > > > +	}
> > > > >  	return 0;
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > > > @@ -629,20 +635,34 @@ EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> > > > >  /**
> > > > >   * acpi_dev_pm_detach - Remove ACPI power management from the device.
> > > > >   * @dev: Device to take care of.
> > > > > + * @power_off: Whether or not to try to remove power from the device.
> > > > >   *
> > > > >   * Remove the device from the general ACPI PM domain and remove its wakeup
> > > > > - * notifier.
> > > > > + * notifier.  If @power_off is set, additionally remove power from the device if
> > > > > + * possible.
> > > > >   *
> > > > >   * Callers must ensure proper synchronization of this function with power
> > > > >   * management callbacks.
> > > > >   */
> > > > > -void acpi_dev_pm_detach(struct device *dev)
> > > > > +void acpi_dev_pm_detach(struct device *dev, bool power_off)
> > > > >  {
> > > > >  	struct acpi_device *adev = acpi_dev_pm_get_node(dev);
> > > > >  
> > > > >  	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> > > > >  		dev->pm_domain = NULL;
> > > > >  		acpi_remove_pm_notifier(adev, acpi_wakeup_device);
> > > > > +		if (power_off) {
> > > > > +			/*
> > > > > +			 * If the device's PM QoS resume latency limit or flags
> > > > > +			 * have been exposed to user space, they have to be
> > > > > +			 * hidden at this point, so that they don't affect the
> > > > > +			 * choice of the low-power state to put the device into.
> > > > > +			 */
> > > > > +			dev_pm_qos_hide_latency_limit(dev);
> > > > > +			dev_pm_qos_hide_flags(dev);
> > > > 
> > > > NO_POWER_OFF flag is ignored here.  Is it possible for some device (or
> > > > corresponding ACPI method) has broken D3cold implementation, so that the
> > > > user need a way to disable it?
> > > 
> > > We are removing the driver here and we haven't exposed the flags, have we?
> > 
> > IMHO, the flag may be exposed by the bus instead of device driver.
> > Because some bus can determine whether D3cold is supported by the
> > device.  If the sysfs file for the flag is exposed by the bus, user set
> > it and we silently ignore it, will it cause confusing for the user?
> 
> OK, put it in a different way: If the entity that calls acpi_dev_pm_attach()
> exposes the flags _before_ calling it, then acpi_dev_pm_detach() should not
> hide the flags.  Otherwise, it should hide them.  Currently, the only user
> of acpi_dev_pm_attach() (that will be the platform bus type) will not expose
> the flags before calling that routine, so it is OK for acpi_dev_pm_detach()
> to remove them.

Yes.  You are right.  Thanks for clarification.

Best Regards,
Huang Ying

[PATCH 2/2] platform / ACPI: Attach/detach ACPI PM during probe/remove/shutdown

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Drivers usually expect that the devices they are supposed to handle
will be operational when their .probe() routines are called, but that
need not be the case on some ACPI-based systems with ACPI-based
device enumeration where the BIOSes don't put devices into D0 by
default.  To work around this problem it is sufficient to change
bus type .probe() routines to ensure that devices will be powered
on before the drivers' .probe() routines run (and their .remove()
and .shutdown() routines accordingly).

Modify platform_drv_probe() to run acpi_dev_pm_attach() for devices
whose ACPI handles are present, so that ACPI power management is used
to change their power states and change their power states to D0
before driver probing.  Analogously, modify platform_drv_remove() and
platform_drv_shutdown() to call acpi_dev_pm_detach() for those
devices, so that they are not subject to ACPI PM any more.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/platform.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Index: linux/drivers/base/platform.c
===================================================================
--- linux.orig/drivers/base/platform.c
+++ linux/drivers/base/platform.c
@@ -484,8 +484,16 @@ static int platform_drv_probe(struct dev
 {
 	struct platform_driver *drv = to_platform_driver(_dev->driver);
 	struct platform_device *dev = to_platform_device(_dev);
+	int ret;
 
-	return drv->probe(dev);
+	if (ACPI_HANDLE(_dev))
+		acpi_dev_pm_attach(_dev, true);
+
+	ret = drv->probe(dev);
+	if (ret && ACPI_HANDLE(_dev))
+		acpi_dev_pm_detach(_dev, true);
+
+	return ret;
 }
 
 static int platform_drv_probe_fail(struct device *_dev)
@@ -497,8 +505,13 @@ static int platform_drv_remove(struct de
 {
 	struct platform_driver *drv = to_platform_driver(_dev->driver);
 	struct platform_device *dev = to_platform_device(_dev);
+	int ret;
+
+	ret = drv->remove(dev);
+	if (ACPI_HANDLE(_dev))
+		acpi_dev_pm_detach(_dev, true);
 
-	return drv->remove(dev);
+	return ret;
 }
 
 static void platform_drv_shutdown(struct device *_dev)
@@ -507,6 +520,8 @@ static void platform_drv_shutdown(struct
 	struct platform_device *dev = to_platform_device(_dev);
 
 	drv->shutdown(dev);
+	if (ACPI_HANDLE(_dev))
+		acpi_dev_pm_detach(_dev, true);
 }
 
 /**

Re: [PATCH 2/2] platform / ACPI: Attach/detach ACPI PM during probe/remove/shutdown

[Greg Kroah-Hartman]

On Sun, Nov 25, 2012 at 03:58:14PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Drivers usually expect that the devices they are supposed to handle
> will be operational when their .probe() routines are called, but that
> need not be the case on some ACPI-based systems with ACPI-based
> device enumeration where the BIOSes don't put devices into D0 by
> default.  To work around this problem it is sufficient to change
> bus type .probe() routines to ensure that devices will be powered
> on before the drivers' .probe() routines run (and their .remove()
> and .shutdown() routines accordingly).
> 
> Modify platform_drv_probe() to run acpi_dev_pm_attach() for devices
> whose ACPI handles are present, so that ACPI power management is used
> to change their power states and change their power states to D0
> before driver probing.  Analogously, modify platform_drv_remove() and
> platform_drv_shutdown() to call acpi_dev_pm_detach() for those
> devices, so that they are not subject to ACPI PM any more.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/2] platform / ACPI: Attach/detach ACPI PM during probe/remove/shutdown

[Rafael J. Wysocki]

On Sunday, November 25, 2012 10:42:25 AM Greg Kroah-Hartman wrote:
> On Sun, Nov 25, 2012 at 03:58:14PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Drivers usually expect that the devices they are supposed to handle
> > will be operational when their .probe() routines are called, but that
> > need not be the case on some ACPI-based systems with ACPI-based
> > device enumeration where the BIOSes don't put devices into D0 by
> > default.  To work around this problem it is sufficient to change
> > bus type .probe() routines to ensure that devices will be powered
> > on before the drivers' .probe() routines run (and their .remove()
> > and .shutdown() routines accordingly).
> > 
> > Modify platform_drv_probe() to run acpi_dev_pm_attach() for devices
> > whose ACPI handles are present, so that ACPI power management is used
> > to change their power states and change their power states to D0
> > before driver probing.  Analogously, modify platform_drv_remove() and
> > platform_drv_shutdown() to call acpi_dev_pm_detach() for those
> > devices, so that they are not subject to ACPI PM any more.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 2/2] platform / ACPI: Attach/detach ACPI PM during probe/remove/shutdown

[Zheng, Lv]

It looks cleanest unless more abstractions are introduced for other power management providers.

Best regards/Lv Zheng

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Sunday, November 25, 2012 10:58 PM
> To: LKML
> Cc: Greg Kroah-Hartman; Linux PM list; ACPI Devel Maling List; Zhang, Rui;
> Svahn, Kai; Mika Westerberg; Huang, Ying; Lan, Tianyu; Zheng, Lv; Lu, Aaron;
> Grant Likely
> Subject: [PATCH 2/2] platform / ACPI: Attach/detach ACPI PM during
> probe/remove/shutdown
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Drivers usually expect that the devices they are supposed to handle will be
> operational when their .probe() routines are called, but that need not be the
> case on some ACPI-based systems with ACPI-based device enumeration where
> the BIOSes don't put devices into D0 by default.  To work around this problem
> it is sufficient to change bus type .probe() routines to ensure that devices will
> be powered on before the drivers' .probe() routines run (and their .remove()
> and .shutdown() routines accordingly).
> 
> Modify platform_drv_probe() to run acpi_dev_pm_attach() for devices whose
> ACPI handles are present, so that ACPI power management is used to change
> their power states and change their power states to D0 before driver probing.
> Analogously, modify platform_drv_remove() and
> platform_drv_shutdown() to call acpi_dev_pm_detach() for those devices, so
> that they are not subject to ACPI PM any more.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/platform.c |   19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> Index: linux/drivers/base/platform.c
> ================================================================
> ===
> --- linux.orig/drivers/base/platform.c
> +++ linux/drivers/base/platform.c
> @@ -484,8 +484,16 @@ static int platform_drv_probe(struct dev  {
>  	struct platform_driver *drv = to_platform_driver(_dev->driver);
>  	struct platform_device *dev = to_platform_device(_dev);
> +	int ret;
> 
> -	return drv->probe(dev);
> +	if (ACPI_HANDLE(_dev))
> +		acpi_dev_pm_attach(_dev, true);
> +
> +	ret = drv->probe(dev);
> +	if (ret && ACPI_HANDLE(_dev))
> +		acpi_dev_pm_detach(_dev, true);
> +
> +	return ret;
>  }
> 
>  static int platform_drv_probe_fail(struct device *_dev) @@ -497,8 +505,13
> @@ static int platform_drv_remove(struct de  {
>  	struct platform_driver *drv = to_platform_driver(_dev->driver);
>  	struct platform_device *dev = to_platform_device(_dev);
> +	int ret;
> +
> +	ret = drv->remove(dev);
> +	if (ACPI_HANDLE(_dev))
> +		acpi_dev_pm_detach(_dev, true);
> 
> -	return drv->remove(dev);
> +	return ret;
>  }
> 
>  static void platform_drv_shutdown(struct device *_dev) @@ -507,6 +520,8
> @@ static void platform_drv_shutdown(struct
>  	struct platform_device *dev = to_platform_device(_dev);
> 
>  	drv->shutdown(dev);
> +	if (ACPI_HANDLE(_dev))
> +		acpi_dev_pm_detach(_dev, true);
>  }
> 
>  /**

Re: [PATCH 0/2] ACPI / platform: Put devices enumerated via ACPI into D0 before probing drivers

[Mika Westerberg]

On Sun, Nov 25, 2012 at 03:54:14PM +0100, Rafael J. Wysocki wrote:
> The following two patches are meant to work around a problem where some
> devices may be in the ACPI D3cold power state (power off) before their
> drivers' .probe() routines are called, but those routines assume devices
> to be operational.  They also allow ACPI PM to be used going forward with
> the devices in question.

We have such machines here. I've tested the series on one of them and it
solves the problem - drivers can now access device registers right from the
start.

> [1/2] - Allow ACPI PM attach/detach routines to change device power states.
> [2/2] - Call the ACPI PM attach/detach routines in the platform probe and
>         remove/shutdown callbacks (and change power states of devices as
>         necessary).

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>