[PATCH 0/3] ACPI / PM: Device PM cleanups

[PATCH 0/3] ACPI / PM: Device PM cleanups

[Rafael J. Wysocki]

Hi All,

The following 3 patches clean up ACPI device PM a bit:

[1/3] Fix acpi_device_set_power() to avoid printing useless messages for
      devices that aren't power manageable.

[2/3] Fix messages in acpi_device_set_power() to always contain a device
      name.

[3/3] Replace ACPI_STATE_D3 witn ACPI_STATE_D3_COLD everywhere in the tree
      (leave definition as part of ACPICA).

Thanks,
Rafael


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

[PATCH 1/3] ACPI / PM: Only set power states of devices that are power manageable

[Rafael J. Wysocki]

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

Make acpi_device_set_power() check if the given device is power
manageable before checking if the given power state is valid for that
device.  Otherwise it will print that "Device does not support" that
power state into the kernel log, which may not make sense for some
power states (D0 and D3cold are supported by all devices by
definition).

Tested-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -159,7 +159,8 @@ int acpi_device_set_power(struct acpi_de
 	int result = 0;
 	bool cut_power = false;
 
-	if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
+	if (!device || !device->flags.power_manageable
+	    || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
 		return -EINVAL;
 
 	/* Make sure this is a valid target state */

[PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Rafael J. Wysocki]

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

Modify acpi_device_set_power() so that diagnostic messages printed by
it to the kernel log always contain the name of the device concerned
to make it possible to identify the device that triggered the message
if need be.

Also replace printk(KERN_WARNING ) with dev_warn() everywhere in that
function.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
 	/* Make sure this is a valid target state */
 
 	if (state == device->power.state) {
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at %s\n",
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] already in %s\n",
+				  device->pnp.bus_id,
 				  acpi_power_state_string(state)));
 		return 0;
 	}
 
 	if (!device->power.states[state].flags.valid) {
-		printk(KERN_WARNING PREFIX "Device does not support %s\n",
-		       acpi_power_state_string(state));
+		dev_warn(&device->dev, "Power state %s not supported\n",
+			 acpi_power_state_string(state));
 		return -ENODEV;
 	}
 	if (device->parent && (state < device->parent->power.state)) {
-		printk(KERN_WARNING PREFIX
-			      "Cannot set device to a higher-powered"
-			      " state than parent\n");
+		dev_warn(&device->dev, "Cannot transition to a higher-powered "
+			 "state than parent\n");
 		return -ENODEV;
 	}
 
@@ -192,8 +192,8 @@ int acpi_device_set_power(struct acpi_de
 
 	if (state < device->power.state && state != ACPI_STATE_D0
 	    && device->power.state >= ACPI_STATE_D3_HOT) {
-		printk(KERN_WARNING PREFIX
-			"Cannot transition to non-D0 state from D3\n");
+		dev_warn(&device->dev,
+			 "Cannot transition to non-D0 state from D3\n");
 		return -ENODEV;
 	}
 
@@ -220,10 +220,8 @@ int acpi_device_set_power(struct acpi_de
 
  end:
 	if (result) {
-		printk(KERN_WARNING PREFIX
-			      "Device [%s] failed to transition to %s\n",
-			      device->pnp.bus_id,
-			      acpi_power_state_string(state));
+		dev_warn(&device->dev, "Failed to change power state to %s\n",
+			 acpi_power_state_string(state));
 	} else {
 		device->power.state = state;
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,

[PATCH 3/3] ACPI / PM: Use ACPI_STATE_D3_COLD instead of ACPI_STATE_D3 everywhere

[Rafael J. Wysocki]

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

There are several places in the tree where ACPI_STATE_D3 is used
instead of ACPI_STATE_D3_COLD which should be used instead for
clarity.  Modify them all to use ACPI_STATE_D3_COLD as appropriate.

[The definition of ACPI_STATE_D3 itself cannot go away at this point
 as it is part of ACPICA.]

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/fan.c         |    4 ++--
 drivers/acpi/power.c       |    2 +-
 drivers/acpi/scan.c        |    4 ++--
 drivers/ata/libata-acpi.c  |    4 ++--
 drivers/ide/ide-acpi.c     |    5 +++--
 drivers/pnp/pnpacpi/core.c |    6 +++---
 include/acpi/acpi_bus.h    |    3 ++-
 7 files changed, 15 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/fan.c
===================================================================
--- linux-pm.orig/drivers/acpi/fan.c
+++ linux-pm/drivers/acpi/fan.c
@@ -93,7 +93,7 @@ static int fan_get_cur_state(struct ther
 	if (result)
 		return result;
 
-	*state = (acpi_state == ACPI_STATE_D3 ? 0 :
+	*state = (acpi_state == ACPI_STATE_D3_COLD ? 0 :
 		 (acpi_state == ACPI_STATE_D0 ? 1 : -1));
 	return 0;
 }
@@ -108,7 +108,7 @@ fan_set_cur_state(struct thermal_cooling
 		return -EINVAL;
 
 	result = acpi_bus_set_power(device->handle,
-				state ? ACPI_STATE_D0 : ACPI_STATE_D3);
+				state ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD);
 
 	return result;
 }
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -784,7 +784,7 @@ int acpi_power_get_inferred_state(struct
 		}
 	}
 
-	*state = ACPI_STATE_D3;
+	*state = ACPI_STATE_D3_COLD;
 	return 0;
 }
 
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1409,8 +1409,8 @@ static void acpi_bus_get_power_flags(str
 	/* Set defaults for D0 and D3 states (always valid) */
 	device->power.states[ACPI_STATE_D0].flags.valid = 1;
 	device->power.states[ACPI_STATE_D0].power = 100;
-	device->power.states[ACPI_STATE_D3].flags.valid = 1;
-	device->power.states[ACPI_STATE_D3].power = 0;
+	device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
+	device->power.states[ACPI_STATE_D3_COLD].power = 0;
 
 	/* Set D3cold's explicit_set flag if _PS3 exists. */
 	if (device->power.states[ACPI_STATE_D3_HOT].flags.explicit_set)
Index: linux-pm/drivers/ata/libata-acpi.c
===================================================================
--- linux-pm.orig/drivers/ata/libata-acpi.c
+++ linux-pm/drivers/ata/libata-acpi.c
@@ -947,11 +947,11 @@ static void pata_acpi_set_state(struct a
 			continue;
 
 		acpi_bus_set_power(dev_handle, state.event & PM_EVENT_RESUME ?
-						ACPI_STATE_D0 : ACPI_STATE_D3);
+					ACPI_STATE_D0 : ACPI_STATE_D3_COLD);
 	}
 
 	if (!(state.event & PM_EVENT_RESUME))
-		acpi_bus_set_power(port_handle, ACPI_STATE_D3);
+		acpi_bus_set_power(port_handle, ACPI_STATE_D3_COLD);
 }
 
 /**
Index: linux-pm/drivers/ide/ide-acpi.c
===================================================================
--- linux-pm.orig/drivers/ide/ide-acpi.c
+++ linux-pm/drivers/ide/ide-acpi.c
@@ -520,11 +520,12 @@ void ide_acpi_set_state(ide_hwif_t *hwif
 	ide_port_for_each_present_dev(i, drive, hwif) {
 		if (drive->acpidata->obj_handle)
 			acpi_bus_set_power(drive->acpidata->obj_handle,
-					   on ? ACPI_STATE_D0 : ACPI_STATE_D3);
+				on ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD);
 	}
 
 	if (!on)
-		acpi_bus_set_power(hwif->acpidata->obj_handle, ACPI_STATE_D3);
+		acpi_bus_set_power(hwif->acpidata->obj_handle,
+				   ACPI_STATE_D3_COLD);
 }
 
 /**
Index: linux-pm/drivers/pnp/pnpacpi/core.c
===================================================================
--- linux-pm.orig/drivers/pnp/pnpacpi/core.c
+++ linux-pm/drivers/pnp/pnpacpi/core.c
@@ -131,7 +131,7 @@ static int pnpacpi_disable_resources(str
 	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
 	ret = 0;
 	if (acpi_bus_power_manageable(handle))
-		acpi_bus_set_power(handle, ACPI_STATE_D3);
+		acpi_bus_set_power(handle, ACPI_STATE_D3_COLD);
 		/* continue even if acpi_bus_set_power() fails */
 	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DIS", NULL, NULL)))
 		ret = -ENODEV;
