[Resend][PATCH 0/3] ACPI / PM: Patches missing from linux-acpi-2.6/test

[Resend][PATCH 0/3] ACPI / PM: Patches missing from linux-acpi-2.6/test

[Rafael J. Wysocki]

Hi Len,

The following three patches seem to have been dropped from your 'test' branch.

If that happened by accident, please reapply.  Otherwise, please let me know
what's wrong with the patches so that I can fix them.

[1/3] - Make fujitsu_laptop use acpi_bus_update_power() instead of
        acpi_bus_get_power() which is unsafe.

[2/3] - Drop acpi_bus_get_power() which is unsafe and has no users.

[3/3] - Do not call __acpi_bus_get_power() from acpi_bus_set_power()
        and remove acpi_power_nocheck that is not necessary any more.

Without these patches the power resources handling rework is incomplete and
the code will not work correctly in some situations. 

Thanks,
Rafael

[Resend][PATCH 1/3] Platform / x86: Make fujitsu_laptop use acpi_bus_update_power()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Use the new function acpi_bus_update_power(), which is safer than
acpi_bus_get_power(), for getting device power state in
acpi_fujitsu_add() and acpi_fujitsu_hotkey_add().

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Reported-and-Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
---
 drivers/platform/x86/fujitsu-laptop.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/platform/x86/fujitsu-laptop.c
