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.
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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,
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
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 */
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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) {}
On 07/27/2013 09:11 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
2013/7/27 Rafael J. Wysocki <[email protected]>:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best regards
Tianyu Lan
On Mon, 2013-07-29 at 11:06 +0800, Lan Tianyu wrote:
> 2013/7/27 Rafael J. Wysocki <[email protected]>:
> > From: Rafael J. Wysocki <[email protected]>
[]
> > 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.
On Monday, July 29, 2013 11:06:36 AM Lan Tianyu wrote:
> 2013/7/27 Rafael J. Wysocki <[email protected]>:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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.
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 <[email protected]>:
> > > From: Rafael J. Wysocki <[email protected]>
> []
> > > 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
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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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.
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
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 <[email protected]>
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 <[email protected]>
---
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,
On 07/27/2013 09:10 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> 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 */
>
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 <[email protected]>
> 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 <[email protected]>
Reviewed-by: Aaron Lu <[email protected]>
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,
>
On 07/27/2013 09:14 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Aaron Lu <[email protected]>
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) {}
>
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > 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.
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>> ---
>>> 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 */
>>>
>>
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 <[email protected]>
> >>>
> >>> 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.
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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> ---
> >>> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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.
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 <[email protected]>
> >>>>>
> >>>>> 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 <[email protected]>
> 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 <[email protected]>
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.
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 <[email protected]>
>> > 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 <[email protected]>
>> > ---
>> > 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 <[email protected]>
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 <[email protected]>
---
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