@@ -174,10 +174,10 @@ static int pnpacpi_suspend(struct pnp_de
 
 	if (acpi_bus_power_manageable(handle)) {
 		int power_state = acpi_pm_device_sleep_state(&dev->dev, NULL,
-							     ACPI_STATE_D3);
+							ACPI_STATE_D3_COLD);
 		if (power_state < 0)
 			power_state = (state.event == PM_EVENT_ON) ?
-					ACPI_STATE_D0 : ACPI_STATE_D3;
+					ACPI_STATE_D0 : ACPI_STATE_D3_COLD;
 
 		/*
 		 * acpi_bus_set_power() often fails (keyboard port can't be
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -477,7 +477,8 @@ static inline int acpi_pm_device_sleep_s
 	if (p)
 		*p = ACPI_STATE_D0;
 
-	return (m >= ACPI_STATE_D0 && m <= ACPI_STATE_D3) ? m : ACPI_STATE_D0;
+	return (m >= ACPI_STATE_D0 && m <= ACPI_STATE_D3_COLD) ?
+		m : ACPI_STATE_D0;
 }
 static inline void acpi_dev_pm_add_dependent(acpi_handle handle,
 					     struct device *depdev) {}

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Aaron Lu]

On 07/27/2013 09:11 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify acpi_device_set_power() so that diagnostic messages printed by
> it to the kernel log always contain the name of the device concerned
> to make it possible to identify the device that triggered the message
> if need be.
> 
> Also replace printk(KERN_WARNING ) with dev_warn() everywhere in that
> function.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/device_pm.c |   22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
>  	/* Make sure this is a valid target state */
>  
>  	if (state == device->power.state) {
> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at %s\n",
> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] already in %s\n",
> +				  device->pnp.bus_id,
>  				  acpi_power_state_string(state)));
>  		return 0;
>  	}
>  
>  	if (!device->power.states[state].flags.valid) {
> -		printk(KERN_WARNING PREFIX "Device does not support %s\n",
> -		       acpi_power_state_string(state));
> +		dev_warn(&device->dev, "Power state %s not supported\n",
> +			 acpi_power_state_string(state));
>  		return -ENODEV;
>  	}
>  	if (device->parent && (state < device->parent->power.state)) {
> -		printk(KERN_WARNING PREFIX
> -			      "Cannot set device to a higher-powered"
> -			      " state than parent\n");
> +		dev_warn(&device->dev, "Cannot transition to a higher-powered "
> +			 "state than parent\n");

I think the state information would also be useful here:

		dev_warn(&device->dev, "Cannot transition to a higher-powereed "
		         "state %d than paeren's state %d\n", state,
			 device->parent->power.state);

Thanks,
Aaron

>  		return -ENODEV;
>  	}
>  
> @@ -192,8 +192,8 @@ int acpi_device_set_power(struct acpi_de
>  
>  	if (state < device->power.state && state != ACPI_STATE_D0
>  	    && device->power.state >= ACPI_STATE_D3_HOT) {
> -		printk(KERN_WARNING PREFIX
> -			"Cannot transition to non-D0 state from D3\n");
> +		dev_warn(&device->dev,
> +			 "Cannot transition to non-D0 state from D3\n");
>  		return -ENODEV;
>  	}
>  
> @@ -220,10 +220,8 @@ int acpi_device_set_power(struct acpi_de
>  
>   end:
>  	if (result) {
> -		printk(KERN_WARNING PREFIX
> -			      "Device [%s] failed to transition to %s\n",
> -			      device->pnp.bus_id,
> -			      acpi_power_state_string(state));
> +		dev_warn(&device->dev, "Failed to change power state to %s\n",
> +			 acpi_power_state_string(state));
>  	} else {
>  		device->power.state = state;
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Lan Tianyu]

2013/7/27 Rafael J. Wysocki <rjw@sisk.pl>:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Modify acpi_device_set_power() so that diagnostic messages printed by
> it to the kernel log always contain the name of the device concerned
> to make it possible to identify the device that triggered the message
> if need be.
>
> Also replace printk(KERN_WARNING ) with dev_warn() everywhere in that
> function.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/device_pm.c |   22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
>         /* Make sure this is a valid target state */
>
>         if (state == device->power.state) {
> -               ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at %s\n",
> +               ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] already in %s\n",
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
Missing "is" or it should be omitted?

> +                                 device->pnp.bus_id,
>                                   acpi_power_state_string(state)));
>                 return 0;
>         }
>
>         if (!device->power.states[state].flags.valid) {
> -               printk(KERN_WARNING PREFIX "Device does not support %s\n",
> -                      acpi_power_state_string(state));
> +               dev_warn(&device->dev, "Power state %s not supported\n",
> +                        acpi_power_state_string(state));
>                 return -ENODEV;
>         }
>         if (device->parent && (state < device->parent->power.state)) {
> -               printk(KERN_WARNING PREFIX
> -                             "Cannot set device to a higher-powered"
> -                             " state than parent\n");
> +               dev_warn(&device->dev, "Cannot transition to a higher-powered "
> +                        "state than parent\n");
>                 return -ENODEV;
>         }
>
> @@ -192,8 +192,8 @@ int acpi_device_set_power(struct acpi_de
>
>         if (state < device->power.state && state != ACPI_STATE_D0
>             && device->power.state >= ACPI_STATE_D3_HOT) {
> -               printk(KERN_WARNING PREFIX
> -                       "Cannot transition to non-D0 state from D3\n");
> +               dev_warn(&device->dev,
> +                        "Cannot transition to non-D0 state from D3\n");
>                 return -ENODEV;
>         }
>
> @@ -220,10 +220,8 @@ int acpi_device_set_power(struct acpi_de
>
>   end:
>         if (result) {
> -               printk(KERN_WARNING PREFIX
> -                             "Device [%s] failed to transition to %s\n",
> -                             device->pnp.bus_id,
> -                             acpi_power_state_string(state));
> +               dev_warn(&device->dev, "Failed to change power state to %s\n",
> +                        acpi_power_state_string(state));
>         } else {
>                 device->power.state = state;
>                 ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best regards
Tianyu Lan

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Joe Perches]

On Mon, 2013-07-29 at 11:06 +0800, Lan Tianyu wrote:
> 2013/7/27 Rafael J. Wysocki <rjw@sisk.pl>:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[]
> >  drivers/acpi/device_pm.c |   22 ++++++++++------------
[]
> > @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
[]
> >         if (device->parent && (state < device->parent->power.state)) {
> > -               printk(KERN_WARNING PREFIX
> > -                             "Cannot set device to a higher-powered"
> > -                             " state than parent\n");
> > +               dev_warn(&device->dev, "Cannot transition to a higher-powered "
> > +                        "state than parent\n");

coalesce format please.

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Rafael J. Wysocki]

On Monday, July 29, 2013 11:06:36 AM Lan Tianyu wrote:
> 2013/7/27 Rafael J. Wysocki <rjw@sisk.pl>:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Modify acpi_device_set_power() so that diagnostic messages printed by
> > it to the kernel log always contain the name of the device concerned
> > to make it possible to identify the device that triggered the message
> > if need be.
> >
> > Also replace printk(KERN_WARNING ) with dev_warn() everywhere in that
> > function.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/device_pm.c |   22 ++++++++++------------
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/device_pm.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/device_pm.c
> > +++ linux-pm/drivers/acpi/device_pm.c
> > @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
> >         /* Make sure this is a valid target state */
> >
> >         if (state == device->power.state) {
> > -               ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at %s\n",
> > +               ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] already in %s\n",
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
> Missing "is" or it should be omitted?

The "is" is not necessary here (in my opinion).

Kind of in analogy with "This has been done already" and "Already done".

Thanks,
Rafael


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

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Sergei Shtylyov]

Hello.

On 29-07-2013 16:17, Rafael J. Wysocki wrote:

[...]

>>>> @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
>> []
>>>>          if (device->parent && (state < device->parent->power.state)) {
>>>> -               printk(KERN_WARNING PREFIX
>>>> -                             "Cannot set device to a higher-powered"
>>>> -                             " state than parent\n");
>>>> +               dev_warn(&device->dev, "Cannot transition to a higher-powered "
>>>> +                        "state than parent\n");

>> coalesce format please.

> I can, but then it'll cross the 80 columns boundary.

   It's not a problem with checkpatch.pl anymore. Contrariwise, it whines 
about the broken up string literals, AFAIR.

> Thanks,
> Rafael

WBR, Sergei

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Rafael J. Wysocki]

On Sunday, July 28, 2013 08:11:53 PM Joe Perches wrote:
> On Mon, 2013-07-29 at 11:06 +0800, Lan Tianyu wrote:
> > 2013/7/27 Rafael J. Wysocki <rjw@sisk.pl>:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> []
> > >  drivers/acpi/device_pm.c |   22 ++++++++++------------
> []
> > > @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
> []
> > >         if (device->parent && (state < device->parent->power.state)) {
> > > -               printk(KERN_WARNING PREFIX
> > > -                             "Cannot set device to a higher-powered"
> > > -                             " state than parent\n");
> > > +               dev_warn(&device->dev, "Cannot transition to a higher-powered "
> > > +                        "state than parent\n");
> 
> coalesce format please.

I can, but then it'll cross the 80 columns boundary.

Thanks,
Rafael

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Rafael J. Wysocki]

On Monday, July 29, 2013 10:29:36 AM Aaron Lu wrote:
> On 07/27/2013 09:11 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Modify acpi_device_set_power() so that diagnostic messages printed by
> > it to the kernel log always contain the name of the device concerned
> > to make it possible to identify the device that triggered the message
> > if need be.
> > 
> > Also replace printk(KERN_WARNING ) with dev_warn() everywhere in that
> > function.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/device_pm.c |   22 ++++++++++------------
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> > 
> > Index: linux-pm/drivers/acpi/device_pm.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/device_pm.c
> > +++ linux-pm/drivers/acpi/device_pm.c
> > @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
> >  	/* Make sure this is a valid target state */
> >  
> >  	if (state == device->power.state) {
> > -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at %s\n",
> > +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] already in %s\n",
> > +				  device->pnp.bus_id,
> >  				  acpi_power_state_string(state)));
> >  		return 0;
> >  	}
> >  
> >  	if (!device->power.states[state].flags.valid) {
> > -		printk(KERN_WARNING PREFIX "Device does not support %s\n",
> > -		       acpi_power_state_string(state));
> > +		dev_warn(&device->dev, "Power state %s not supported\n",
> > +			 acpi_power_state_string(state));
> >  		return -ENODEV;
> >  	}
> >  	if (device->parent && (state < device->parent->power.state)) {
> > -		printk(KERN_WARNING PREFIX
> > -			      "Cannot set device to a higher-powered"
> > -			      " state than parent\n");
> > +		dev_warn(&device->dev, "Cannot transition to a higher-powered "
> > +			 "state than parent\n");
> 
> I think the state information would also be useful here:
> 
> 		dev_warn(&device->dev, "Cannot transition to a higher-powereed "
> 		         "state %d than paeren's state %d\n", state,
> 			 device->parent->power.state);

This is not the scope of this patch, please send another one on top of it.

Thanks,
Rafael


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

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Rafael J. Wysocki]