===================================================================
--- linux-2.6.orig/drivers/platform/x86/fujitsu-laptop.c
+++ linux-2.6/drivers/platform/x86/fujitsu-laptop.c
@@ -689,7 +689,7 @@ static int acpi_fujitsu_add(struct acpi_
 	if (error)
 		goto err_free_input_dev;
 
-	result = acpi_bus_get_power(fujitsu->acpi_handle, &state);
+	result = acpi_bus_update_power(fujitsu->acpi_handle, &state);
 	if (result) {
 		printk(KERN_ERR "Error reading power state\n");
 		goto err_unregister_input_dev;
@@ -857,7 +857,7 @@ static int acpi_fujitsu_hotkey_add(struc
 	if (error)
 		goto err_free_input_dev;
 
-	result = acpi_bus_get_power(fujitsu_hotkey->acpi_handle, &state);
+	result = acpi_bus_update_power(fujitsu_hotkey->acpi_handle, &state);
 	if (result) {
 		printk(KERN_ERR "Error reading power state\n");
 		goto err_unregister_input_dev;

Re: [Resend][PATCH 1/3] Platform / x86: Make fujitsu_laptop use acpi_bus_update_power()

[Jonathan Woithe]

Hi guys

For a whole variety of reasons which are too complicated to go into now
(including but not limited to me not being on the direct "to" list), I
missed this when it was posted in early-to-mid December.

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Use the new function acpi_bus_update_power(), which is safer than
> acpi_bus_get_power(), for getting device power state in
> acpi_fujitsu_add() and acpi_fujitsu_hotkey_add().
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> Reported-and-Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> ---
>  drivers/platform/x86/fujitsu-laptop.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/drivers/platform/x86/fujitsu-laptop.c
> ===================================================================
> --- linux-2.6.orig/drivers/platform/x86/fujitsu-laptop.c
> +++ linux-2.6/drivers/platform/x86/fujitsu-laptop.c
> @@ -689,7 +689,7 @@ static int acpi_fujitsu_add(struct acpi_
>  	if (error)
>  		goto err_free_input_dev;
>  
> -	result = acpi_bus_get_power(fujitsu->acpi_handle, &state);
> +	result = acpi_bus_update_power(fujitsu->acpi_handle, &state);
>  	if (result) {
>  		printk(KERN_ERR "Error reading power state\n");
>  		goto err_unregister_input_dev;
> @@ -857,7 +857,7 @@ static int acpi_fujitsu_hotkey_add(struc
>  	if (error)
>  		goto err_free_input_dev;
>  
> -	result = acpi_bus_get_power(fujitsu_hotkey->acpi_handle, &state);
> +	result = acpi_bus_update_power(fujitsu_hotkey->acpi_handle, &state);
>  	if (result) {
>  		printk(KERN_ERR "Error reading power state\n");
>  		goto err_unregister_input_dev;
> 

For what it's worth and in case it's important, here's a belated ack:

Acked-by: Jonathan Woithe <jwoithe@physics.adelaide.edu.au>

Regards
  jonathan

[Resend][PATCH 2/3] ACPI / PM: Drop acpi_bus_get_power()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

There are no more users of acpi_bus_get_power(), so it can be
dropped.  Moreover, it should be dropped, because it modifies
the device->power.state field of an ACPI device without updating
the reference counters of the device's power resources, which is
wrong.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/bus.c      |   19 -------------------
 include/acpi/acpi_bus.h |    1 -
 2 files changed, 20 deletions(-)

Index: linux-2.6/drivers/acpi/bus.c
===================================================================
--- linux-2.6.orig/drivers/acpi/bus.c
+++ linux-2.6/drivers/acpi/bus.c
@@ -236,25 +236,6 @@ static int __acpi_bus_get_power(struct a
 }
 
 
-int acpi_bus_get_power(acpi_handle handle, int *state)
-{
-	struct acpi_device *device;
-	int result;
-
-	result = acpi_bus_get_device(handle, &device);
-	if (result)
-		return result;
-
-	result = __acpi_bus_get_power(device, state);
-	if (result)
-		return result;
-
-	device->power.state = *state;
-	return 0;
-}
-EXPORT_SYMBOL(acpi_bus_get_power);
-
-
 static int __acpi_bus_set_power(struct acpi_device *device, int state)
 {
 	int result = 0;
Index: linux-2.6/include/acpi/acpi_bus.h
===================================================================
--- linux-2.6.orig/include/acpi/acpi_bus.h
+++ linux-2.6/include/acpi/acpi_bus.h
@@ -327,7 +327,6 @@ void acpi_bus_data_handler(acpi_handle h
 acpi_status acpi_bus_get_status_handle(acpi_handle handle,
 				       unsigned long long *sta);
 int acpi_bus_get_status(struct acpi_device *device);
-int acpi_bus_get_power(acpi_handle handle, int *state);
 int acpi_bus_set_power(acpi_handle handle, int state);
 int acpi_bus_update_power(acpi_handle handle, int *state_p);
 bool acpi_bus_power_manageable(acpi_handle handle);

[Resend][PATCH 3/3] ACPI / PM: Drop acpi_power_nocheck

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Since acpi_bus_set_power() should not use __acpi_bus_get_power() to
update the device's device->power.state field before changing its
power state (this may cause device->power.state to be inconsistent
with the device power resources' reference counters), remove this
call from it.  In consequence, the acpi_power_nocheck variable is not
necessary any more, so it can be dropped along with the DMI table
used for setting that variable for HP Pavilion 05.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/bus.c      |   39 ---------------------------------------
 drivers/acpi/internal.h |    1 -
 drivers/acpi/power.c    |    3 ---
 3 files changed, 43 deletions(-)

Index: linux-2.6/drivers/acpi/bus.c
===================================================================
--- linux-2.6.orig/drivers/acpi/bus.c
+++ linux-2.6/drivers/acpi/bus.c
@@ -52,22 +52,6 @@ EXPORT_SYMBOL(acpi_root_dir);
 
 #define STRUCT_TO_INT(s)	(*((int*)&s))
 
-static int set_power_nocheck(const struct dmi_system_id *id)
-{
-	printk(KERN_NOTICE PREFIX "%s detected - "
-		"disable power check in power transition\n", id->ident);
-	acpi_power_nocheck = 1;
-	return 0;
-}
-static struct dmi_system_id __cpuinitdata power_nocheck_dmi_table[] = {
-	{
-	set_power_nocheck, "HP Pavilion 05", {
-	DMI_MATCH(DMI_BIOS_VENDOR, "Phoenix Technologies LTD"),
-	DMI_MATCH(DMI_SYS_VENDOR, "HP Pavilion 05"),
-	DMI_MATCH(DMI_PRODUCT_VERSION, "2001211RE101GLEND") }, NULL},
-	{},
-};
-
 
 #ifdef CONFIG_X86
 static int set_copy_dsdt(const struct dmi_system_id *id)
@@ -333,23 +317,6 @@ int acpi_bus_set_power(acpi_handle handl
 		return -ENODEV;
 	}
 
-	/*
-	 * Get device's current 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, &device->power.state);
-	}
-
 	return __acpi_bus_set_power(device, state);
 }
 EXPORT_SYMBOL(acpi_bus_set_power);
@@ -1072,12 +1039,6 @@ static int __init acpi_init(void)
 	if (acpi_disabled)
 		return result;
 
-	/*
-	 * If the laptop falls into the DMI check table, the power state check
-	 * will be disabled in the course of device power transition.
-	 */
-	dmi_check_system(power_nocheck_dmi_table);
-
 	acpi_scan_init();
 	acpi_ec_init();
 	acpi_debugfs_init();
Index: linux-2.6/drivers/acpi/internal.h
===================================================================
--- linux-2.6.orig/drivers/acpi/internal.h
+++ linux-2.6/drivers/acpi/internal.h
@@ -45,7 +45,6 @@ int acpi_power_get_inferred_state(struct
 int acpi_power_on_resources(struct acpi_device *device, int state);
 int acpi_power_transition(struct acpi_device *device, int state);
 int acpi_bus_init_power(struct acpi_device *device);
-extern int acpi_power_nocheck;
 
 int acpi_wakeup_device_init(void);
 void acpi_early_processor_set_pdc(void);
Index: linux-2.6/drivers/acpi/power.c
===================================================================
--- linux-2.6.orig/drivers/acpi/power.c
+++ linux-2.6/drivers/acpi/power.c
@@ -56,9 +56,6 @@ ACPI_MODULE_NAME("power");
 #define ACPI_POWER_RESOURCE_STATE_ON	0x01
 #define ACPI_POWER_RESOURCE_STATE_UNKNOWN 0xFF
 
-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);

Re: [Resend][PATCH 0/3] ACPI / PM: Patches missing from linux-acpi-2.6/test

[ykzhao]

On Sun, 2010-12-12 at 06:39 +0800, Rafael J. Wysocki wrote:
> Hi Len,
> 
> The following three patches seem to have been dropped from your 'test' branch.
> 
> If that happened by accident, please reapply.  Otherwise, please let me know
> what's wrong with the patches so that I can fix them.
> 
> [1/3] - Make fujitsu_laptop use acpi_bus_update_power() instead of
>         acpi_bus_get_power() which is unsafe.

It seems that the function of acpi_bus_update_power not only obtains
the current power state, but also set the corresponding power state.
Right?

If the device reports the bogus power state, maybe we will set the
incorrect power state for the corresponding device when using the
function of acpi_bus_update_power instead of acpi_bus_get_power. In such
case maybe the device can't work well. 

The bogus power state is reported for some devices on some laptops. For
example: 
    http://bugzilla.kernel.org/show_bug.cgi?id=8049
    http://bugzilla.kernel.org/show_bug.cgi?id=11000

Thanks.
     Yakui

> 
> [2/3] - Drop acpi_bus_get_power() which is unsafe and has no users.
> 
> [3/3] - Do not call __acpi_bus_get_power() from acpi_bus_set_power()
>         and remove acpi_power_nocheck that is not necessary any more.      
> 
> Without these patches the power resources handling rework is incomplete and
> the code will not work correctly in some situations. 
> 
> Thanks,
> Rafael
> 
> --
> 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

--
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

Re: [Resend][PATCH 0/3] ACPI / PM: Patches missing from linux-acpi-2.6/test

[Rafael J. Wysocki]

On Monday, December 13, 2010, ykzhao wrote:
> On Sun, 2010-12-12 at 06:39 +0800, Rafael J. Wysocki wrote:
> > Hi Len,
> > 
> > The following three patches seem to have been dropped from your 'test' branch.
> > 
> > If that happened by accident, please reapply.  Otherwise, please let me know
> > what's wrong with the patches so that I can fix them.
> > 
> > [1/3] - Make fujitsu_laptop use acpi_bus_update_power() instead of
> >         acpi_bus_get_power() which is unsafe.
> 
> It seems that the function of acpi_bus_update_power not only obtains
> the current power state, but also set the corresponding power state.
> Right?

Yes, it does.

> If the device reports the bogus power state, maybe we will set the
> incorrect power state for the corresponding device when using the
> function of acpi_bus_update_power instead of acpi_bus_get_power.

Please actually look at acpi_bus_get_power() (being removed by [2/3]) and note
that it _also_ modifies device->power.state (it doesn't return the state, actually),
so if the returned state is really bogus, we'll have a mismatch between
device->power.state and the real state of the device.  This cannot be good.

In the case of acpi_bus_update_power() we at least _try_ to keep the two things
in sync.

Note, this is _essentially_ important for power resources (if
acpi_bus_get_power() is used, the refcounts are _guaranteed_ not to be in sync
with device->power.state in some situations).

> In such case maybe the device can't work well. 
> 
> The bogus power state is reported for some devices on some laptops. For
> example: 
>     http://bugzilla.kernel.org/show_bug.cgi?id=8049
>     http://bugzilla.kernel.org/show_bug.cgi?id=11000

These bugs are about acpi_bus_set_power() doing the acpi_bus_get_power()
before setting the state, which is wrong and is being removed by my previous
patches (now in the Len's tree).

Thanks,
Rafael
--
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

Re: [Resend][PATCH 0/3] ACPI / PM: Patches missing from linux-acpi-2.6/test

[ykzhao]

On Tue, 2010-12-14 at 05:25 +0800, Rafael J. Wysocki wrote:
> On Monday, December 13, 2010, ykzhao wrote:
> > On Sun, 2010-12-12 at 06:39 +0800, Rafael J. Wysocki wrote:
> > > Hi Len,
> > > 
> > > The following three patches seem to have been dropped from your 'test' branch.
> > > 
> > > If that happened by accident, please reapply.  Otherwise, please let me know
> > > what's wrong with the patches so that I can fix them.
> > > 
> > > [1/3] - Make fujitsu_laptop use acpi_bus_update_power() instead of
> > >         acpi_bus_get_power() which is unsafe.
> > 
> > It seems that the function of acpi_bus_update_power not only obtains
> > the current power state, but also set the corresponding power state.
> > Right?
> 
> Yes, it does.
> 
> > If the device reports the bogus power state, maybe we will set the
> > incorrect power state for the corresponding device when using the
> > function of acpi_bus_update_power instead of acpi_bus_get_power.
> 
> Please actually look at acpi_bus_get_power() (being removed by [2/3]) and note
> that it _also_ modifies device->power.state (it doesn't return the state, actually),
> so if the returned state is really bogus, we'll have a mismatch between
> device->power.state and the real state of the device.  This cannot be good.

The device->power.state is also updated in the function of
acpi_bus_get_power. But it won't try to call the _PSC or _ON/OFF method
to really change the corresponding power state. It only reports the
corresponding power state. 

> In the case of acpi_bus_update_power() we at least _try_ to keep the two things
> in sync.

Yes. I agree that the acpi_bus_update_power can always assure that the
two things are in sync state. But some systems will report the bogus
power state although we already set another power state(For example:
Maybe it reports that it is in D3 state because of bogus BIOS code). In
such case if we try to turn off the corresponding power resource by
using _PSC/_ON/_OFF method, maybe we will cut off the power supply for
these devices.


> Note, this is _essentially_ important for power resources (if
> acpi_bus_get_power() is used, the refcounts are _guaranteed_ not to be in sync
> with device->power.state in some situations).
> 
> > In such case maybe the device can't work well. 
> > 
> > The bogus power state is reported for some devices on some laptops. For
> > example: 
> >     http://bugzilla.kernel.org/show_bug.cgi?id=8049
> >     http://bugzilla.kernel.org/show_bug.cgi?id=11000
> 
> These bugs are about acpi_bus_set_power() doing the acpi_bus_get_power()
> before setting the state, which is wrong and is being removed by my previous
> patches (now in the Len's tree).

It seems that another point is missed in previous patch. 
   Before the reworking patch of power resource, the force_power_state
flag is used when setting the corresponding power state for some ACPI
devices(For example: Fan). This flag will still force to call the
_PSC/_ON/_OFF method even when it is already the same as the target
state. Maybe this is to workaround the BIOS issue that the power state
is not reported correctly first time.

   Not sure whether my above understanding is reasonable.

Thanks.
    Yakui
   
> 
> Thanks,
> Rafael

--
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

Re: [Resend][PATCH 0/3] ACPI / PM: Patches missing from linux-acpi-2.6/test

[Rafael J. Wysocki]

On Tuesday, December 14, 2010, ykzhao wrote:
> On Tue, 2010-12-14 at 05:25 +0800, Rafael J. Wysocki wrote:
> > On Monday, December 13, 2010, ykzhao wrote:
> > > On Sun, 2010-12-12 at 06:39 +0800, Rafael J. Wysocki wrote:
> > > > Hi Len,
> > > > 
> > > > The following three patches seem to have been dropped from your 'test' branch.
> > > > 
> > > > If that happened by accident, please reapply.  Otherwise, please let me know
> > > > what's wrong with the patches so that I can fix them.
> > > > 
> > > > [1/3] - Make fujitsu_laptop use acpi_bus_update_power() instead of
> > > >         acpi_bus_get_power() which is unsafe.
> > > 
> > > It seems that the function of acpi_bus_update_power not only obtains
> > > the current power state, but also set the corresponding power state.
> > > Right?
> > 
> > Yes, it does.
> > 
> > > If the device reports the bogus power state, maybe we will set the
> > > incorrect power state for the corresponding device when using the
> > > function of acpi_bus_update_power instead of acpi_bus_get_power.
> > 
> > Please actually look at acpi_bus_get_power() (being removed by [2/3]) and note
> > that it _also_ modifies device->power.state (it doesn't return the state, actually),
> > so if the returned state is really bogus, we'll have a mismatch between
> > device->power.state and the real state of the device.  This cannot be good.
> 
> The device->power.state is also updated in the function of
> acpi_bus_get_power. But it won't try to call the _PSC or _ON/OFF method
> to really change the corresponding power state. It only reports the
> corresponding power state. 
> 
> > In the case of acpi_bus_update_power() we at least _try_ to keep the two things
> > in sync.
> 
> Yes. I agree that the acpi_bus_update_power can always assure that the
> two things are in sync state. But some systems will report the bogus
> power state although we already set another power state(For example:
> Maybe it reports that it is in D3 state because of bogus BIOS code). In
> such case if we try to turn off the corresponding power resource by
> using _PSC/_ON/_OFF method, maybe we will cut off the power supply for
> these devices.

If you put a device into D0 and __acpi_bus_get_power() sees it in D3, you never
know why that is so.  Either the device refused to change the state, in which
case the reported value is valid, or it reports a wrong value, but there's no
way to tell which the case really is.  Assuming that the reported value is
possibly wrong, we mishandle the case when it's actually correct.  Assuming
that the reported value is correct we may end up putting the device into the
state corresponding to the reported value, which effectively prevents it from
going into D0.  In this case, though, our knowledge of the device state is
consistent with the reported state, which I think is better.

Anyway, IMO the only place where acpi_bus_update_power() may possibly cause
problems to happen on broken hardware is fan_get_cur_state(), but I'll only
change that if anyone reports a problem with it.  The other usage cases are (a)
initialization and (b) resume where we have no real choice but to actually
trust the state reported ACPI control methods (because we don't know what
it is in the first place).

> > Note, this is _essentially_ important for power resources (if
> > acpi_bus_get_power() is used, the refcounts are _guaranteed_ not to be in sync
> > with device->power.state in some situations).
> > 
> > > In such case maybe the device can't work well. 
> > > 
> > > The bogus power state is reported for some devices on some laptops. For
> > > example: 
> > >     http://bugzilla.kernel.org/show_bug.cgi?id=8049
> > >     http://bugzilla.kernel.org/show_bug.cgi?id=11000
> > 
> > These bugs are about acpi_bus_set_power() doing the acpi_bus_get_power()
> > before setting the state, which is wrong and is being removed by my previous
> > patches (now in the Len's tree).
> 
> It seems that another point is missed in previous patch. 
>    Before the reworking patch of power resource, the force_power_state
> flag is used when setting the corresponding power state for some ACPI
> devices(For example: Fan). This flag will still force to call the
> _PSC/_ON/_OFF method even when it is already the same as the target
> state. Maybe this is to workaround the BIOS issue that the power state
> is not reported correctly first time.
> 
>    Not sure whether my above understanding is reasonable.

The force_power_state was _only_ a workaround for the case when
acpi_bus_set_power() called acpi_bus_get_power() and apparently received
a wrong value from it on some systems (two of them had been identified, one was
blacklisted in the end).  However, after my changes acpi_bus_set_power()
doesn't call acpi_bus_get_power() (or any equivalent) any more, so this
workaround isn't necessary any more either.

Thanks,
Rafael
--
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

Re: [Resend][PATCH 0/3] ACPI / PM: Patches missing from linux-acpi-2.6/test

[ykzhao]

On Wed, 2010-12-15 at 04:24 +0800, Rafael J. Wysocki wrote:
> On Tuesday, December 14, 2010, ykzhao wrote:
> > On Tue, 2010-12-14 at 05:25 +0800, Rafael J. Wysocki wrote:
> > > On Monday, December 13, 2010, ykzhao wrote:
> > > > On Sun, 2010-12-12 at 06:39 +0800, Rafael J. Wysocki wrote:
> > > > > Hi Len,
> > > > > 
> > > > > The following three patches seem to have been dropped from your 'test' branch.
> > > > > 
> > > > > If that happened by accident, please reapply.  Otherwise, please let me know
> > > > > what's wrong with the patches so that I can fix them.
> > > > > 
> > > > > [1/3] - Make fujitsu_laptop use acpi_bus_update_power() instead of
> > > > >         acpi_bus_get_power() which is unsafe.
> > > > 
> > > > It seems that the function of acpi_bus_update_power not only obtains
> > > > the current power state, but also set the corresponding power state.
> > > > Right?
> > > 
> > > Yes, it does.
> > > 
> > > > If the device reports the bogus power state, maybe we will set the
> > > > incorrect power state for the corresponding device when using the
> > > > function of acpi_bus_update_power instead of acpi_bus_get_power.
> > > 
> > > Please actually look at acpi_bus_get_power() (being removed by [2/3]) and note
> > > that it _also_ modifies device->power.state (it doesn't return the state, actually),
> > > so if the returned state is really bogus, we'll have a mismatch between
> > > device->power.state and the real state of the device.  This cannot be good.
> > 
> > The device->power.state is also updated in the function of
> > acpi_bus_get_power. But it won't try to call the _PSC or _ON/OFF method
> > to really change the corresponding power state. It only reports the
> > corresponding power state. 
> > 
> > > In the case of acpi_bus_update_power() we at least _try_ to keep the two things
> > > in sync.
> > 
> > Yes. I agree that the acpi_bus_update_power can always assure that the
> > two things are in sync state. But some systems will report the bogus
> > power state although we already set another power state(For example:
> > Maybe it reports that it is in D3 state because of bogus BIOS code). In
> > such case if we try to turn off the corresponding power resource by
> > using _PSC/_ON/_OFF method, maybe we will cut off the power supply for
> > these devices.
> 
> If you put a device into D0 and __acpi_bus_get_power() sees it in D3, you never
> know why that is so.  Either the device refused to change the state, in which
> case the reported value is valid, or it reports a wrong value, but there's no
> way to tell which the case really is.  Assuming that the reported value is
> possibly wrong, we mishandle the case when it's actually correct.  Assuming
> that the reported value is correct we may end up putting the device into the
> state corresponding to the reported value, which effectively prevents it from
> going into D0.  In this case, though, our knowledge of the device state is
> consistent with the reported state, which I think is better.
> 
> Anyway, IMO the only place where acpi_bus_update_power() may possibly cause
> problems to happen on broken hardware is fan_get_cur_state(), but I'll only
> change that if anyone reports a problem with it.  The other usage cases are (a)
> initialization and (b) resume where we have no real choice but to actually
> trust the state reported ACPI control methods (because we don't know what
> it is in the first place).

Agree.  Maybe the matter is the broken BIOS/hardware.

> > > Note, this is _essentially_ important for power resources (if
> > > acpi_bus_get_power() is used, the refcounts are _guaranteed_ not to be in sync
> > > with device->power.state in some situations).
> > > 
> > > > In such case maybe the device can't work well. 
> > > > 
> > > > The bogus power state is reported for some devices on some laptops. For
> > > > example: 
> > > >     http://bugzilla.kernel.org/show_bug.cgi?id=8049
> > > >     http://bugzilla.kernel.org/show_bug.cgi?id=11000
> > > 
> > > These bugs are about acpi_bus_set_power() doing the acpi_bus_get_power()
> > > before setting the state, which is wrong and is being removed by my previous
> > > patches (now in the Len's tree).
> > 
> > It seems that another point is missed in previous patch. 
> >    Before the reworking patch of power resource, the force_power_state
> > flag is used when setting the corresponding power state for some ACPI
> > devices(For example: Fan). This flag will still force to call the
> > _PSC/_ON/_OFF method even when it is already the same as the target
> > state. Maybe this is to workaround the BIOS issue that the power state
> > is not reported correctly first time.
> > 
> >    Not sure whether my above understanding is reasonable.
> 
> The force_power_state was _only_ a workaround for the case when
> acpi_bus_set_power() called acpi_bus_get_power() and apparently received
> a wrong value from it on some systems (two of them had been identified, one was
> blacklisted in the end).  However, after my changes acpi_bus_set_power()
> doesn't call acpi_bus_get_power() (or any equivalent) any more, so this
> workaround isn't necessary any more either.

The flag of force_power_state will force it to do the transition of
power state even when the target state is already the same as the
current power state. Without this flag, it will skip the power
transition if the two states are consistent. This is useful under the
following cases: (The FAN device is used as an example)
    1. The initial state is reported as D0 state (But we don't know
whether the FAN device is really turned on)
    2. If we expect to turn on the corresponding FAN device for the
first time(put it into D0 state),  it will skip the power transition as
the check of "state == device->power.state". In such case it will be
better that the power transition is forced in order to assure that the
FAN device is turned on by calling the _PSC/_ON object. 

Thanks.
    Yakui 

> 
> Thanks,
> Rafael

--
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

Re: [Resend][PATCH 0/3] ACPI / PM: Patches missing from linux-acpi-2.6/test

[Rafael J. Wysocki]

On Wednesday, December 15, 2010, ykzhao wrote:
> On Wed, 2010-12-15 at 04:24 +0800, Rafael J. Wysocki wrote:
> > On Tuesday, December 14, 2010, ykzhao wrote:
> > > On Tue, 2010-12-14 at 05:25 +0800, Rafael J. Wysocki wrote:
> > > > On Monday, December 13, 2010, ykzhao wrote:
> > > > > On Sun, 2010-12-12 at 06:39 +0800, Rafael J. Wysocki wrote:
...
> > 
> > The force_power_state was _only_ a workaround for the case when
> > acpi_bus_set_power() called acpi_bus_get_power() and apparently received
> > a wrong value from it on some systems (two of them had been identified, one was
> > blacklisted in the end).  However, after my changes acpi_bus_set_power()
> > doesn't call acpi_bus_get_power() (or any equivalent) any more, so this
> > workaround isn't necessary any more either.
> 
> The flag of force_power_state will force it to do the transition of
> power state even when the target state is already the same as the
> current power state. Without this flag, it will skip the power
> transition if the two states are consistent. This is useful under the
> following cases: (The FAN device is used as an example)
>     1. The initial state is reported as D0 state (But we don't know
> whether the FAN device is really turned on)
>     2. If we expect to turn on the corresponding FAN device for the
> first time(put it into D0 state),  it will skip the power transition as
> the check of "state == device->power.state". In such case it will be
> better that the power transition is forced in order to assure that the
> FAN device is turned on by calling the _PSC/_ON object. 

Before my changes the force_power_state device flag was used only by the fan
driver in two places: in acpi_fan_add() (ie. during fan initialization) and in
acpi_fan_resume().  In both cases it was used to make acpi_bus_set_power()
accept the state read by acpi_bus_get_power().  In both cases
acpi_bus_update_power() can be used to obtain _exactly_ the same result,
so the flag is not necessary any more.

Thanks,
Rafael

Re: [Resend][PATCH 0/3] ACPI / PM: Patches missing from linux-acpi-2.6/test

[Len Brown]

> Hi Len,
> 
> The following three patches seem to have been dropped from your 'test' branch.
> 
> If that happened by accident, please reapply.  Otherwise, please let me know
> what's wrong with the patches so that I can fix them.
> 
> [1/3] - Make fujitsu_laptop use acpi_bus_update_power() instead of
>         acpi_bus_get_power() which is unsafe.
> 
> [2/3] - Drop acpi_bus_get_power() which is unsafe and has no users.
> 
> [3/3] - Do not call __acpi_bus_get_power() from acpi_bus_set_power()
>         and remove acpi_power_nocheck that is not necessary any more.
> 
> Without these patches the power resources handling rework is incomplete and
> the code will not work correctly in some situations. 

Looks like my screw-up.

I recall inserting the fujitsu patch to fix the 
build before what were originally 12/13 and 13/13 of this series,
but apparently I kept the baseline branch and deleted the fix on top of it.

Yes, in general, I do squawk when I drop patches from acpi-test,
even if it is a temporary drop due to a time-consuming manual merge.

So thanks for noticing, I'll look at these again now.

Len Brown
Intel Open Source Technology Center

Re: [Resend][PATCH 0/3] ACPI / PM: Patches missing from linux-acpi-2.6/test

[Rafael J. Wysocki]

On Monday, December 13, 2010, Len Brown wrote:
> > Hi Len,
> > 
> > The following three patches seem to have been dropped from your 'test' branch.
> > 
> > If that happened by accident, please reapply.  Otherwise, please let me know
> > what's wrong with the patches so that I can fix them.
> > 
> > [1/3] - Make fujitsu_laptop use acpi_bus_update_power() instead of
> >         acpi_bus_get_power() which is unsafe.
> > 
> > [2/3] - Drop acpi_bus_get_power() which is unsafe and has no users.
> > 
> > [3/3] - Do not call __acpi_bus_get_power() from acpi_bus_set_power()
> >         and remove acpi_power_nocheck that is not necessary any more.
> > 
> > Without these patches the power resources handling rework is incomplete and
> > the code will not work correctly in some situations. 
> 
> Looks like my screw-up.
> 
> I recall inserting the fujitsu patch to fix the 
> build before what were originally 12/13 and 13/13 of this series,
> but apparently I kept the baseline branch and deleted the fix on top of it.
> 
> Yes, in general, I do squawk when I drop patches from acpi-test,
> even if it is a temporary drop due to a time-consuming manual merge.
> 
> So thanks for noticing, I'll look at these again now.

Thanks!

Rafael