On Monday 14 July 2008 09:39:34 Zhao Yakui wrote: > From: Zhao Yakui <yakui.zhao@xxxxxxxxx> > > Maybe the incorrect power state is returned on the bogus bios, which > is different with the real power state. For example: the bios returns D0 > state and the real power state is D3. OS expects to set the device to D0 > state. In such case if OS uses the power state returned by the BIOS and > checks the device power state very strictly in power transition, the device > can't be transited to the correct power state. > > So the boot option of "acpi.power_nocheck=1" is added to avoid checking > the device power state in the course of device power transition. > > http://bugzilla.kernel.org/show_bug.cgi?id=8049 > http://bugzilla.kernel.org/show_bug.cgi?id=11000 > > Signed-off-by: Zhao Yakui <yakui.zhao@xxxxxxxxx> > Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx> > Signed-off-by: Li Shaohua <shaohua.li@xxxxxxxxx> > > --- > Documentation/kernel-parameters.txt | 7 ++++++ > drivers/acpi/bus.c | 14 +++++++++++- > drivers/acpi/power.c | 42 > ++++++++++++++++++++++++++---------- include/acpi/acpi_drivers.h | > 1 > 4 files changed, 52 insertions(+), 12 deletions(-) > > Index: linux-2.6/Documentation/kernel-parameters.txt > =================================================================== > --- linux-2.6.orig/Documentation/kernel-parameters.txt > +++ linux-2.6/Documentation/kernel-parameters.txt > @@ -224,6 +224,13 @@ and is between 256 and 4096 characters. > The number can be in decimal or prefixed with 0x in hex. > Warning: Many of these options can produce a lot of > output and make your system unusable. Be very careful. > + acpi.power_nocheck= [HW,ACPI] > + Format: 1/0 enable/disable the check of power state. > + On some bogus BIOS the _PSC object/_STA object of > + power resource can't return the correct device power > + state. In such case it is unneccessary to check its > + power state again in power transition. > + 1 : disable the power state check > > acpi_pm_good [X86-32,X86-64] > Override the pmtimer bug detection: force the kernel > Index: linux-2.6/drivers/acpi/power.c > =================================================================== > --- linux-2.6.orig/drivers/acpi/power.c > +++ linux-2.6/drivers/acpi/power.c > @@ -54,6 +54,14 @@ ACPI_MODULE_NAME("power"); > #define ACPI_POWER_RESOURCE_STATE_OFF 0x00 > #define ACPI_POWER_RESOURCE_STATE_ON 0x01 > #define ACPI_POWER_RESOURCE_STATE_UNKNOWN 0xFF > + > +#ifdef MODULE_PARAM_PREFIX > +#undef MODULE_PARAM_PREFIX > +#endif > +#define MODULE_PARAM_PREFIX "acpi." > +int acpi_power_nocheck; > +module_param_named(power_nocheck, acpi_power_nocheck, bool, 000); > + > static int acpi_power_add(struct acpi_device *device); > static int acpi_power_remove(struct acpi_device *device, int type); > static int acpi_power_resume(struct acpi_device *device); > @@ -228,12 +236,18 @@ static int acpi_power_on(acpi_handle han > if (ACPI_FAILURE(status)) > return -ENODEV; > > - result = acpi_power_get_state(resource->device->handle, &state); > - if (result) > - return result; > - if (state != ACPI_POWER_RESOURCE_STATE_ON) > - return -ENOEXEC; > - > + if (!acpi_power_nocheck) { > + /* > + * If acpi_power_nocheck is set, it is unnecessary to check > + * the power state after power transition. > + */ > + result = acpi_power_get_state(resource->device->handle, > + &state); > + if (result) > + return result; > + if (state != ACPI_POWER_RESOURCE_STATE_ON) > + return -ENOEXEC; > + } > /* Update the power resource's _device_ power state */ > resource->device->power.state = ACPI_STATE_D0; A fat ACPI_EXCEPTION that the power state is bogus and already point to the acpi.power_nocheck boot param before returning -ENOEXEC would help to find this boot param for affected machines? I wonder whether this (acpi.power_nocheck=1) could even get the default, maybe after waiting for reponse about how many machines really show this and why... The current implementation sets: device->power.state = ACPI_STATE_UNKNOWN; if -ENOEXEC is returned (or other errors happen) thermal management is rather blown up then? Setting a sane default (as you did if the boot param is passed) if things go wrong and print a warning sounds reasonable in general? While going through power.c it looks like there could be more warnings or more better fallbacks added: in acpi_power_add(): switch (state) { case ACPI_POWER_RESOURCE_STATE_ON: device->power.state = ACPI_STATE_D0; break; case ACPI_POWER_RESOURCE_STATE_OFF: device->power.state = ACPI_STATE_D3; break; default: device->power.state = ACPI_STATE_UNKNOWN; break; } The last one is a BIOS bug. Also the power state must never be ACPI_STATE_UNKNOWN or the whole driver is rather useless? E.g. the check in acpi_power_transition() indicates that if the state is ACPI_STATE_UNKNOWN things are rather broken: if ((device->power.state < ACPI_STATE_D0) || (device->power.state > ACPI_STATE_D3)) return -ENODEV; What do you think about this cleanup patch. I wonder whether it makes sense to not only throw an error message at places where ACPI_STATE_UNKNOWN is set, but also set ACPI_STATE_D3 (or ACPI_STATE_D0, whatever is expected) and try to keep power resource management up, even on bogus BIOS values? --- drivers/acpi/power.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) Index: pci-2.6/drivers/acpi/power.c =================================================================== --- pci-2.6.orig/drivers/acpi/power.c +++ pci-2.6/drivers/acpi/power.c @@ -337,7 +337,7 @@ int acpi_device_sleep_wake(struct acpi_d if (ACPI_SUCCESS(status)) { return 0; } else if (status != AE_NOT_FOUND) { - printk(KERN_ERR PREFIX "_DSW execution failed\n"); + ACPI_EXCEPTION((AE_INFO, status, "_DSW execution failed\n")); dev->wakeup.flags.valid = 0; return -ENODEV; } @@ -347,7 +347,7 @@ int acpi_device_sleep_wake(struct acpi_d in_arg[0].integer.value = enable; status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL); if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) { - printk(KERN_ERR PREFIX "_PSW execution failed\n"); + ACPI_EXCEPTION((AE_INFO, status, "_PSW execution failed\n")); dev->wakeup.flags.valid = 0; return -ENODEV; } @@ -379,7 +379,11 @@ int acpi_enable_wakeup_device_power(stru for (i = 0; i < dev->wakeup.resources.count; i++) { int ret = acpi_power_on(dev->wakeup.resources.handles[i], dev); if (ret) { - printk(KERN_ERR PREFIX "Transition power state\n"); + /* ToDo make use of acpi_status in acpi_power_on and + * others for better error tracking + */ + ACPI_EXCEPTION((AE_INFO, AE_ERROR, + "Transition power state\n")); dev->wakeup.flags.valid = 0; return -ENODEV; } @@ -462,8 +466,13 @@ int acpi_power_get_inferred_state(struct continue; result = acpi_power_get_list_state(list, &list_state); - if (result) + if (result) { + ACPI_EXCEPTION((AE_INFO, AE_ERROR, + "Could not infer state for" + "power resource [%s]\n", + acpi_device_bid(device))); return result; + } if (list_state == ACPI_POWER_RESOURCE_STATE_ON) { device->power.state = i; @@ -680,6 +689,9 @@ static int acpi_power_add(struct acpi_de device->power.state = ACPI_STATE_D3; break; default: + ACPI_EXCEPTION((AE_INFO, AE_ERROR, + "Power resource [%s] returned bogus value\n", + acpi_device_bid(device))); device->power.state = ACPI_STATE_UNKNOWN; break; } Thomas > > @@ -279,11 +293,17 @@ static int acpi_power_off_device(acpi_ha > if (ACPI_FAILURE(status)) > return -ENODEV; > > - result = acpi_power_get_state(handle, &state); > - if (result) > - return result; > - if (state != ACPI_POWER_RESOURCE_STATE_OFF) > - return -ENOEXEC; > + if (!acpi_power_nocheck) { > + /* > + * If acpi_power_nocheck is set, it is unnecessary to check > + * the power state after power transition. > + */ > + result = acpi_power_get_state(handle, &state); > + if (result) > + return result; > + if (state != ACPI_POWER_RESOURCE_STATE_OFF) > + return -ENOEXEC; > + } > > /* Update the power resource's _device_ power state */ > resource->device->power.state = ACPI_STATE_D3; > Index: linux-2.6/include/acpi/acpi_drivers.h > =================================================================== > --- linux-2.6.orig/include/acpi/acpi_drivers.h > +++ linux-2.6/include/acpi/acpi_drivers.h > @@ -91,6 +91,7 @@ int acpi_enable_wakeup_device_power(stru > int acpi_disable_wakeup_device_power(struct acpi_device *dev); > int acpi_power_get_inferred_state(struct acpi_device *device); > int acpi_power_transition(struct acpi_device *device, int state); > +extern int acpi_power_nocheck; > #endif > > /* > -------------------------------------------------------------------------- > Index: linux-2.6/drivers/acpi/bus.c > =================================================================== > --- linux-2.6.orig/drivers/acpi/bus.c > +++ linux-2.6/drivers/acpi/bus.c > @@ -223,7 +223,19 @@ int acpi_bus_set_power(acpi_handle handl > /* > * Get device's current power state > */ > - acpi_bus_get_power(device->handle, &device->power.state); > + if (!acpi_power_nocheck) { > + /* > + * Maybe the incorrect power state is returned on the bogus > + * bios, which is different with the real power state. > + * For example: the bios returns D0 state and the real power > + * state is D3. OS expects to set the device to D0 state. In > + * such case if OS uses the power state returned by the BIOS, > + * the device can't be transisted to the correct power state. > + * So if the acpi_power_nocheck is set, it is unnecessary to > + * get the power state by calling acpi_bus_get_power. > + */ > + acpi_bus_get_power(device->handle, &device->power.state); > + } > if ((state == device->power.state) && !device->flags.force_power_state) { > ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device is already at D%d\n", > state)); > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