On Monday, July 29, 2013 04:16:31 PM Sergei Shtylyov wrote:
> Hello.
> 
> On 29-07-2013 16:17, Rafael J. Wysocki wrote:
> 
> [...]
> 
> >>>> @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
> >> []
> >>>>          if (device->parent && (state < device->parent->power.state)) {
> >>>> -               printk(KERN_WARNING PREFIX
> >>>> -                             "Cannot set device to a higher-powered"
> >>>> -                             " state than parent\n");
> >>>> +               dev_warn(&device->dev, "Cannot transition to a higher-powered "
> >>>> +                        "state than parent\n");
> 
> >> coalesce format please.
> 
> > I can, but then it'll cross the 80 columns boundary.
> 
>    It's not a problem with checkpatch.pl anymore. Contrariwise, it whines 
> about the broken up string literals, AFAIR.

Oh, at last.

Updated patch follows.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / PM: Make messages in acpi_device_set_power() print device names

Modify acpi_device_set_power() so that diagnostic messages printed by
it to the kernel log always contain the name of the device concerned
to make it possible to identify the device that triggered the message
if need be.

Also replace printk(KERN_WARNING ) with dev_warn() everywhere in that
function.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
 	/* Make sure this is a valid target state */
 
 	if (state == device->power.state) {
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at %s\n",
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] already in %s\n",
+				  device->pnp.bus_id,
 				  acpi_power_state_string(state)));
 		return 0;
 	}
 
 	if (!device->power.states[state].flags.valid) {
-		printk(KERN_WARNING PREFIX "Device does not support %s\n",
-		       acpi_power_state_string(state));
+		dev_warn(&device->dev, "Power state %s not supported\n",
+			 acpi_power_state_string(state));
 		return -ENODEV;
 	}
 	if (device->parent && (state < device->parent->power.state)) {
-		printk(KERN_WARNING PREFIX
-			      "Cannot set device to a higher-powered"
-			      " state than parent\n");
+		dev_warn(&device->dev,
+			 "Cannot transition to a higher-powered state than parent\n");
 		return -ENODEV;
 	}
 
@@ -192,8 +192,8 @@ int acpi_device_set_power(struct acpi_de
 
 	if (state < device->power.state && state != ACPI_STATE_D0
 	    && device->power.state >= ACPI_STATE_D3_HOT) {
-		printk(KERN_WARNING PREFIX
-			"Cannot transition to non-D0 state from D3\n");
+		dev_warn(&device->dev,
+			 "Cannot transition to non-D0 state from D3\n");
 		return -ENODEV;
 	}
 
@@ -220,10 +220,8 @@ int acpi_device_set_power(struct acpi_de
 
  end:
 	if (result) {
-		printk(KERN_WARNING PREFIX
-			      "Device [%s] failed to transition to %s\n",
-			      device->pnp.bus_id,
-			      acpi_power_state_string(state));
+		dev_warn(&device->dev, "Failed to change power state to %s\n",
+			 acpi_power_state_string(state));
 	} else {
 		device->power.state = state;
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,

Re: [PATCH 1/3] ACPI / PM: Only set power states of devices that are power manageable

[Aaron Lu]

On 07/27/2013 09:10 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make acpi_device_set_power() check if the given device is power
> manageable before checking if the given power state is valid for that
> device.  Otherwise it will print that "Device does not support" that
> power state into the kernel log, which may not make sense for some
> power states (D0 and D3cold are supported by all devices by
> definition).

It will not print "Device does not support" that power state if that
power state is D0 or D3cold since we have unconditionally set those two
power state's valid flag.

OTOH, what value should we return for a device node that is not power
manageable in acpi_device_set_power when the target state is D0 or D3
cold? The old behavior is to return 0, meanning success without taking
any actual action.

In acpi_bus_set_power, if the device is not power manageable, we will
return -ENODEV; in acpi_dev_pm_full/low_power, we will return 0 as in
the original acpi_device_set_power. So return -EINVAL here is correct?

-Aaron

> 
> Tested-by: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/device_pm.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -159,7 +159,8 @@ int acpi_device_set_power(struct acpi_de
>  	int result = 0;
>  	bool cut_power = false;
>  
> -	if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
> +	if (!device || !device->flags.power_manageable
> +	    || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
>  		return -EINVAL;
>  
>  	/* Make sure this is a valid target state */
> 

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Aaron Lu]

On 07/29/2013 09:36 PM, Rafael J. Wysocki wrote:
> On Monday, July 29, 2013 04:16:31 PM Sergei Shtylyov wrote:
>> Hello.
>>
>> On 29-07-2013 16:17, Rafael J. Wysocki wrote:
>>
>> [...]
>>
>>>>>> @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
>>>> []
>>>>>>          if (device->parent && (state < device->parent->power.state)) {
>>>>>> -               printk(KERN_WARNING PREFIX
>>>>>> -                             "Cannot set device to a higher-powered"
>>>>>> -                             " state than parent\n");
>>>>>> +               dev_warn(&device->dev, "Cannot transition to a higher-powered "
>>>>>> +                        "state than parent\n");
>>
>>>> coalesce format please.
>>
>>> I can, but then it'll cross the 80 columns boundary.
>>
>>    It's not a problem with checkpatch.pl anymore. Contrariwise, it whines 
>> about the broken up string literals, AFAIR.
> 
> Oh, at last.
> 
> Updated patch follows.
> 
> Thanks,
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: ACPI / PM: Make messages in acpi_device_set_power() print device names
> 
> Modify acpi_device_set_power() so that diagnostic messages printed by
> it to the kernel log always contain the name of the device concerned
> to make it possible to identify the device that triggered the message
> if need be.
> 
> Also replace printk(KERN_WARNING ) with dev_warn() everywhere in that
> function.

This makes the log much more informative, thanks!

> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Aaron Lu <aaron.lu@intel.com>

And I'll add the state information in another patch on top of this one
later.

-Aaron

> ---
>  drivers/acpi/device_pm.c |   22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
>  	/* Make sure this is a valid target state */
>  
>  	if (state == device->power.state) {
> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at %s\n",
> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] already in %s\n",
> +				  device->pnp.bus_id,
>  				  acpi_power_state_string(state)));
>  		return 0;
>  	}
>  
>  	if (!device->power.states[state].flags.valid) {
> -		printk(KERN_WARNING PREFIX "Device does not support %s\n",
> -		       acpi_power_state_string(state));
> +		dev_warn(&device->dev, "Power state %s not supported\n",
> +			 acpi_power_state_string(state));
>  		return -ENODEV;
>  	}
>  	if (device->parent && (state < device->parent->power.state)) {
> -		printk(KERN_WARNING PREFIX
> -			      "Cannot set device to a higher-powered"
> -			      " state than parent\n");
> +		dev_warn(&device->dev,
> +			 "Cannot transition to a higher-powered state than parent\n");
>  		return -ENODEV;
>  	}
>  
> @@ -192,8 +192,8 @@ int acpi_device_set_power(struct acpi_de
>  
>  	if (state < device->power.state && state != ACPI_STATE_D0
>  	    && device->power.state >= ACPI_STATE_D3_HOT) {
> -		printk(KERN_WARNING PREFIX
> -			"Cannot transition to non-D0 state from D3\n");
> +		dev_warn(&device->dev,
> +			 "Cannot transition to non-D0 state from D3\n");
>  		return -ENODEV;
>  	}
>  
> @@ -220,10 +220,8 @@ int acpi_device_set_power(struct acpi_de
>  
>   end:
>  	if (result) {
> -		printk(KERN_WARNING PREFIX
> -			      "Device [%s] failed to transition to %s\n",
> -			      device->pnp.bus_id,
> -			      acpi_power_state_string(state));
> +		dev_warn(&device->dev, "Failed to change power state to %s\n",
> +			 acpi_power_state_string(state));
>  	} else {
>  		device->power.state = state;
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> 

Re: [PATCH 3/3] ACPI / PM: Use ACPI_STATE_D3_COLD instead of ACPI_STATE_D3 everywhere

[Aaron Lu]

On 07/27/2013 09:14 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> There are several places in the tree where ACPI_STATE_D3 is used
> instead of ACPI_STATE_D3_COLD which should be used instead for
> clarity.  Modify them all to use ACPI_STATE_D3_COLD as appropriate.
> 
> [The definition of ACPI_STATE_D3 itself cannot go away at this point
>  as it is part of ACPICA.]
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Aaron Lu <aaron.lu@intel.com>

Thanks,
Aaron

> ---
>  drivers/acpi/fan.c         |    4 ++--
>  drivers/acpi/power.c       |    2 +-
>  drivers/acpi/scan.c        |    4 ++--
>  drivers/ata/libata-acpi.c  |    4 ++--
>  drivers/ide/ide-acpi.c     |    5 +++--
>  drivers/pnp/pnpacpi/core.c |    6 +++---
>  include/acpi/acpi_bus.h    |    3 ++-
>  7 files changed, 15 insertions(+), 13 deletions(-)
> 
> Index: linux-pm/drivers/acpi/fan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/fan.c
> +++ linux-pm/drivers/acpi/fan.c
> @@ -93,7 +93,7 @@ static int fan_get_cur_state(struct ther
>  	if (result)
>  		return result;
>  
> -	*state = (acpi_state == ACPI_STATE_D3 ? 0 :
> +	*state = (acpi_state == ACPI_STATE_D3_COLD ? 0 :
>  		 (acpi_state == ACPI_STATE_D0 ? 1 : -1));
>  	return 0;
>  }
> @@ -108,7 +108,7 @@ fan_set_cur_state(struct thermal_cooling
>  		return -EINVAL;
>  
>  	result = acpi_bus_set_power(device->handle,
> -				state ? ACPI_STATE_D0 : ACPI_STATE_D3);
> +				state ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD);
>  
>  	return result;
>  }
> Index: linux-pm/drivers/acpi/power.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/power.c
> +++ linux-pm/drivers/acpi/power.c
> @@ -784,7 +784,7 @@ int acpi_power_get_inferred_state(struct
>  		}
>  	}
>  
> -	*state = ACPI_STATE_D3;
> +	*state = ACPI_STATE_D3_COLD;
>  	return 0;
>  }
>  
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1409,8 +1409,8 @@ static void acpi_bus_get_power_flags(str
>  	/* Set defaults for D0 and D3 states (always valid) */
>  	device->power.states[ACPI_STATE_D0].flags.valid = 1;
>  	device->power.states[ACPI_STATE_D0].power = 100;
> -	device->power.states[ACPI_STATE_D3].flags.valid = 1;
> -	device->power.states[ACPI_STATE_D3].power = 0;
> +	device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> +	device->power.states[ACPI_STATE_D3_COLD].power = 0;
>  
>  	/* Set D3cold's explicit_set flag if _PS3 exists. */
>  	if (device->power.states[ACPI_STATE_D3_HOT].flags.explicit_set)
> Index: linux-pm/drivers/ata/libata-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/ata/libata-acpi.c
> +++ linux-pm/drivers/ata/libata-acpi.c
> @@ -947,11 +947,11 @@ static void pata_acpi_set_state(struct a
>  			continue;
>  
>  		acpi_bus_set_power(dev_handle, state.event & PM_EVENT_RESUME ?
> -						ACPI_STATE_D0 : ACPI_STATE_D3);
> +					ACPI_STATE_D0 : ACPI_STATE_D3_COLD);
>  	}
>  
>  	if (!(state.event & PM_EVENT_RESUME))
> -		acpi_bus_set_power(port_handle, ACPI_STATE_D3);
> +		acpi_bus_set_power(port_handle, ACPI_STATE_D3_COLD);
>  }
>  
>  /**
> Index: linux-pm/drivers/ide/ide-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/ide/ide-acpi.c
> +++ linux-pm/drivers/ide/ide-acpi.c
> @@ -520,11 +520,12 @@ void ide_acpi_set_state(ide_hwif_t *hwif
>  	ide_port_for_each_present_dev(i, drive, hwif) {
>  		if (drive->acpidata->obj_handle)
>  			acpi_bus_set_power(drive->acpidata->obj_handle,
> -					   on ? ACPI_STATE_D0 : ACPI_STATE_D3);
> +				on ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD);
>  	}
>  
>  	if (!on)
> -		acpi_bus_set_power(hwif->acpidata->obj_handle, ACPI_STATE_D3);
> +		acpi_bus_set_power(hwif->acpidata->obj_handle,
> +				   ACPI_STATE_D3_COLD);
>  }
>  
>  /**
> Index: linux-pm/drivers/pnp/pnpacpi/core.c
> ===================================================================
> --- linux-pm.orig/drivers/pnp/pnpacpi/core.c
> +++ linux-pm/drivers/pnp/pnpacpi/core.c
> @@ -131,7 +131,7 @@ static int pnpacpi_disable_resources(str
>  	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
>  	ret = 0;
>  	if (acpi_bus_power_manageable(handle))
> -		acpi_bus_set_power(handle, ACPI_STATE_D3);
> +		acpi_bus_set_power(handle, ACPI_STATE_D3_COLD);
>  		/* continue even if acpi_bus_set_power() fails */
>  	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DIS", NULL, NULL)))
>  		ret = -ENODEV;
> @@ -174,10 +174,10 @@ static int pnpacpi_suspend(struct pnp_de
>  
>  	if (acpi_bus_power_manageable(handle)) {
>  		int power_state = acpi_pm_device_sleep_state(&dev->dev, NULL,
> -							     ACPI_STATE_D3);
> +							ACPI_STATE_D3_COLD);
>  		if (power_state < 0)
>  			power_state = (state.event == PM_EVENT_ON) ?
> -					ACPI_STATE_D0 : ACPI_STATE_D3;
> +					ACPI_STATE_D0 : ACPI_STATE_D3_COLD;
>  
>  		/*
>  		 * acpi_bus_set_power() often fails (keyboard port can't be
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -477,7 +477,8 @@ static inline int acpi_pm_device_sleep_s
>  	if (p)
>  		*p = ACPI_STATE_D0;
>  
> -	return (m >= ACPI_STATE_D0 && m <= ACPI_STATE_D3) ? m : ACPI_STATE_D0;
> +	return (m >= ACPI_STATE_D0 && m <= ACPI_STATE_D3_COLD) ?
> +		m : ACPI_STATE_D0;
>  }
>  static inline void acpi_dev_pm_add_dependent(acpi_handle handle,
>  					     struct device *depdev) {}
> 

Re: [PATCH 1/3] ACPI / PM: Only set power states of devices that are power manageable

[Rafael J. Wysocki]

On Monday, July 29, 2013 10:09:53 PM Aaron Lu wrote:
> On 07/27/2013 09:10 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Make acpi_device_set_power() check if the given device is power
> > manageable before checking if the given power state is valid for that
> > device.  Otherwise it will print that "Device does not support" that
> > power state into the kernel log, which may not make sense for some
> > power states (D0 and D3cold are supported by all devices by
> > definition).
> 
> It will not print "Device does not support" that power state if that
> power state is D0 or D3cold since we have unconditionally set those two
> power state's valid flag.

So you didn't actually looked at acpi_bus_get_power_flags() that set the
power.states[].flags.valid flag, because If you had looked at it, you would
have seen that that's not the case.

No, we don't set the valid flag for devices that aren't power manageable
(i.e. have flags.power_manageable unset), which is the *whole* *point* of
this change.

> OTOH, what value should we return for a device node that is not power
> manageable in acpi_device_set_power when the target state is D0 or D3
> cold? The old behavior is to return 0, meanning success without taking
> any actual action.
> 
> In acpi_bus_set_power, if the device is not power manageable, we will
> return -ENODEV; in acpi_dev_pm_full/low_power, we will return 0 as in
> the original acpi_device_set_power. So return -EINVAL here is correct?

No, the original acpi_device_set_power() will return -ENODEV then, but
in my opinion returning -EINVAL is more accurate, because "power
manageable" means "you can change power state of it".

Thanks,
Rafael


> > Tested-by: Yinghai Lu <yinghai@kernel.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/device_pm.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > Index: linux-pm/drivers/acpi/device_pm.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/device_pm.c
> > +++ linux-pm/drivers/acpi/device_pm.c
> > @@ -159,7 +159,8 @@ int acpi_device_set_power(struct acpi_de
> >  	int result = 0;
> >  	bool cut_power = false;
> >  
> > -	if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
> > +	if (!device || !device->flags.power_manageable
> > +	    || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
> >  		return -EINVAL;
> >  
> >  	/* Make sure this is a valid target state */
> > 
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/3] ACPI / PM: Only set power states of devices that are power manageable

[Aaron Lu]

On 07/30/2013 06:21 AM, Rafael J. Wysocki wrote:
> On Monday, July 29, 2013 10:09:53 PM Aaron Lu wrote:
>> On 07/27/2013 09:10 PM, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Make acpi_device_set_power() check if the given device is power
>>> manageable before checking if the given power state is valid for that
>>> device.  Otherwise it will print that "Device does not support" that
>>> power state into the kernel log, which may not make sense for some
>>> power states (D0 and D3cold are supported by all devices by
>>> definition).
>>
>> It will not print "Device does not support" that power state if that
>> power state is D0 or D3cold since we have unconditionally set those two
>> power state's valid flag.
> 
> So you didn't actually looked at acpi_bus_get_power_flags() that set the
> power.states[].flags.valid flag, because If you had looked at it, you would
> have seen that that's not the case.
> 
> No, we don't set the valid flag for devices that aren't power manageable
> (i.e. have flags.power_manageable unset), which is the *whole* *point* of
> this change.

Right, I missed this. Sorry for the noise.

> 
>> OTOH, what value should we return for a device node that is not power
>> manageable in acpi_device_set_power when the target state is D0 or D3
>> cold? The old behavior is to return 0, meanning success without taking
>> any actual action.
>>
>> In acpi_bus_set_power, if the device is not power manageable, we will
>> return -ENODEV; in acpi_dev_pm_full/low_power, we will return 0 as in
>> the original acpi_device_set_power. So return -EINVAL here is correct?
> 
> No, the original acpi_device_set_power() will return -ENODEV then, but
> in my opinion returning -EINVAL is more accurate, because "power
> manageable" means "you can change power state of it".

Shall I prepare a patch to update the errno in acpi_bus_set_power?

Thanks,
Aaron

> 
> Thanks,
> Rafael
> 
> 
>>> Tested-by: Yinghai Lu <yinghai@kernel.org>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>  drivers/acpi/device_pm.c |    3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> Index: linux-pm/drivers/acpi/device_pm.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/acpi/device_pm.c
>>> +++ linux-pm/drivers/acpi/device_pm.c
>>> @@ -159,7 +159,8 @@ int acpi_device_set_power(struct acpi_de
>>>  	int result = 0;
>>>  	bool cut_power = false;
>>>  
>>> -	if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
>>> +	if (!device || !device->flags.power_manageable
>>> +	    || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
>>>  		return -EINVAL;
>>>  
>>>  	/* Make sure this is a valid target state */
>>>
>>

Re: [PATCH 1/3] ACPI / PM: Only set power states of devices that are power manageable

[Rafael J. Wysocki]

On Tuesday, July 30, 2013 07:43:48 AM Aaron Lu wrote:
> On 07/30/2013 06:21 AM, Rafael J. Wysocki wrote:
> > On Monday, July 29, 2013 10:09:53 PM Aaron Lu wrote:
> >> On 07/27/2013 09:10 PM, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> Make acpi_device_set_power() check if the given device is power
> >>> manageable before checking if the given power state is valid for that
> >>> device.  Otherwise it will print that "Device does not support" that
> >>> power state into the kernel log, which may not make sense for some
> >>> power states (D0 and D3cold are supported by all devices by
> >>> definition).
> >>
> >> It will not print "Device does not support" that power state if that
> >> power state is D0 or D3cold since we have unconditionally set those two
> >> power state's valid flag.
> > 
> > So you didn't actually looked at acpi_bus_get_power_flags() that set the
> > power.states[].flags.valid flag, because If you had looked at it, you would
> > have seen that that's not the case.
> > 
> > No, we don't set the valid flag for devices that aren't power manageable
> > (i.e. have flags.power_manageable unset), which is the *whole* *point* of
> > this change.
> 
> Right, I missed this. Sorry for the noise.
> 
> > 
> >> OTOH, what value should we return for a device node that is not power
> >> manageable in acpi_device_set_power when the target state is D0 or D3
> >> cold? The old behavior is to return 0, meanning success without taking
> >> any actual action.
> >>
> >> In acpi_bus_set_power, if the device is not power manageable, we will
> >> return -ENODEV; in acpi_dev_pm_full/low_power, we will return 0 as in
> >> the original acpi_device_set_power. So return -EINVAL here is correct?
> > 
> > No, the original acpi_device_set_power() will return -ENODEV then, but
> > in my opinion returning -EINVAL is more accurate, because "power
> > manageable" means "you can change power state of it".
> 
> Shall I prepare a patch to update the errno in acpi_bus_set_power?

In fact, it doesn't need to check flags.power_manageable after this patch
and the debug message won't be missed I think, so please just remove
the whole if () from there, if that's not a problem.

Thanks,
Rafael


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

Re: [PATCH 1/3] ACPI / PM: Only set power states of devices that are power manageable

[Aaron Lu]

On 07/30/2013 10:04 PM, Rafael J. Wysocki wrote:
> On Tuesday, July 30, 2013 07:43:48 AM Aaron Lu wrote:
>> On 07/30/2013 06:21 AM, Rafael J. Wysocki wrote:
>>> On Monday, July 29, 2013 10:09:53 PM Aaron Lu wrote:
>>>> On 07/27/2013 09:10 PM, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> Make acpi_device_set_power() check if the given device is power
>>>>> manageable before checking if the given power state is valid for that
>>>>> device.  Otherwise it will print that "Device does not support" that
>>>>> power state into the kernel log, which may not make sense for some
>>>>> power states (D0 and D3cold are supported by all devices by
>>>>> definition).
>>>>
>>>> It will not print "Device does not support" that power state if that
>>>> power state is D0 or D3cold since we have unconditionally set those two
>>>> power state's valid flag.
>>>
>>> So you didn't actually looked at acpi_bus_get_power_flags() that set the
>>> power.states[].flags.valid flag, because If you had looked at it, you would
>>> have seen that that's not the case.
>>>
>>> No, we don't set the valid flag for devices that aren't power manageable
>>> (i.e. have flags.power_manageable unset), which is the *whole* *point* of
>>> this change.
>>
>> Right, I missed this. Sorry for the noise.
>>
>>>
>>>> OTOH, what value should we return for a device node that is not power
>>>> manageable in acpi_device_set_power when the target state is D0 or D3
>>>> cold? The old behavior is to return 0, meanning success without taking
>>>> any actual action.
>>>>
>>>> In acpi_bus_set_power, if the device is not power manageable, we will
>>>> return -ENODEV; in acpi_dev_pm_full/low_power, we will return 0 as in
>>>> the original acpi_device_set_power. So return -EINVAL here is correct?
>>>
>>> No, the original acpi_device_set_power() will return -ENODEV then, but
>>> in my opinion returning -EINVAL is more accurate, because "power
>>> manageable" means "you can change power state of it".
>>
>> Shall I prepare a patch to update the errno in acpi_bus_set_power?
> 
> In fact, it doesn't need to check flags.power_manageable after this patch
> and the debug message won't be missed I think, so please just remove
> the whole if () from there, if that's not a problem.

Patch to remove the redundant check, apply on top of this one.

From: Aaron Lu <aaron.lu@intel.com>
Subject: [PATCH 1/3] ACPI / PM: Remove redundant check for power manageable in
 acpi_bus_set_power

Now that we will check if a device is power manageable in
acpi_device_set_power, it is no longer necessary to do this check in
acpi_bus_set_power, so remove it.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/device_pm.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 63324b8..8270711 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -245,13 +245,6 @@ int acpi_bus_set_power(acpi_handle handle, int state)
 	if (result)
 		return result;
 
-	if (!device->flags.power_manageable) {
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-				"Device [%s] is not power manageable\n",
-				dev_name(&device->dev)));
-		return -ENODEV;
-	}
-
 	return acpi_device_set_power(device, state);
 }
 EXPORT_SYMBOL(acpi_bus_set_power);
-- 
1.8.3.2.10.g43d11f4

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Aaron Lu]

On 07/29/2013 08:20 PM, Rafael J. Wysocki wrote:
> On Monday, July 29, 2013 10:29:36 AM Aaron Lu wrote:
>> On 07/27/2013 09:11 PM, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Modify acpi_device_set_power() so that diagnostic messages printed by
>>> it to the kernel log always contain the name of the device concerned
>>> to make it possible to identify the device that triggered the message
>>> if need be.
>>>
>>> Also replace printk(KERN_WARNING ) with dev_warn() everywhere in that
>>> function.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>  drivers/acpi/device_pm.c |   22 ++++++++++------------
>>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> Index: linux-pm/drivers/acpi/device_pm.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/acpi/device_pm.c
>>> +++ linux-pm/drivers/acpi/device_pm.c
>>> @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
>>>  	/* Make sure this is a valid target state */
>>>  
>>>  	if (state == device->power.state) {
>>> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at %s\n",
>>> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] already in %s\n",
>>> +				  device->pnp.bus_id,
>>>  				  acpi_power_state_string(state)));
>>>  		return 0;
>>>  	}
>>>  
>>>  	if (!device->power.states[state].flags.valid) {
>>> -		printk(KERN_WARNING PREFIX "Device does not support %s\n",
>>> -		       acpi_power_state_string(state));
>>> +		dev_warn(&device->dev, "Power state %s not supported\n",
>>> +			 acpi_power_state_string(state));
>>>  		return -ENODEV;
>>>  	}
>>>  	if (device->parent && (state < device->parent->power.state)) {
>>> -		printk(KERN_WARNING PREFIX
>>> -			      "Cannot set device to a higher-powered"
>>> -			      " state than parent\n");
>>> +		dev_warn(&device->dev, "Cannot transition to a higher-powered "
>>> +			 "state than parent\n");
>>
>> I think the state information would also be useful here:
>>
>> 		dev_warn(&device->dev, "Cannot transition to a higher-powereed "
>> 		         "state %d than paeren's state %d\n", state,
>> 			 device->parent->power.state);
> 
> This is not the scope of this patch, please send another one on top of it.
> 

Patch to add state information in error message, apply on top of this
one.

From: Aaron Lu <aaron.lu@intel.com>
Subject: [PATCH] ACPI / PM: Add state information in error message for
 acpi_device_set_power

The state information can be useful to know what the problem is when it
appeared in an error message about a device can not being set to a higher
power state than its parent, so this patch adds such state information
for both the target state of the device failed to be set and the current
parent's state.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/device_pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index beb9625..707258b 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -179,7 +179,8 @@ int acpi_device_set_power(struct acpi_device *device, int state)
 	}
 	if (device->parent && (state < device->parent->power.state)) {
 		dev_warn(&device->dev,
-			 "Cannot transition to a higher-powered state than parent\n");
+			 "Cannot transition to a higher-powered state %d than parent's state %d\n",
+			 state, device->parent->power.state);
 		return -ENODEV;
 	}
 
-- 
1.8.3.2.10.g43d11f4

Re: [PATCH 2/3] ACPI / PM: Make messages in acpi_device_set_power() print device names

[Rafael J. Wysocki]

On Wednesday, July 31, 2013 02:52:53 PM Aaron Lu wrote:
> On 07/29/2013 08:20 PM, Rafael J. Wysocki wrote:
> > On Monday, July 29, 2013 10:29:36 AM Aaron Lu wrote:
> >> On 07/27/2013 09:11 PM, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> Modify acpi_device_set_power() so that diagnostic messages printed by
> >>> it to the kernel log always contain the name of the device concerned
> >>> to make it possible to identify the device that triggered the message
> >>> if need be.
> >>>
> >>> Also replace printk(KERN_WARNING ) with dev_warn() everywhere in that
> >>> function.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>> ---
> >>>  drivers/acpi/device_pm.c |   22 ++++++++++------------
> >>>  1 file changed, 10 insertions(+), 12 deletions(-)
> >>>
> >>> Index: linux-pm/drivers/acpi/device_pm.c
> >>> ===================================================================
> >>> --- linux-pm.orig/drivers/acpi/device_pm.c
> >>> +++ linux-pm/drivers/acpi/device_pm.c
> >>> @@ -166,20 +166,20 @@ int acpi_device_set_power(struct acpi_de
> >>>  	/* Make sure this is a valid target state */
> >>>  
> >>>  	if (state == device->power.state) {
> >>> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at %s\n",
> >>> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] already in %s\n",
> >>> +				  device->pnp.bus_id,
> >>>  				  acpi_power_state_string(state)));
> >>>  		return 0;
> >>>  	}
> >>>  
> >>>  	if (!device->power.states[state].flags.valid) {
> >>> -		printk(KERN_WARNING PREFIX "Device does not support %s\n",
> >>> -		       acpi_power_state_string(state));
> >>> +		dev_warn(&device->dev, "Power state %s not supported\n",
> >>> +			 acpi_power_state_string(state));
> >>>  		return -ENODEV;
> >>>  	}
> >>>  	if (device->parent && (state < device->parent->power.state)) {
> >>> -		printk(KERN_WARNING PREFIX
> >>> -			      "Cannot set device to a higher-powered"
> >>> -			      " state than parent\n");
> >>> +		dev_warn(&device->dev, "Cannot transition to a higher-powered "
> >>> +			 "state than parent\n");
> >>
> >> I think the state information would also be useful here:
> >>
> >> 		dev_warn(&device->dev, "Cannot transition to a higher-powereed "
> >> 		         "state %d than paeren's state %d\n", state,
> >> 			 device->parent->power.state);
> > 
> > This is not the scope of this patch, please send another one on top of it.
> > 
> 
> Patch to add state information in error message, apply on top of this
> one.
> 
> From: Aaron Lu <aaron.lu@intel.com>
> Subject: [PATCH] ACPI / PM: Add state information in error message for
>  acpi_device_set_power
> 
> The state information can be useful to know what the problem is when it
> appeared in an error message about a device can not being set to a higher
> power state than its parent, so this patch adds such state information
> for both the target state of the device failed to be set and the current
> parent's state.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/acpi/device_pm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index beb9625..707258b 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -179,7 +179,8 @@ int acpi_device_set_power(struct acpi_device *device, int state)
>  	}
>  	if (device->parent && (state < device->parent->power.state)) {
>  		dev_warn(&device->dev,
> -			 "Cannot transition to a higher-powered state than parent\n");
> +			 "Cannot transition to a higher-powered state %d than parent's state %d\n",

This message will look a little odd I think ->

> +			 state, device->parent->power.state);

-> and please don't use raw numbers in such messages.

What about

	"Cannot transition to power state %s for parent in %s\n",
	acpi_power_state_string(state),
	acpi_power_state_string(device->parent->power.state)

>  		return -ENODEV;
>  	}

Thanks,
Rafael


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

Re: [PATCH 1/3] ACPI / PM: Only set power states of devices that are power manageable

[Rafael J. Wysocki]

On Wednesday, July 31, 2013 02:48:58 PM Aaron Lu wrote:
> On 07/30/2013 10:04 PM, Rafael J. Wysocki wrote:
> > On Tuesday, July 30, 2013 07:43:48 AM Aaron Lu wrote:
> >> On 07/30/2013 06:21 AM, Rafael J. Wysocki wrote:
> >>> On Monday, July 29, 2013 10:09:53 PM Aaron Lu wrote:
> >>>> On 07/27/2013 09:10 PM, Rafael J. Wysocki wrote:
> >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>>>
> >>>>> Make acpi_device_set_power() check if the given device is power
> >>>>> manageable before checking if the given power state is valid for that
> >>>>> device.  Otherwise it will print that "Device does not support" that
> >>>>> power state into the kernel log, which may not make sense for some
> >>>>> power states (D0 and D3cold are supported by all devices by
> >>>>> definition).
> >>>>
> >>>> It will not print "Device does not support" that power state if that
> >>>> power state is D0 or D3cold since we have unconditionally set those two
> >>>> power state's valid flag.
> >>>
> >>> So you didn't actually looked at acpi_bus_get_power_flags() that set the
> >>> power.states[].flags.valid flag, because If you had looked at it, you would
> >>> have seen that that's not the case.
> >>>
> >>> No, we don't set the valid flag for devices that aren't power manageable
> >>> (i.e. have flags.power_manageable unset), which is the *whole* *point* of
> >>> this change.
> >>
> >> Right, I missed this. Sorry for the noise.
> >>
> >>>
> >>>> OTOH, what value should we return for a device node that is not power
> >>>> manageable in acpi_device_set_power when the target state is D0 or D3
> >>>> cold? The old behavior is to return 0, meanning success without taking
> >>>> any actual action.
> >>>>
> >>>> In acpi_bus_set_power, if the device is not power manageable, we will
> >>>> return -ENODEV; in acpi_dev_pm_full/low_power, we will return 0 as in
> >>>> the original acpi_device_set_power. So return -EINVAL here is correct?
> >>>
> >>> No, the original acpi_device_set_power() will return -ENODEV then, but
> >>> in my opinion returning -EINVAL is more accurate, because "power
> >>> manageable" means "you can change power state of it".
> >>
> >> Shall I prepare a patch to update the errno in acpi_bus_set_power?
> > 
> > In fact, it doesn't need to check flags.power_manageable after this patch
> > and the debug message won't be missed I think, so please just remove
> > the whole if () from there, if that's not a problem.
> 
> Patch to remove the redundant check, apply on top of this one.
> 
> From: Aaron Lu <aaron.lu@intel.com>
> Subject: [PATCH 1/3] ACPI / PM: Remove redundant check for power manageable in
>  acpi_bus_set_power
> 
> Now that we will check if a device is power manageable in
> acpi_device_set_power, it is no longer necessary to do this check in
> acpi_bus_set_power, so remove it.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

Looks good, I'll queue it up for 3.12.

Thanks,
Rafael


> ---
>  drivers/acpi/device_pm.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index 63324b8..8270711 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -245,13 +245,6 @@ int acpi_bus_set_power(acpi_handle handle, int state)
>  	if (result)
>  		return result;
>  
> -	if (!device->flags.power_manageable) {
> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> -				"Device [%s] is not power manageable\n",
> -				dev_name(&device->dev)));
> -		return -ENODEV;
> -	}
> -
>  	return acpi_device_set_power(device, state);
>  }
>  EXPORT_SYMBOL(acpi_bus_set_power);
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH updated] ACPI / PM: Add state information in error message for acpi_device_set_power

[Aaron Lu]

On 07/31/2013 06:27 PM, Rafael J. Wysocki wrote:
>> Patch to add state information in error message, apply on top of this
>> > one.
>> > 
>> > From: Aaron Lu <aaron.lu@intel.com>
>> > Subject: [PATCH] ACPI / PM: Add state information in error message for
>> >  acpi_device_set_power
>> > 
>> > The state information can be useful to know what the problem is when it
>> > appeared in an error message about a device can not being set to a higher
>> > power state than its parent, so this patch adds such state information
>> > for both the target state of the device failed to be set and the current
>> > parent's state.
>> > 
>> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> > ---
>> >  drivers/acpi/device_pm.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> > index beb9625..707258b 100644
>> > --- a/drivers/acpi/device_pm.c
>> > +++ b/drivers/acpi/device_pm.c
>> > @@ -179,7 +179,8 @@ int acpi_device_set_power(struct acpi_device *device, int state)
>> >  	}
>> >  	if (device->parent && (state < device->parent->power.state)) {
>> >  		dev_warn(&device->dev,
>> > -			 "Cannot transition to a higher-powered state than parent\n");
>> > +			 "Cannot transition to a higher-powered state %d than parent's state %d\n",
> This message will look a little odd I think ->
> 
>> > +			 state, device->parent->power.state);
> -> and please don't use raw numbers in such messages.
> 
> What about
> 
> 	"Cannot transition to power state %s for parent in %s\n",
> 	acpi_power_state_string(state),
> 	acpi_power_state_string(device->parent->power.state)


Thanks for the suggestion, updated patch here:

From: Aaron Lu <aaron.lu@intel.com>
Subject: [PATCH] ACPI / PM: Add state information in error message for
 acpi_device_set_power

The state information can be useful to know what the problem is when an
error message about a device can not being set to a higher power state
than its parent appeared, so this patch adds such state information
for both the target state of the device and the current state of its
parent.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/device_pm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index beb9625..59d3202 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -179,7 +179,9 @@ int acpi_device_set_power(struct acpi_device *device, int state)
 	}
 	if (device->parent && (state < device->parent->power.state)) {
 		dev_warn(&device->dev,
-			 "Cannot transition to a higher-powered state than parent\n");
+			 "Cannot transition to power state %s for parent in %s\n",
+			 acpi_power_state_string(state),
+			 acpi_power_state_string(device->parent->power.state));
 		return -ENODEV;
 	}
 
-- 
1.8.3.2.10.g43d11f4