[PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Aaron Lu]

When entering D3 Cold from a higher device state, evaluate _PS3 first
and then make the proper power transition.
This is used to solve the ZPODD problem on AMD's platform, _PS3 on such
platforms will power off the ODD device and thus make the device enter
D3 cold state.

Signed-off-by: Aaron Lu <aaron.lu@amd.com>
Cc: Andiry Xu <andiry.xu@amd.com>
Cc: Alex He <alex.he@amd.com>
---
 drivers/acpi/bus.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 3263b68..68e593f 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -227,6 +227,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
 	int result = 0;
 	acpi_status status = AE_OK;
 	char object_name[5] = { '_', 'P', 'S', '0' + state, '\0' };
+	struct acpi_device_power_state *ps;
+	u8 explicit_set;
 
 	if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
 		return -EINVAL;
@@ -239,7 +241,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
 		return 0;
 	}
 
-	if (!device->power.states[state].flags.valid) {
+	ps = &device->power.states[state];
+	if (!ps->flags.valid) {
 		printk(KERN_WARNING PREFIX "Device does not support D%d\n", state);
 		return -ENODEV;
 	}
@@ -263,7 +266,7 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
 			if (result)
 				goto end;
 		}
-		if (device->power.states[state].flags.explicit_set) {
+		if (ps->flags.explicit_set) {
 			status = acpi_evaluate_object(device->handle,
 						      object_name, NULL, NULL);
 			if (ACPI_FAILURE(status)) {
@@ -272,7 +275,15 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
 			}
 		}
 	} else {
-		if (device->power.states[state].flags.explicit_set) {
+		/* If state is D3 Cold, try to evaluate _PS3 first */
+		if (state == ACPI_STATE_D3_COLD) {
+			explicit_set = (ps - 1)->flags.explicit_set;
+			object_name[3] -= 1;
+		}
+		else {
+			explicit_set = ps->flags.explicit_set;
+		}
+		if (explicit_set) {
 			status = acpi_evaluate_object(device->handle,
 						      object_name, NULL, NULL);
 			if (ACPI_FAILURE(status)) {
-- 
1.7.9.2.315.g25a78.dirty

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Lin Ming]

On Sun, 2012-04-01 at 02:18 +0800, Aaron Lu wrote:
> When entering D3 Cold from a higher device state, evaluate _PS3 first
> and then make the proper power transition.
> This is used to solve the ZPODD problem on AMD's platform, _PS3 on such
> platforms will power off the ODD device and thus make the device enter
> D3 cold state.
> 
> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> Cc: Andiry Xu <andiry.xu@amd.com>
> Cc: Alex He <alex.he@amd.com>
> ---
>  drivers/acpi/bus.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 3263b68..68e593f 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -227,6 +227,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>  	int result = 0;
>  	acpi_status status = AE_OK;
>  	char object_name[5] = { '_', 'P', 'S', '0' + state, '\0' };
> +	struct acpi_device_power_state *ps;
> +	u8 explicit_set;
>  
>  	if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
>  		return -EINVAL;
> @@ -239,7 +241,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>  		return 0;
>  	}
>  
> -	if (!device->power.states[state].flags.valid) {
> +	ps = &device->power.states[state];
> +	if (!ps->flags.valid) {
>  		printk(KERN_WARNING PREFIX "Device does not support D%d\n", state);
>  		return -ENODEV;
>  	}
> @@ -263,7 +266,7 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>  			if (result)
>  				goto end;
>  		}
> -		if (device->power.states[state].flags.explicit_set) {
> +		if (ps->flags.explicit_set) {
>  			status = acpi_evaluate_object(device->handle,
>  						      object_name, NULL, NULL);
>  			if (ACPI_FAILURE(status)) {
> @@ -272,7 +275,15 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>  			}
>  		}
>  	} else {
> -		if (device->power.states[state].flags.explicit_set) {
> +		/* If state is D3 Cold, try to evaluate _PS3 first */
> +		if (state == ACPI_STATE_D3_COLD) {
> +			explicit_set = (ps - 1)->flags.explicit_set;
> +			object_name[3] -= 1;
> +		}

I'm not sure whether this works or not.

>From ACPI spec,

_PS3 "is used to put the specific device into its D3hot or D3 state"

D3 neither means D3hot nor D3cold. It's an old term before D3hot and
D3cold were introduced.

Another problem:

With your patch, both D3hot and D3cold will evaluate _PS3, right?

Will it have problem on AMD platform if you try to put ODD into D3hot
state? _PS3 is evaluated, so it actually enters D3Cold state.

Thanks,
Lin Ming

> +		else {
> +			explicit_set = ps->flags.explicit_set;
> +		}
> +		if (explicit_set) {
>  			status = acpi_evaluate_object(device->handle,
>  						      object_name, NULL, NULL);
>  			if (ACPI_FAILURE(status)) {

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Aaron Lu]

Hi,

On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> > -		if (device->power.states[state].flags.explicit_set) {
> > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > +		if (state == ACPI_STATE_D3_COLD) {
> > +			explicit_set = (ps - 1)->flags.explicit_set;
> > +			object_name[3] -= 1;
> > +		}
> 
> I'm not sure whether this works or not.
> 
> From ACPI spec,
> 
> _PS3 "is used to put the specific device into its D3hot or D3 state"
> 
> D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> D3cold were introduced.
I guess D3 has to mean something, right? :-)

Here is the problem, there is no _PR3 in AMD's implementation, just _PS3.
And since _S0W evaluates 4, I've to put this device into D3 cold state
with _PS3.

And the ACPI does have some words like:

------
Platform/drivers must assume that the device will have power completely
removed when the device is place into “D3” via _PS3
------

This is in section 7.2.11: _PR3.

> 
> Another problem:
> 
> With your patch, both D3hot and D3cold will evaluate _PS3, right?
> 
Yes.

> Will it have problem on AMD platform if you try to put ODD into D3hot
> state? _PS3 is evaluated, so it actually enters D3Cold state.

There is no D3 hot support for this device(from the firmware's
perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
_PS3).


Thanks for the review.

-Aaron

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Lin Ming]

On Sun, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
> Hi,
> 
> On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> > > -		if (device->power.states[state].flags.explicit_set) {
> > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > +		if (state == ACPI_STATE_D3_COLD) {
> > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > +			object_name[3] -= 1;
> > > +		}
> > 
> > I'm not sure whether this works or not.
> > 
> > From ACPI spec,
> > 
> > _PS3 "is used to put the specific device into its D3hot or D3 state"
> > 
> > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> > D3cold were introduced.
> I guess D3 has to mean something, right? :-)
> 
> Here is the problem, there is no _PR3 in AMD's implementation, just _PS3.
> And since _S0W evaluates 4, I've to put this device into D3 cold state
> with _PS3.
> 
> And the ACPI does have some words like:
> 
> ------
> Platform/drivers must assume that the device will have power completely
> removed when the device is place into “D3” via _PS3
> ------
> 
> This is in section 7.2.11: _PR3.
> 
> > 
> > Another problem:
> > 
> > With your patch, both D3hot and D3cold will evaluate _PS3, right?
> > 
> Yes.
> 
> > Will it have problem on AMD platform if you try to put ODD into D3hot
> > state? _PS3 is evaluated, so it actually enters D3Cold state.
> 
> There is no D3 hot support for this device(from the firmware's
> perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> _PS3).

But this is the generic code. We can't only consider some special
device.

Maybe we need some flag to tell which D3 state _PS3 is used for.

Lin Ming

> 
> 
> Thanks for the review.
> 
> -Aaron
> 

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

Hi,

Sorry for the delayed response, I've been travelling recently.

On Sunday, April 01, 2012, Lin Ming wrote:
> On Sun, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
> > Hi,
> > 
> > On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> > > > -		if (device->power.states[state].flags.explicit_set) {
> > > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > > +		if (state == ACPI_STATE_D3_COLD) {
> > > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > > +			object_name[3] -= 1;
> > > > +		}
> > > 
> > > I'm not sure whether this works or not.
> > > 
> > > From ACPI spec,
> > > 
> > > _PS3 "is used to put the specific device into its D3hot or D3 state"
> > > 
> > > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> > > D3cold were introduced.
> > I guess D3 has to mean something, right? :-)

Well, not necessarily.

The problem is what state the _PS3 method puts the device into: D3_hot or
D3_cold.

Unfortunately, as far as I can say, ACPI 4.0 didn't specify any "official"
mapping between the "old" D3 and the "new" D3_{hod|cold} states, so we need to
figure out something.  In my opinion, the only reasonable approach is to
assume that the state _PS3 puts the device into is always D3_cold, becuase
_PS3 may remove power completely from the device.  It may not do that, but
we _must_ assume it does that in general.

> > Here is the problem, there is no _PR3 in AMD's implementation, just _PS3.
> > And since _S0W evaluates 4, I've to put this device into D3 cold state
> > with _PS3.
> > 
> > And the ACPI does have some words like:
> > 
> > ------
> > Platform/drivers must assume that the device will have power completely
> > removed when the device is place into “D3” via _PS3

Exactly.  What it means is basically "always reinitialize the device from
scratch if you have run _PS3 on it".  And that's what we should do.

> > ------
> > 
> > This is in section 7.2.11: _PR3.
> > 
> > > 
> > > Another problem:
> > > 
> > > With your patch, both D3hot and D3cold will evaluate _PS3, right?
> > > 
> > Yes.
> > 
> > > Will it have problem on AMD platform if you try to put ODD into D3hot
> > > state? _PS3 is evaluated, so it actually enters D3Cold state.
> > 
> > There is no D3 hot support for this device(from the firmware's
> > perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> > _PS3).
> 
> But this is the generic code. We can't only consider some special
> device.
> 
> Maybe we need some flag to tell which D3 state _PS3 is used for.

No, please.  As I said above, we need to reinitialize devices that _PS3 was
executed on, which is equivalent to saying that those devices were put into
D3_cold.

The only situation where a device can be put into ACPI D3_hot (which is not
the same as PCI D3_hot, mind you) is when:

(1) There is _PR3 listing some of the device's power resources as "on".
(2) The power resources listed by the _PR3 as "off" are turned off and the
    power resources listed by the _PR3 as "on" are left in the "on" state.

Our generic code should reflect that.

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Zhang Rui]

On 日, 2012-04-01 at 09:23 +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> Sorry for the delayed response, I've been travelling recently.
> 
> On Sunday, April 01, 2012, Lin Ming wrote:
> > On Sun, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
> > > Hi,
> > > 
> > > On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> > > > > -		if (device->power.states[state].flags.explicit_set) {
> > > > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > > > +		if (state == ACPI_STATE_D3_COLD) {
> > > > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > > > +			object_name[3] -= 1;
> > > > > +		}
> > > > 
> > > > I'm not sure whether this works or not.
> > > > 
> > > > From ACPI spec,
> > > > 
> > > > _PS3 "is used to put the specific device into its D3hot or D3 state"
> > > > 
> > > > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> > > > D3cold were introduced.
> > > I guess D3 has to mean something, right? :-)
> 
> Well, not necessarily.
> 
> The problem is what state the _PS3 method puts the device into: D3_hot or
> D3_cold.
> 
> Unfortunately, as far as I can say, ACPI 4.0 didn't specify any "official"
> mapping between the "old" D3 and the "new" D3_{hod|cold} states, so we need to
> figure out something.  In my opinion, the only reasonable approach is to
> assume that the state _PS3 puts the device into is always D3_cold, becuase
> _PS3 may remove power completely from the device.  It may not do that, but
> we _must_ assume it does that in general.
> 
There is a problem that I can think of.
Say currently, ACPI always returns D3 when _PS3 exists.
And this "ACPI_STATE_D3" is translated to PCI_D3hot.
But with this approach, we're going to put these devices to PCI_D3cold
instead, right?
I'm not against this approach, but this may affect a lot of PCI devices,
which we need to take care of, no?

thanks,
rui

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Sunday, April 01, 2012, Zhang Rui wrote:
> On 日, 2012-04-01 at 09:23 +0200, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > Sorry for the delayed response, I've been travelling recently.
> > 
> > On Sunday, April 01, 2012, Lin Ming wrote:
> > > On Sun, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
> > > > Hi,
> > > > 
> > > > On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> > > > > > -		if (device->power.states[state].flags.explicit_set) {
> > > > > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > > > > +		if (state == ACPI_STATE_D3_COLD) {
> > > > > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > > > > +			object_name[3] -= 1;
> > > > > > +		}
> > > > > 
> > > > > I'm not sure whether this works or not.
> > > > > 
> > > > > From ACPI spec,
> > > > > 
> > > > > _PS3 "is used to put the specific device into its D3hot or D3 state"
> > > > > 
> > > > > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> > > > > D3cold were introduced.
> > > > I guess D3 has to mean something, right? :-)
> > 
> > Well, not necessarily.
> > 
> > The problem is what state the _PS3 method puts the device into: D3_hot or
> > D3_cold.
> > 
> > Unfortunately, as far as I can say, ACPI 4.0 didn't specify any "official"
> > mapping between the "old" D3 and the "new" D3_{hod|cold} states, so we need to
> > figure out something.  In my opinion, the only reasonable approach is to
> > assume that the state _PS3 puts the device into is always D3_cold, becuase
> > _PS3 may remove power completely from the device.  It may not do that, but
> > we _must_ assume it does that in general.
> > 
> There is a problem that I can think of.
> Say currently, ACPI always returns D3 when _PS3 exists.

I'm not sure what you mean exactly, but please see below.

> And this "ACPI_STATE_D3" is translated to PCI_D3hot.
> But with this approach, we're going to put these devices to PCI_D3cold
> instead, right?
> I'm not against this approach, but this may affect a lot of PCI devices,
> which we need to take care of, no?

Do you mean acpi_pm_device_sleep_state() should be modified?  It doesn't
refer to whether or not _PS3 exists, as far as I can say.

In the acpi_pci_set_power_state() code path, on the other hand, there are
two potentially problematic situations when PCI wants to put the device into
PCI_D3hot (which need not map 1:1 onto ACPI D3_hot), one of which is when _PS3
is present, and the other is when neither _PS3, nor _PR3 is present.

If _PS3 is present, then _PR3 may or may not be present.  In the latter case
we can only execute _PS3 in the hope it does the right thing, but as long
as we restore the device's configuration registers while resuming it (which is
done by all of our PCI device resume callback routines as far as I can say),
the only possible difference is the resume latency (which may be greater if
power is removed from the device entirely).  However, in that case we shouldn't
turn off the device's power resources after _PS3 has been executed (if we
turned them off, power would be removed from the device, which wouldn't be
what PCI wanted).  So, to handle this particular case we need to pass
ACPI_STATE_D3_HOT to acpi_bus_set_power(), meaning "avoid going into D3_cold,
if possible".

In both _PS3 and _PR3 are present, we should evaluate _PS3 and then turn off
the power resources listed as "off" by _PR3 (and turn on the power resoruces
listed by it as "on"), but we need to restore the configuration registers of
the device while resuming it.  I think this is handled correctly without
modifications.

If neither _PS3 nor _PR3 is present, we shouldn't turn off the device's
power resources, because PCI doesn't want power to be removed from the device.

In summary, if PCI wants the device to be put into PCI_D3hot and _PS3 is
present, we should evaluate _PS3.  However, we shouldn't turn the device's
power resources off unless _PR3 is present, in which case we can turn off
the power resources listed by it as "off".

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[huang ying]

Hi, Rafael,

On Sun, Apr 1, 2012 at 4:49 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday, April 01, 2012, Zhang Rui wrote:
>> On 日, 2012-04-01 at 09:23 +0200, Rafael J. Wysocki wrote:
>> > Hi,
>> >
>> > Sorry for the delayed response, I've been travelling recently.
>> >
>> > On Sunday, April 01, 2012, Lin Ming wrote:
>> > > On Sun, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
>> > > > Hi,
>> > > >
>> > > > On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
>> > > > > > -           if (device->power.states[state].flags.explicit_set) {
>> > > > > > +           /* If state is D3 Cold, try to evaluate _PS3 first */
>> > > > > > +           if (state == ACPI_STATE_D3_COLD) {
>> > > > > > +                   explicit_set = (ps - 1)->flags.explicit_set;
>> > > > > > +                   object_name[3] -= 1;
>> > > > > > +           }
>> > > > >
>> > > > > I'm not sure whether this works or not.
>> > > > >
>> > > > > From ACPI spec,
>> > > > >
>> > > > > _PS3 "is used to put the specific device into its D3hot or D3 state"
>> > > > >
>> > > > > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
>> > > > > D3cold were introduced.
>> > > > I guess D3 has to mean something, right? :-)
>> >
>> > Well, not necessarily.
>> >
>> > The problem is what state the _PS3 method puts the device into: D3_hot or
>> > D3_cold.
>> >
>> > Unfortunately, as far as I can say, ACPI 4.0 didn't specify any "official"
>> > mapping between the "old" D3 and the "new" D3_{hod|cold} states, so we need to
>> > figure out something.  In my opinion, the only reasonable approach is to
>> > assume that the state _PS3 puts the device into is always D3_cold, becuase
>> > _PS3 may remove power completely from the device.  It may not do that, but
>> > we _must_ assume it does that in general.
>> >
>> There is a problem that I can think of.
>> Say currently, ACPI always returns D3 when _PS3 exists.
>
> I'm not sure what you mean exactly, but please see below.
>
>> And this "ACPI_STATE_D3" is translated to PCI_D3hot.
>> But with this approach, we're going to put these devices to PCI_D3cold
>> instead, right?
>> I'm not against this approach, but this may affect a lot of PCI devices,
>> which we need to take care of, no?
>
> Do you mean acpi_pm_device_sleep_state() should be modified?  It doesn't
> refer to whether or not _PS3 exists, as far as I can say.
>
> In the acpi_pci_set_power_state() code path, on the other hand, there are
> two potentially problematic situations when PCI wants to put the device into
> PCI_D3hot (which need not map 1:1 onto ACPI D3_hot), one of which is when _PS3
> is present, and the other is when neither _PS3, nor _PR3 is present.
>
> If _PS3 is present, then _PR3 may or may not be present.  In the latter case
> we can only execute _PS3 in the hope it does the right thing, but as long
> as we restore the device's configuration registers while resuming it (which is
> done by all of our PCI device resume callback routines as far as I can say),
> the only possible difference is the resume latency (which may be greater if
> power is removed from the device entirely).

Another difference between D3Hot and D3Cold for PCI devices is config
space availability.  That is, in D3Hot, you can access D3Hot, while in
D3Cold you can not do that.  For example, PME poll logic need to be
disabled if we put device into D3Cold.

> However, in that case we shouldn't
> turn off the device's power resources after _PS3 has been executed (if we
> turned them off, power would be removed from the device, which wouldn't be
> what PCI wanted).  So, to handle this particular case we need to pass
> ACPI_STATE_D3_HOT to acpi_bus_set_power(), meaning "avoid going into D3_cold,
> if possible".
>
> In both _PS3 and _PR3 are present, we should evaluate _PS3 and then turn off
> the power resources listed as "off" by _PR3 (and turn on the power resoruces
> listed by it as "on"), but we need to restore the configuration registers of
> the device while resuming it.  I think this is handled correctly without
> modifications.
>
> If neither _PS3 nor _PR3 is present, we shouldn't turn off the device's
> power resources, because PCI doesn't want power to be removed from the device.

For PCI device plugged into system via slot (not integrated into PCH
or motherboard), there is no ACPI handle associate with it, so that
there are neither _PS3 nor _PR3 presented.  But it is still possible
to turn off the device power via the associated PCIe port, which has
_PS3 and/or _PR3 presented.  I think that situation is reasonable too.

> In summary, if PCI wants the device to be put into PCI_D3hot and _PS3 is
> present, we should evaluate _PS3.  However, we shouldn't turn the device's
> power resources off unless _PR3 is present, in which case we can turn off
> the power resources listed by it as "off".

How to turn the device's power resources off without _PR3?  It may be
possible via PCIe port as I said above.  Do you mean that?  Or
something else?

Best Regards,
Huang Ying

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Thursday, April 05, 2012, huang ying wrote:
> Hi, Rafael,

Hi,

> On Sun, Apr 1, 2012 at 4:49 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday, April 01, 2012, Zhang Rui wrote:
> >> On 日, 2012-04-01 at 09:23 +0200, Rafael J. Wysocki wrote:
> >> > Hi,
> >> >
> >> > Sorry for the delayed response, I've been travelling recently.
> >> >
> >> > On Sunday, April 01, 2012, Lin Ming wrote:
> >> > > On Sun, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
> >> > > > Hi,
> >> > > >
> >> > > > On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> >> > > > > > -           if (device->power.states[state].flags.explicit_set) {
> >> > > > > > +           /* If state is D3 Cold, try to evaluate _PS3 first */
> >> > > > > > +           if (state == ACPI_STATE_D3_COLD) {
> >> > > > > > +                   explicit_set = (ps - 1)->flags.explicit_set;
> >> > > > > > +                   object_name[3] -= 1;
> >> > > > > > +           }
> >> > > > >
> >> > > > > I'm not sure whether this works or not.
> >> > > > >
> >> > > > > From ACPI spec,
> >> > > > >
> >> > > > > _PS3 "is used to put the specific device into its D3hot or D3 state"
> >> > > > >
> >> > > > > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> >> > > > > D3cold were introduced.
> >> > > > I guess D3 has to mean something, right? :-)
> >> >
> >> > Well, not necessarily.
> >> >
> >> > The problem is what state the _PS3 method puts the device into: D3_hot or
> >> > D3_cold.
> >> >
> >> > Unfortunately, as far as I can say, ACPI 4.0 didn't specify any "official"
> >> > mapping between the "old" D3 and the "new" D3_{hod|cold} states, so we need to
> >> > figure out something.  In my opinion, the only reasonable approach is to
> >> > assume that the state _PS3 puts the device into is always D3_cold, becuase
> >> > _PS3 may remove power completely from the device.  It may not do that, but
> >> > we _must_ assume it does that in general.
> >> >
> >> There is a problem that I can think of.
> >> Say currently, ACPI always returns D3 when _PS3 exists.
> >
> > I'm not sure what you mean exactly, but please see below.
> >
> >> And this "ACPI_STATE_D3" is translated to PCI_D3hot.
> >> But with this approach, we're going to put these devices to PCI_D3cold
> >> instead, right?
> >> I'm not against this approach, but this may affect a lot of PCI devices,
> >> which we need to take care of, no?
> >
> > Do you mean acpi_pm_device_sleep_state() should be modified?  It doesn't
> > refer to whether or not _PS3 exists, as far as I can say.
> >
> > In the acpi_pci_set_power_state() code path, on the other hand, there are
> > two potentially problematic situations when PCI wants to put the device into
> > PCI_D3hot (which need not map 1:1 onto ACPI D3_hot), one of which is when _PS3
> > is present, and the other is when neither _PS3, nor _PR3 is present.
> >
> > If _PS3 is present, then _PR3 may or may not be present.  In the latter case
> > we can only execute _PS3 in the hope it does the right thing, but as long
> > as we restore the device's configuration registers while resuming it (which is
> > done by all of our PCI device resume callback routines as far as I can say),
> > the only possible difference is the resume latency (which may be greater if
> > power is removed from the device entirely).
> 
> Another difference between D3Hot and D3Cold for PCI devices is config
> space availability.  That is, in D3Hot, you can access D3Hot, while in
> D3Cold you can not do that.  For example, PME poll logic need to be
> disabled if we put device into D3Cold.

We're not talking about PCI here.  PCI D3hot/D3cold is actually well defined,
while the ACPI "couterparts" aren't.  And BTW I know the properties of the PCI
power management states. :-)

> > However, in that case we shouldn't
> > turn off the device's power resources after _PS3 has been executed (if we
> > turned them off, power would be removed from the device, which wouldn't be
> > what PCI wanted).  So, to handle this particular case we need to pass
> > ACPI_STATE_D3_HOT to acpi_bus_set_power(), meaning "avoid going into D3_cold,
> > if possible".
> >
> > In both _PS3 and _PR3 are present, we should evaluate _PS3 and then turn off
> > the power resources listed as "off" by _PR3 (and turn on the power resoruces
> > listed by it as "on"), but we need to restore the configuration registers of
> > the device while resuming it.  I think this is handled correctly without
> > modifications.
> >
> > If neither _PS3 nor _PR3 is present, we shouldn't turn off the device's
> > power resources, because PCI doesn't want power to be removed from the device.
> 
> For PCI device plugged into system via slot (not integrated into PCH
> or motherboard), there is no ACPI handle associate with it, so that
> there are neither _PS3 nor _PR3 presented.  But it is still possible
> to turn off the device power via the associated PCIe port, which has
> _PS3 and/or _PR3 presented.  I think that situation is reasonable too.

Again, this hasn't anything to do with ACPI.

We're discussing standard interfaces exposed by ACPI.  That is, if I say
"to turn of the device's power resources" I mean to call _OFF for all of the
power resources listed by _PR0 for that device.  Nothing more or less than
that.

> > In summary, if PCI wants the device to be put into PCI_D3hot and _PS3 is
> > present, we should evaluate _PS3.  However, we shouldn't turn the device's
> > power resources off unless _PR3 is present, in which case we can turn off
> > the power resources listed by it as "off".
> 
> How to turn the device's power resources off without _PR3?  It may be
> possible via PCIe port as I said above.  Do you mean that?  Or
> something else?

If _PR0 is present, it returns the list of power resources needed by the
device.  If you turn them all off, the assumption is that power has been
removed from the device, so it is in D3(cold).

PCI hotplug (and other standards) provide alternative means for removing
power from devices, but that's beyond the scope of what is being discussed
in this thread.

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Huang Ying]

On Mon, 2012-04-09 at 01:41 +0200, Rafael J. Wysocki wrote:
> > >
> > > If _PS3 is present, then _PR3 may or may not be present.  In the latter case
> > > we can only execute _PS3 in the hope it does the right thing, but as long
> > > as we restore the device's configuration registers while resuming it (which is
> > > done by all of our PCI device resume callback routines as far as I can say),
> > > the only possible difference is the resume latency (which may be greater if
> > > power is removed from the device entirely).
> > 
> > Another difference between D3Hot and D3Cold for PCI devices is config
> > space availability.  That is, in D3Hot, you can access D3Hot, while in
> > D3Cold you can not do that.  For example, PME poll logic need to be
> > disabled if we put device into D3Cold.
> 
> We're not talking about PCI here.  PCI D3hot/D3cold is actually well defined,
> while the ACPI "couterparts" aren't.  And BTW I know the properties of the PCI
> power management states. :-)

I see.

> > > However, in that case we shouldn't
> > > turn off the device's power resources after _PS3 has been executed (if we
> > > turned them off, power would be removed from the device, which wouldn't be
> > > what PCI wanted).  So, to handle this particular case we need to pass
> > > ACPI_STATE_D3_HOT to acpi_bus_set_power(), meaning "avoid going into D3_cold,
> > > if possible".
> > >
> > > In both _PS3 and _PR3 are present, we should evaluate _PS3 and then turn off
> > > the power resources listed as "off" by _PR3 (and turn on the power resoruces
> > > listed by it as "on"), but we need to restore the configuration registers of
> > > the device while resuming it.  I think this is handled correctly without
> > > modifications.
> > >
> > > If neither _PS3 nor _PR3 is present, we shouldn't turn off the device's
> > > power resources, because PCI doesn't want power to be removed from the device.
> > 
> > For PCI device plugged into system via slot (not integrated into PCH
> > or motherboard), there is no ACPI handle associate with it, so that
> > there are neither _PS3 nor _PR3 presented.  But it is still possible
> > to turn off the device power via the associated PCIe port, which has
> > _PS3 and/or _PR3 presented.  I think that situation is reasonable too.
> 
> Again, this hasn't anything to do with ACPI.
> 
> We're discussing standard interfaces exposed by ACPI.  That is, if I say
> "to turn of the device's power resources" I mean to call _OFF for all of the
> power resources listed by _PR0 for that device.  Nothing more or less than
> that.
> 
> > > In summary, if PCI wants the device to be put into PCI_D3hot and _PS3 is
> > > present, we should evaluate _PS3.  However, we shouldn't turn the device's
> > > power resources off unless _PR3 is present, in which case we can turn off
> > > the power resources listed by it as "off".
> > 
> > How to turn the device's power resources off without _PR3?  It may be
> > possible via PCIe port as I said above.  Do you mean that?  Or
> > something else?
> 
> If _PR0 is present, it returns the list of power resources needed by the
> device.  If you turn them all off, the assumption is that power has been
> removed from the device, so it is in D3(cold).

So.  If PCI wants the device to be put into PCI_D3hot and _PS3 is
present.  We should evaluate _PS3 AND set the pci_dev->current_state to
PCI_D3cold?

Best Regards,
Huang Ying

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Monday, April 09, 2012, Huang Ying wrote:
> On Mon, 2012-04-09 at 01:41 +0200, Rafael J. Wysocki wrote:
> > > >
> > > > If _PS3 is present, then _PR3 may or may not be present.  In the latter case
> > > > we can only execute _PS3 in the hope it does the right thing, but as long
> > > > as we restore the device's configuration registers while resuming it (which is
> > > > done by all of our PCI device resume callback routines as far as I can say),
> > > > the only possible difference is the resume latency (which may be greater if
> > > > power is removed from the device entirely).
> > > 
> > > Another difference between D3Hot and D3Cold for PCI devices is config
> > > space availability.  That is, in D3Hot, you can access D3Hot, while in
> > > D3Cold you can not do that.  For example, PME poll logic need to be
> > > disabled if we put device into D3Cold.
> > 
> > We're not talking about PCI here.  PCI D3hot/D3cold is actually well defined,
> > while the ACPI "couterparts" aren't.  And BTW I know the properties of the PCI
> > power management states. :-)
> 
> I see.
> 
> > > > However, in that case we shouldn't
> > > > turn off the device's power resources after _PS3 has been executed (if we
> > > > turned them off, power would be removed from the device, which wouldn't be
> > > > what PCI wanted).  So, to handle this particular case we need to pass
> > > > ACPI_STATE_D3_HOT to acpi_bus_set_power(), meaning "avoid going into D3_cold,
> > > > if possible".
> > > >
> > > > In both _PS3 and _PR3 are present, we should evaluate _PS3 and then turn off
> > > > the power resources listed as "off" by _PR3 (and turn on the power resoruces
> > > > listed by it as "on"), but we need to restore the configuration registers of
> > > > the device while resuming it.  I think this is handled correctly without
> > > > modifications.
> > > >
> > > > If neither _PS3 nor _PR3 is present, we shouldn't turn off the device's
> > > > power resources, because PCI doesn't want power to be removed from the device.
> > > 
> > > For PCI device plugged into system via slot (not integrated into PCH
> > > or motherboard), there is no ACPI handle associate with it, so that
> > > there are neither _PS3 nor _PR3 presented.  But it is still possible
> > > to turn off the device power via the associated PCIe port, which has
> > > _PS3 and/or _PR3 presented.  I think that situation is reasonable too.
> > 
> > Again, this hasn't anything to do with ACPI.
> > 
> > We're discussing standard interfaces exposed by ACPI.  That is, if I say
> > "to turn of the device's power resources" I mean to call _OFF for all of the
> > power resources listed by _PR0 for that device.  Nothing more or less than
> > that.
> > 
> > > > In summary, if PCI wants the device to be put into PCI_D3hot and _PS3 is
> > > > present, we should evaluate _PS3.  However, we shouldn't turn the device's
> > > > power resources off unless _PR3 is present, in which case we can turn off
> > > > the power resources listed by it as "off".
> > > 
> > > How to turn the device's power resources off without _PR3?  It may be
> > > possible via PCIe port as I said above.  Do you mean that?  Or
> > > something else?
> > 
> > If _PR0 is present, it returns the list of power resources needed by the
> > device.  If you turn them all off, the assumption is that power has been
> > removed from the device, so it is in D3(cold).
> 
> So.  If PCI wants the device to be put into PCI_D3hot and _PS3 is
> present.  We should evaluate _PS3 AND set the pci_dev->current_state to
> PCI_D3cold?

No, I believe we should use the standard PCI PM interface to put the device
into PCI_D3hot and skip the execution of _PS3 (which is not what we do now,
but that's only because acpi_pci_set_power_state() doesn't distinguish
ACPI D3_hot from ACPI D3_cold).

If the standard PCI PM interface is not supported, the PCI_D3hot state is not
well defined for the device.

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Lin Ming]

On Sun, 2012-04-01 at 09:23 +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> Sorry for the delayed response, I've been travelling recently.
> 
> On Sunday, April 01, 2012, Lin Ming wrote:
> > On Sun, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
> > > Hi,
> > > 
> > > On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> > > > > -		if (device->power.states[state].flags.explicit_set) {
> > > > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > > > +		if (state == ACPI_STATE_D3_COLD) {
> > > > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > > > +			object_name[3] -= 1;
> > > > > +		}
> > > > 
> > > > I'm not sure whether this works or not.
> > > > 
> > > > From ACPI spec,
> > > > 
> > > > _PS3 "is used to put the specific device into its D3hot or D3 state"
> > > > 
> > > > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> > > > D3cold were introduced.
> > > I guess D3 has to mean something, right? :-)
> 
> Well, not necessarily.
> 
> The problem is what state the _PS3 method puts the device into: D3_hot or
> D3_cold.
> 
> Unfortunately, as far as I can say, ACPI 4.0 didn't specify any "official"
> mapping between the "old" D3 and the "new" D3_{hod|cold} states, so we need to
> figure out something.  In my opinion, the only reasonable approach is to
> assume that the state _PS3 puts the device into is always D3_cold, becuase
> _PS3 may remove power completely from the device.  It may not do that, but
> we _must_ assume it does that in general.
> 
> > > Here is the problem, there is no _PR3 in AMD's implementation, just _PS3.
> > > And since _S0W evaluates 4, I've to put this device into D3 cold state
> > > with _PS3.
> > > 
> > > And the ACPI does have some words like:
> > > 
> > > ------
> > > Platform/drivers must assume that the device will have power completely
> > > removed when the device is place into “D3” via _PS3
> 
> Exactly.  What it means is basically "always reinitialize the device from
> scratch if you have run _PS3 on it".  And that's what we should do.
> 
> > > ------
> > > 
> > > This is in section 7.2.11: _PR3.
> > > 
> > > > 
> > > > Another problem:
> > > > 
> > > > With your patch, both D3hot and D3cold will evaluate _PS3, right?
> > > > 
> > > Yes.
> > > 
> > > > Will it have problem on AMD platform if you try to put ODD into D3hot
> > > > state? _PS3 is evaluated, so it actually enters D3Cold state.
> > > 
> > > There is no D3 hot support for this device(from the firmware's
> > > perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> > > _PS3).
> > 
> > But this is the generic code. We can't only consider some special
> > device.
> > 
> > Maybe we need some flag to tell which D3 state _PS3 is used for.
> 
> No, please.  As I said above, we need to reinitialize devices that _PS3 was
> executed on, which is equivalent to saying that those devices were put into
> D3_cold.
> 
> The only situation where a device can be put into ACPI D3_hot (which is not
> the same as PCI D3_hot, mind you) is when:
> 
> (1) There is _PR3 listing some of the device's power resources as "on".
> (2) The power resources listed by the _PR3 as "off" are turned off and the
>     power resources listed by the _PR3 as "on" are left in the "on" state.

I don't understand item (2):

If the power resource is listed as "off", which means it's already
turned off. Then why should it be turned off again?

Let's see an example

Assume a device "dev0" depends on 5 power resources:

pr1, pr2, pr3, pr4, pr5

_PR3 lists 3 power resources: pr3, pr4, pr5

Device(dev0)
{
	Name(_PR3, Package (0x03)
	{
		pr3,
		pr4,
		pr5
	})
}

If dev0 is put into ACPI D3_hot and pr1 and pr2 are not referenced by
other devices, then it requires:

- pr1 and pr2 are off
- pr3, pr4 and pr5 are on

right?

Thanks,
Lin Ming

> 
> Our generic code should reflect that.
> 
> Thanks,
> Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Aaron Lu]

Hi,

On Thu, Apr 05, 2012 at 10:31:20AM +0800, Lin Ming wrote:
> > 
> > The only situation where a device can be put into ACPI D3_hot (which is not
> > the same as PCI D3_hot, mind you) is when:
> > 
> > (1) There is _PR3 listing some of the device's power resources as "on".
> > (2) The power resources listed by the _PR3 as "off" are turned off and the
> >     power resources listed by the _PR3 as "on" are left in the "on" state.
> 
> I don't understand item (2):
> 
> If the power resource is listed as "off", which means it's already
> turned off. Then why should it be turned off again?

Rafael,
I think you misunderstand the meaning of _PR3.
The _PR3 will evaluate a list of power resources, not two lists(one "on"
list and one "off" list), as illustrated by Ming below.

And for a device to be put to D3 hot, it should:
1 execuate _PS3 first if available
2 turn on all the power resources referenced by _PR3

And for a device to be put to D3 cold, it should:
1 execute _PS3 first if available
2 turn off power resources referenced by _PRx, where x is the previous
state number of the device. Say if the device is put to D3 cold from D0,
the x would be 0.

Is this correct?

Thank,
Aaron

> 
> Let's see an example
> 
> Assume a device "dev0" depends on 5 power resources:
> 
> pr1, pr2, pr3, pr4, pr5
> 
> _PR3 lists 3 power resources: pr3, pr4, pr5
> 
> Device(dev0)
> {
> 	Name(_PR3, Package (0x03)
> 	{
> 		pr3,
> 		pr4,
> 		pr5
> 	})
> }
> 
> If dev0 is put into ACPI D3_hot and pr1 and pr2 are not referenced by
> other devices, then it requires:
> 
> - pr1 and pr2 are off
> - pr3, pr4 and pr5 are on
> 
> right?
> 
> Thanks,
> Lin Ming
> 
> 
> 

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Lin Ming]

On Thu, 2012-04-05 at 10:56 +0800, Aaron Lu wrote:
> Hi,
> 
> On Thu, Apr 05, 2012 at 10:31:20AM +0800, Lin Ming wrote:
> > > 
> > > The only situation where a device can be put into ACPI D3_hot (which is not
> > > the same as PCI D3_hot, mind you) is when:
> > > 
> > > (1) There is _PR3 listing some of the device's power resources as "on".
> > > (2) The power resources listed by the _PR3 as "off" are turned off and the
> > >     power resources listed by the _PR3 as "on" are left in the "on" state.
> > 
> > I don't understand item (2):
> > 
> > If the power resource is listed as "off", which means it's already
> > turned off. Then why should it be turned off again?
> 
> Rafael,
> I think you misunderstand the meaning of _PR3.
> The _PR3 will evaluate a list of power resources, not two lists(one "on"
> list and one "off" list), as illustrated by Ming below.
> 
> And for a device to be put to D3 hot, it should:
> 1 execuate _PS3 first if available
> 2 turn on all the power resources referenced by _PR3

  3 turn off all the power resources referenced by previous state

Thanks,
Lin Ming

> 
> And for a device to be put to D3 cold, it should:
> 1 execute _PS3 first if available
> 2 turn off power resources referenced by _PRx, where x is the previous
> state number of the device. Say if the device is put to D3 cold from D0,
> the x would be 0.
> 
> Is this correct?
> 
> Thank,
> Aaron
> 
> > 
> > Let's see an example
> > 
> > Assume a device "dev0" depends on 5 power resources:
> > 
> > pr1, pr2, pr3, pr4, pr5
> > 
> > _PR3 lists 3 power resources: pr3, pr4, pr5
> > 
> > Device(dev0)
> > {
> > 	Name(_PR3, Package (0x03)
> > 	{
> > 		pr3,
> > 		pr4,
> > 		pr5
> > 	})
> > }
> > 
> > If dev0 is put into ACPI D3_hot and pr1 and pr2 are not referenced by
> > other devices, then it requires:
> > 
> > - pr1 and pr2 are off
> > - pr3, pr4 and pr5 are on
> > 
> > right?
> > 
> > Thanks,
> > Lin Ming
> > 
> > 
> > 
> 

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Thursday, April 05, 2012, Lin Ming wrote:
> On Thu, 2012-04-05 at 10:56 +0800, Aaron Lu wrote:
> > Hi,
> > 
> > On Thu, Apr 05, 2012 at 10:31:20AM +0800, Lin Ming wrote:
> > > > 
> > > > The only situation where a device can be put into ACPI D3_hot (which is not
> > > > the same as PCI D3_hot, mind you) is when:
> > > > 
> > > > (1) There is _PR3 listing some of the device's power resources as "on".
> > > > (2) The power resources listed by the _PR3 as "off" are turned off and the
> > > >     power resources listed by the _PR3 as "on" are left in the "on" state.
> > > 
> > > I don't understand item (2):
> > > 
> > > If the power resource is listed as "off", which means it's already
> > > turned off. Then why should it be turned off again?
> > 
> > Rafael,
> > I think you misunderstand the meaning of _PR3.
> > The _PR3 will evaluate a list of power resources, not two lists(one "on"
> > list and one "off" list), as illustrated by Ming below.
> > 
> > And for a device to be put to D3 hot, it should:
> > 1 execuate _PS3 first if available
> > 2 turn on all the power resources referenced by _PR3
> 
>   3 turn off all the power resources referenced by previous state

But leave the ones listed by _PR3 in the "on" state, you mean?

Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Lin Ming]

On Mon, 2012-04-09 at 01:54 +0200, Rafael J. Wysocki wrote:
> On Thursday, April 05, 2012, Lin Ming wrote:
> > On Thu, 2012-04-05 at 10:56 +0800, Aaron Lu wrote:
> > > Hi,
> > > 
> > > On Thu, Apr 05, 2012 at 10:31:20AM +0800, Lin Ming wrote:
> > > > > 
> > > > > The only situation where a device can be put into ACPI D3_hot (which is not
> > > > > the same as PCI D3_hot, mind you) is when:
> > > > > 
> > > > > (1) There is _PR3 listing some of the device's power resources as "on".
> > > > > (2) The power resources listed by the _PR3 as "off" are turned off and the
> > > > >     power resources listed by the _PR3 as "on" are left in the "on" state.
> > > > 
> > > > I don't understand item (2):
> > > > 
> > > > If the power resource is listed as "off", which means it's already
> > > > turned off. Then why should it be turned off again?
> > > 
> > > Rafael,
> > > I think you misunderstand the meaning of _PR3.
> > > The _PR3 will evaluate a list of power resources, not two lists(one "on"
> > > list and one "off" list), as illustrated by Ming below.
> > > 
> > > And for a device to be put to D3 hot, it should:
> > > 1 execuate _PS3 first if available
> > > 2 turn on all the power resources referenced by _PR3
> > 
> >   3 turn off all the power resources referenced by previous state
> 
> But leave the ones listed by _PR3 in the "on" state, you mean?

Yes.

_PR0: {r1, r2, r3, r4, r5}
_PR3: {r4, r5}

D0: {r1, r2, r3, r4, r5} are all on
D3 hot: {r1, r2, r3} are turned off and {r4, r5} are *left* on
D3 cold: {r1, r2, r3, r4, r5} are all turned off

Is this correct?

Thanks for your detail explanation.

> 
> Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Monday, April 09, 2012, Lin Ming wrote:
> On Mon, 2012-04-09 at 01:54 +0200, Rafael J. Wysocki wrote:
> > On Thursday, April 05, 2012, Lin Ming wrote:
> > > On Thu, 2012-04-05 at 10:56 +0800, Aaron Lu wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Apr 05, 2012 at 10:31:20AM +0800, Lin Ming wrote:
> > > > > > 
> > > > > > The only situation where a device can be put into ACPI D3_hot (which is not
> > > > > > the same as PCI D3_hot, mind you) is when:
> > > > > > 
> > > > > > (1) There is _PR3 listing some of the device's power resources as "on".
> > > > > > (2) The power resources listed by the _PR3 as "off" are turned off and the
> > > > > >     power resources listed by the _PR3 as "on" are left in the "on" state.
> > > > > 
> > > > > I don't understand item (2):
> > > > > 
> > > > > If the power resource is listed as "off", which means it's already
> > > > > turned off. Then why should it be turned off again?
> > > > 
> > > > Rafael,
> > > > I think you misunderstand the meaning of _PR3.
> > > > The _PR3 will evaluate a list of power resources, not two lists(one "on"
> > > > list and one "off" list), as illustrated by Ming below.
> > > > 
> > > > And for a device to be put to D3 hot, it should:
> > > > 1 execuate _PS3 first if available
> > > > 2 turn on all the power resources referenced by _PR3
> > > 
> > >   3 turn off all the power resources referenced by previous state
> > 
> > But leave the ones listed by _PR3 in the "on" state, you mean?
> 
> Yes.
> 
> _PR0: {r1, r2, r3, r4, r5}
> _PR3: {r4, r5}
> 
> D0: {r1, r2, r3, r4, r5} are all on
> D3 hot: {r1, r2, r3} are turned off and {r4, r5} are *left* on
> D3 cold: {r1, r2, r3, r4, r5} are all turned off
> 
> Is this correct?

Yes, that's my understanding.

> Thanks for your detail explanation.

No problem.

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Thursday, April 05, 2012, Aaron Lu wrote:
> Hi,
> 
> On Thu, Apr 05, 2012 at 10:31:20AM +0800, Lin Ming wrote:
> > > 
> > > The only situation where a device can be put into ACPI D3_hot (which is not
> > > the same as PCI D3_hot, mind you) is when:
> > > 
> > > (1) There is _PR3 listing some of the device's power resources as "on".
> > > (2) The power resources listed by the _PR3 as "off" are turned off and the
> > >     power resources listed by the _PR3 as "on" are left in the "on" state.
> > 
> > I don't understand item (2):
> > 
> > If the power resource is listed as "off", which means it's already
> > turned off. Then why should it be turned off again?
> 
> Rafael,
> I think you misunderstand the meaning of _PR3.
> The _PR3 will evaluate a list of power resources, not two lists(one "on"
> list and one "off" list), as illustrated by Ming below.

Yes, the other list is returned by _PR0.

> And for a device to be put to D3 hot, it should:
> 1 execuate _PS3 first if available
> 2 turn on all the power resources referenced by _PR3

Not turn them on, but rather leave then in the "on" state.

> And for a device to be put to D3 cold, it should:
> 1 execute _PS3 first if available
> 2 turn off power resources referenced by _PRx, where x is the previous
> state number of the device. Say if the device is put to D3 cold from D0,
> the x would be 0.
> 
> Is this correct?

Yes, it is, except that you only need to evaluate _PR0 to get the entire
list of device's power resources and from there you know which resources to
turn off to put the device into D3_cold.

And BTW, please note that if any power resources are shared between devices,
then it may be unclear which state an individual device is at the moment.  For
example, if dev0 depends on power resources r1, r2, r3 and dev1 depends
on power resources r3, r4, r5, then dev0 will not really go to D3(cold) until
r3 is turned off, but this may be held by dev1.

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Thursday, April 05, 2012, Lin Ming wrote:
> On Sun, 2012-04-01 at 09:23 +0200, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > Sorry for the delayed response, I've been travelling recently.
> > 
> > On Sunday, April 01, 2012, Lin Ming wrote:
> > > On Sun, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
> > > > Hi,
> > > > 
> > > > On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> > > > > > -		if (device->power.states[state].flags.explicit_set) {
> > > > > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > > > > +		if (state == ACPI_STATE_D3_COLD) {
> > > > > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > > > > +			object_name[3] -= 1;
> > > > > > +		}
> > > > > 
> > > > > I'm not sure whether this works or not.
> > > > > 
> > > > > From ACPI spec,
> > > > > 
> > > > > _PS3 "is used to put the specific device into its D3hot or D3 state"
> > > > > 
> > > > > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> > > > > D3cold were introduced.
> > > > I guess D3 has to mean something, right? :-)
> > 
> > Well, not necessarily.
> > 
> > The problem is what state the _PS3 method puts the device into: D3_hot or
> > D3_cold.
> > 
> > Unfortunately, as far as I can say, ACPI 4.0 didn't specify any "official"
> > mapping between the "old" D3 and the "new" D3_{hod|cold} states, so we need to
> > figure out something.  In my opinion, the only reasonable approach is to
> > assume that the state _PS3 puts the device into is always D3_cold, becuase
> > _PS3 may remove power completely from the device.  It may not do that, but
> > we _must_ assume it does that in general.
> > 
> > > > Here is the problem, there is no _PR3 in AMD's implementation, just _PS3.
> > > > And since _S0W evaluates 4, I've to put this device into D3 cold state
> > > > with _PS3.
> > > > 
> > > > And the ACPI does have some words like:
> > > > 
> > > > ------
> > > > Platform/drivers must assume that the device will have power completely
> > > > removed when the device is place into “D3” via _PS3
> > 
> > Exactly.  What it means is basically "always reinitialize the device from
> > scratch if you have run _PS3 on it".  And that's what we should do.
> > 
> > > > ------
> > > > 
> > > > This is in section 7.2.11: _PR3.
> > > > 
> > > > > 
> > > > > Another problem:
> > > > > 
> > > > > With your patch, both D3hot and D3cold will evaluate _PS3, right?
> > > > > 
> > > > Yes.
> > > > 
> > > > > Will it have problem on AMD platform if you try to put ODD into D3hot
> > > > > state? _PS3 is evaluated, so it actually enters D3Cold state.
> > > > 
> > > > There is no D3 hot support for this device(from the firmware's
> > > > perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> > > > _PS3).
> > > 
> > > But this is the generic code. We can't only consider some special
> > > device.
> > > 
> > > Maybe we need some flag to tell which D3 state _PS3 is used for.
> > 
> > No, please.  As I said above, we need to reinitialize devices that _PS3 was
> > executed on, which is equivalent to saying that those devices were put into
> > D3_cold.
> > 
> > The only situation where a device can be put into ACPI D3_hot (which is not
> > the same as PCI D3_hot, mind you) is when:
> > 
> > (1) There is _PR3 listing some of the device's power resources as "on".
> > (2) The power resources listed by the _PR3 as "off" are turned off and the
> >     power resources listed by the _PR3 as "on" are left in the "on" state.
> 
> I don't understand item (2):
> 
> If the power resource is listed as "off", which means it's already
> turned off. Then why should it be turned off again?

Sorry, there are two lists.  One of them is returned by _PR0 (these
are needed to put the device into D0) and the second is returned by
_PR3 (if that exists) and these need to be "on" for the device to stay
in D3_hot (if they are "off", the device will go into D3_cold instead).

Actually, section 7.2.11 of ACPI 5.0 is quite clear in that respect.

> Let's see an example
> 
> Assume a device "dev0" depends on 5 power resources:
> 
> pr1, pr2, pr3, pr4, pr5
> 
> _PR3 lists 3 power resources: pr3, pr4, pr5
> 
> Device(dev0)
> {
> 	Name(_PR3, Package (0x03)
> 	{
> 		pr3,
> 		pr4,
> 		pr5
> 	})
> }
> 
> If dev0 is put into ACPI D3_hot and pr1 and pr2 are not referenced by
> other devices, then it requires:
> 
> - pr1 and pr2 are off
> - pr3, pr4 and pr5 are on
> 
> right?

That's correct.

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Lin Ming]

On Sun, 2012-04-01 at 09:23 +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> Sorry for the delayed response, I've been travelling recently.
> 
> On Sunday, April 01, 2012, Lin Ming wrote:
> > On Sun, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
> > > Hi,
> > > 
> > > On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> > > > > -		if (device->power.states[state].flags.explicit_set) {
> > > > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > > > +		if (state == ACPI_STATE_D3_COLD) {
> > > > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > > > +			object_name[3] -= 1;
> > > > > +		}
> > > > 
> > > > I'm not sure whether this works or not.
> > > > 
> > > > From ACPI spec,
> > > > 
> > > > _PS3 "is used to put the specific device into its D3hot or D3 state"
> > > > 
> > > > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> > > > D3cold were introduced.
> > > I guess D3 has to mean something, right? :-)
> 
> Well, not necessarily.
> 
> The problem is what state the _PS3 method puts the device into: D3_hot or
> D3_cold.
> 
> Unfortunately, as far as I can say, ACPI 4.0 didn't specify any "official"
> mapping between the "old" D3 and the "new" D3_{hod|cold} states, so we need to
> figure out something.  In my opinion, the only reasonable approach is to
> assume that the state _PS3 puts the device into is always D3_cold, becuase
> _PS3 may remove power completely from the device.  It may not do that, but
> we _must_ assume it does that in general.
> 
> > > Here is the problem, there is no _PR3 in AMD's implementation, just _PS3.
> > > And since _S0W evaluates 4, I've to put this device into D3 cold state
> > > with _PS3.
> > > 
> > > And the ACPI does have some words like:
> > > 
> > > ------
> > > Platform/drivers must assume that the device will have power completely
> > > removed when the device is place into “D3” via _PS3
> 
> Exactly.  What it means is basically "always reinitialize the device from
> scratch if you have run _PS3 on it".  And that's what we should do.
> 
> > > ------
> > > 
> > > This is in section 7.2.11: _PR3.
> > > 
> > > > 
> > > > Another problem:
> > > > 
> > > > With your patch, both D3hot and D3cold will evaluate _PS3, right?
> > > > 
> > > Yes.
> > > 
> > > > Will it have problem on AMD platform if you try to put ODD into D3hot
> > > > state? _PS3 is evaluated, so it actually enters D3Cold state.
> > > 
> > > There is no D3 hot support for this device(from the firmware's
> > > perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> > > _PS3).
> > 
> > But this is the generic code. We can't only consider some special
> > device.
> > 
> > Maybe we need some flag to tell which D3 state _PS3 is used for.
> 
> No, please.  As I said above, we need to reinitialize devices that _PS3 was
> executed on, which is equivalent to saying that those devices were put into
> D3_cold.
> 
> The only situation where a device can be put into ACPI D3_hot (which is not
> the same as PCI D3_hot, mind you) is when:
> 
> (1) There is _PR3 listing some of the device's power resources as "on".
> (2) The power resources listed by the _PR3 as "off" are turned off and the
>     power resources listed by the _PR3 as "on" are left in the "on" state.

(1) and (2) seems conflict.

(1) means all power resources listed in _PR3 are "on", but
(2) means some power resources listed in _PR3 are "off" and others are
"on"

Is my understanding correct?

Thanks,
Lin Ming

> 
> Our generic code should reflect that.
> 
> Thanks,
> Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Thursday, April 05, 2012, Lin Ming wrote:
> On Sun, 2012-04-01 at 09:23 +0200, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > Sorry for the delayed response, I've been travelling recently.
> > 
> > On Sunday, April 01, 2012, Lin Ming wrote:
> > > On Sun, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
> > > > Hi,
> > > > 
> > > > On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> > > > > > -		if (device->power.states[state].flags.explicit_set) {
> > > > > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > > > > +		if (state == ACPI_STATE_D3_COLD) {
> > > > > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > > > > +			object_name[3] -= 1;
> > > > > > +		}
> > > > > 
> > > > > I'm not sure whether this works or not.
> > > > > 
> > > > > From ACPI spec,
> > > > > 
> > > > > _PS3 "is used to put the specific device into its D3hot or D3 state"
> > > > > 
> > > > > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> > > > > D3cold were introduced.
> > > > I guess D3 has to mean something, right? :-)
> > 
> > Well, not necessarily.
> > 
> > The problem is what state the _PS3 method puts the device into: D3_hot or
> > D3_cold.
> > 
> > Unfortunately, as far as I can say, ACPI 4.0 didn't specify any "official"
> > mapping between the "old" D3 and the "new" D3_{hod|cold} states, so we need to
> > figure out something.  In my opinion, the only reasonable approach is to
> > assume that the state _PS3 puts the device into is always D3_cold, becuase
> > _PS3 may remove power completely from the device.  It may not do that, but
> > we _must_ assume it does that in general.
> > 
> > > > Here is the problem, there is no _PR3 in AMD's implementation, just _PS3.
> > > > And since _S0W evaluates 4, I've to put this device into D3 cold state
> > > > with _PS3.
> > > > 
> > > > And the ACPI does have some words like:
> > > > 
> > > > ------
> > > > Platform/drivers must assume that the device will have power completely
> > > > removed when the device is place into “D3” via _PS3
> > 
> > Exactly.  What it means is basically "always reinitialize the device from
> > scratch if you have run _PS3 on it".  And that's what we should do.
> > 
> > > > ------
> > > > 
> > > > This is in section 7.2.11: _PR3.
> > > > 
> > > > > 
> > > > > Another problem:
> > > > > 
> > > > > With your patch, both D3hot and D3cold will evaluate _PS3, right?
> > > > > 
> > > > Yes.
> > > > 
> > > > > Will it have problem on AMD platform if you try to put ODD into D3hot
> > > > > state? _PS3 is evaluated, so it actually enters D3Cold state.
> > > > 
> > > > There is no D3 hot support for this device(from the firmware's
> > > > perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> > > > _PS3).
> > > 
> > > But this is the generic code. We can't only consider some special
> > > device.
> > > 
> > > Maybe we need some flag to tell which D3 state _PS3 is used for.
> > 
> > No, please.  As I said above, we need to reinitialize devices that _PS3 was
> > executed on, which is equivalent to saying that those devices were put into
> > D3_cold.
> > 
> > The only situation where a device can be put into ACPI D3_hot (which is not
> > the same as PCI D3_hot, mind you) is when:
> > 
> > (1) There is _PR3 listing some of the device's power resources as "on".
> > (2) The power resources listed by the _PR3 as "off" are turned off and the
> >     power resources listed by the _PR3 as "on" are left in the "on" state.
> 
> (1) and (2) seems conflict.
> 
> (1) means all power resources listed in _PR3 are "on", but
> (2) means some power resources listed in _PR3 are "off" and others are
> "on"
> 
> Is my understanding correct?

Not really, sorry for the confusion.  As I said in another message, if _PR3 is
present, there are two lists of resources, the one returned by _PR3 and the
one returned by _PR0 (which lists all power resources the device depends on).

So (1) above really means "power resources returned by _PR3 are on" and (2) means
"the power resources listed by _PR0 and not by _PR3 are in the 'off' state,
while the ones listed by both _PR0 and _PR3 are in the 'on' state".

In theory, there may be power resources that are listed by _PR3 and not by
_PR0, but that would be a very strange case in practice.

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Aaron Lu]

On Sun, Apr 01, 2012 at 02:28:57PM +0800, Lin Ming wrote:
> > 
> > There is no D3 hot support for this device(from the firmware's
> > perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> > _PS3).
> 
> But this is the generic code. We can't only consider some special
> device.
Agree.

The code change I've made only affects D3 cold path, existing code path
to set device power state to D0-D3hot is not affected.

And for D3 cold path, I don't think evaluating _PS3 should do any harm
to the device, isn't it the case?

> 
> Maybe we need some flag to tell which D3 state _PS3 is used for.
Do you mean the ACPI spec needs to be updated to reflect which D3
state the device will be at once its _PS3 is evaluated?

-Aaron

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Zhang Rui]

On 日, 2012-04-01 at 13:56 +0800, Aaron Lu wrote:
> Hi,
> 
> On Sun, Apr 01, 2012 at 01:27:33PM +0800, Lin Ming wrote:
> > > -		if (device->power.states[state].flags.explicit_set) {
> > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > +		if (state == ACPI_STATE_D3_COLD) {
> > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > +			object_name[3] -= 1;
> > > +		}
> > 
> > I'm not sure whether this works or not.
> > 
> > From ACPI spec,
> > 
> > _PS3 "is used to put the specific device into its D3hot or D3 state"
> > 
> > D3 neither means D3hot nor D3cold. It's an old term before D3hot and
> > D3cold were introduced.
> I guess D3 has to mean something, right? :-)
> 
> Here is the problem, there is no _PR3 in AMD's implementation, just _PS3.
> And since _S0W evaluates 4, I've to put this device into D3 cold state
> with _PS3.
> 
First of all, I agree that we must evaluate _PS3 when setting device to
either D3_HOT or D3_COLD.

And here is my understanding about D3/D3_HOT/D3_COLD,

if _PR3 exists, it means the devices supports both D3_HOT and D3_COLD.

if only _PS3 exists, we can only say that the state after evaluating
_PS3 is D3, it could either be D3_HOT or D3_COLD, and this is device
specific, which in your case, is D3_COLD.

BTW, here is the description of _S0W in ACPI spec,
If OSPM has not indicated that it supports _PR3 through the OSPM
Platform-Wide Capabilities (see Section 6.2.10.2), then the value "3"
corresponds to D3. If it has indicated _PR3 support, the value "3"
represents D3hot and the value "4" represents D3cold.

So IMO, the _S0W should return 3 in AMD's implementation as it does not
have _PR3.

> And the ACPI does have some words like:
> 
> ------
> Platform/drivers must assume that the device will have power completely
> removed when the device is place into “D3” via _PS3
> ------
> 
I think this means OS can not access device any more after evaluating
_PS3, and it should re-enumerate the device when transiting back to D0.

> This is in section 7.2.11: _PR3.
> 
> > 
> > Another problem:
> > 
> > With your patch, both D3hot and D3cold will evaluate _PS3, right?
> > 
> Yes.
> 
> > Will it have problem on AMD platform if you try to put ODD into D3hot
> > state? _PS3 is evaluated, so it actually enters D3Cold state.
> 
> There is no D3 hot support for this device(from the firmware's
> perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> _PS3).
> 
I was trying to make a cleanup of the D3/D3_HOT/D3_COLD support in
Linux, and this gives me some clue.

How about this?

We should use the term "D3" in general in Linux.
Without _PR3, OS should *assume* that the power is removed, although it
may be not true.
With _PR3, OS can *assure* that the power is removed, because it knows
how to remove thw power (evaluating _PR3._OFF).

So the difference is that OS need to make sure whether to evaluate
_PR3._OFF when _PR3 exists. For example, a device has _PR3, but _S0W
returns 3, OS should not evaluate _PR3._OFF when the device sleeps with
remote wakeup support.

what do you think?

thanks,
rui

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Sunday, April 01, 2012, Zhang Rui wrote:
[...]
> 
> How about this?
> 
> We should use the term "D3" in general in Linux.

I don't see why.  We can refer to the "old D3" as D3_cold.

> Without _PR3, OS should *assume* that the power is removed, although it
> may be not true.

That's correct.

> With _PR3, OS can *assure* that the power is removed, because it knows
> how to remove thw power (evaluating _PR3._OFF).

I'd rather say that with _PR3 we have the opportunity to avoid removing
power completely from the device.  In other words, D3_hot is supported (and
it is supported _only_ in that case).

> So the difference is that OS need to make sure whether to evaluate
> _PR3._OFF when _PR3 exists. For example, a device has _PR3, but _S0W
> returns 3, OS should not evaluate _PR3._OFF when the device sleeps with
> remote wakeup support.

That's correct.

> what do you think?

Well, see above and my other message in this thread.

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Aaron Lu]

Hi,

On Sun, Apr 01, 2012 at 03:03:39PM +0800, Zhang Rui wrote:
> First of all, I agree that we must evaluate _PS3 when setting device to
> either D3_HOT or D3_COLD.
Good.

> 
> And here is my understanding about D3/D3_HOT/D3_COLD,
> 
> if _PR3 exists, it means the devices supports both D3_HOT and D3_COLD.
Agree.

> 
> if only _PS3 exists, we can only say that the state after evaluating
> _PS3 is D3, it could either be D3_HOT or D3_COLD, and this is device
> specific, which in your case, is D3_COLD.
I prefer Rafeal's definition, let's just *assume* the device is at D3
cold after its _PS3 is executed. Unless it has _PR3, in which case, we
have a chance to put the device into D3 hot instead.

> 
> BTW, here is the description of _S0W in ACPI spec,
> If OSPM has not indicated that it supports _PR3 through the OSPM
> Platform-Wide Capabilities (see Section 6.2.10.2), then the value "3"
> corresponds to D3. If it has indicated _PR3 support, the value "3"
> represents D3hot and the value "4" represents D3cold.
> 
> So IMO, the _S0W should return 3 in AMD's implementation as it does not
> have _PR3.
OK, sounds like a firmware bug.
Thanks for identifying this.

> 
> > And the ACPI does have some words like:
> > 
> > ------
> > Platform/drivers must assume that the device will have power completely
> > removed when the device is place into “D3” via _PS3
> > ------
> > 
> I think this means OS can not access device any more after evaluating
> _PS3, and it should re-enumerate the device when transiting back to D0.
> 
> > This is in section 7.2.11: _PR3.
> > 
> > > 
> > > Another problem:
> > > 
> > > With your patch, both D3hot and D3cold will evaluate _PS3, right?
> > > 
> > Yes.
> > 
> > > Will it have problem on AMD platform if you try to put ODD into D3hot
> > > state? _PS3 is evaluated, so it actually enters D3Cold state.
> > 
> > There is no D3 hot support for this device(from the firmware's
> > perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> > _PS3).
> > 
> I was trying to make a cleanup of the D3/D3_HOT/D3_COLD support in
> Linux, and this gives me some clue.
This is great, and I would like to help as much as I can.

> 
> How about this?
> 
> We should use the term "D3" in general in Linux.
> Without _PR3, OS should *assume* that the power is removed, although it
> may be not true.
> With _PR3, OS can *assure* that the power is removed, because it knows
> how to remove thw power (evaluating _PR3._OFF).
> 
> So the difference is that OS need to make sure whether to evaluate
> _PR3._OFF when _PR3 exists. For example, a device has _PR3, but _S0W
> returns 3, OS should not evaluate _PR3._OFF when the device sleeps with
> remote wakeup support.
> 
> what do you think?
I agree with Rafeal's ideas.

-Aaron

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Sunday, April 01, 2012, Aaron Lu wrote:
> Hi,
> 
> On Sun, Apr 01, 2012 at 03:03:39PM +0800, Zhang Rui wrote:
> > First of all, I agree that we must evaluate _PS3 when setting device to
> > either D3_HOT or D3_COLD.
> Good.
> 
> > 
> > And here is my understanding about D3/D3_HOT/D3_COLD,
> > 
> > if _PR3 exists, it means the devices supports both D3_HOT and D3_COLD.
> Agree.
> 
> > 
> > if only _PS3 exists, we can only say that the state after evaluating
> > _PS3 is D3, it could either be D3_HOT or D3_COLD, and this is device
> > specific, which in your case, is D3_COLD.
> I prefer Rafeal's definition, let's just *assume* the device is at D3
> cold after its _PS3 is executed. Unless it has _PR3, in which case, we
> have a chance to put the device into D3 hot instead.
> 
> > 
> > BTW, here is the description of _S0W in ACPI spec,
> > If OSPM has not indicated that it supports _PR3 through the OSPM
> > Platform-Wide Capabilities (see Section 6.2.10.2), then the value "3"
> > corresponds to D3. If it has indicated _PR3 support, the value "3"
> > represents D3hot and the value "4" represents D3cold.
> > 
> > So IMO, the _S0W should return 3 in AMD's implementation as it does not
> > have _PR3.
> OK, sounds like a firmware bug.
> Thanks for identifying this.

I don't think this is a bug.  It actually may return either 3 or 4, because
there is no difference between them if there's no _PR3 (i.e. the action to
carry out by software would only be different if _PR3 were present).

> > > And the ACPI does have some words like:
> > > 
> > > ------
> > > Platform/drivers must assume that the device will have power completely
> > > removed when the device is place into “D3” via _PS3
> > > ------
> > > 
> > I think this means OS can not access device any more after evaluating
> > _PS3, and it should re-enumerate the device when transiting back to D0.
> > 
> > > This is in section 7.2.11: _PR3.
> > > 
> > > > 
> > > > Another problem:
> > > > 
> > > > With your patch, both D3hot and D3cold will evaluate _PS3, right?
> > > > 
> > > Yes.
> > > 
> > > > Will it have problem on AMD platform if you try to put ODD into D3hot
> > > > state? _PS3 is evaluated, so it actually enters D3Cold state.
> > > 
> > > There is no D3 hot support for this device(from the firmware's
> > > perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> > > _PS3).
> > > 
> > I was trying to make a cleanup of the D3/D3_HOT/D3_COLD support in
> > Linux, and this gives me some clue.
> This is great, and I would like to help as much as I can.
> 
> > 
> > How about this?
> > 
> > We should use the term "D3" in general in Linux.
> > Without _PR3, OS should *assume* that the power is removed, although it
> > may be not true.
> > With _PR3, OS can *assure* that the power is removed, because it knows
> > how to remove thw power (evaluating _PR3._OFF).
> > 
> > So the difference is that OS need to make sure whether to evaluate
> > _PR3._OFF when _PR3 exists. For example, a device has _PR3, but _S0W
> > returns 3, OS should not evaluate _PR3._OFF when the device sleeps with
> > remote wakeup support.
> > 
> > what do you think?
> I agree with Rafeal's ideas.

Good. :-)

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Zhang Rui]

On 日, 2012-04-01 at 09:47 +0200, Rafael J. Wysocki wrote:
> On Sunday, April 01, 2012, Aaron Lu wrote:
> > Hi,
> > 
> > On Sun, Apr 01, 2012 at 03:03:39PM +0800, Zhang Rui wrote:
> > > First of all, I agree that we must evaluate _PS3 when setting device to
> > > either D3_HOT or D3_COLD.
> > Good.
> > 
> > > 
> > > And here is my understanding about D3/D3_HOT/D3_COLD,
> > > 
> > > if _PR3 exists, it means the devices supports both D3_HOT and D3_COLD.
> > Agree.
> > 
> > > 
> > > if only _PS3 exists, we can only say that the state after evaluating
> > > _PS3 is D3, it could either be D3_HOT or D3_COLD, and this is device
> > > specific, which in your case, is D3_COLD.
> > I prefer Rafeal's definition, let's just *assume* the device is at D3
> > cold after its _PS3 is executed. Unless it has _PR3, in which case, we
> > have a chance to put the device into D3 hot instead.
> > 
> > > 
> > > BTW, here is the description of _S0W in ACPI spec,
> > > If OSPM has not indicated that it supports _PR3 through the OSPM
> > > Platform-Wide Capabilities (see Section 6.2.10.2), then the value "3"
> > > corresponds to D3. If it has indicated _PR3 support, the value "3"
> > > represents D3hot and the value "4" represents D3cold.
> > > 
> > > So IMO, the _S0W should return 3 in AMD's implementation as it does not
> > > have _PR3.
> > OK, sounds like a firmware bug.
> > Thanks for identifying this.
> 
> I don't think this is a bug.  It actually may return either 3 or 4, because
> there is no difference between them if there's no _PR3 (i.e. the action to
> carry out by software would only be different if _PR3 were present).
> 
I mean, surely that software should handle this case.
But this is still a violation of ACPI spec, as the device has only one
D3 state, instead of D3_HOT/D3_COLD,, thus _S0W should return 3 instead.

thanks,
rui
> > > > And the ACPI does have some words like:
> > > > 
> > > > ------
> > > > Platform/drivers must assume that the device will have power completely
> > > > removed when the device is place into “D3” via _PS3
> > > > ------
> > > > 
> > > I think this means OS can not access device any more after evaluating
> > > _PS3, and it should re-enumerate the device when transiting back to D0.
> > > 
> > > > This is in section 7.2.11: _PR3.
> > > > 
> > > > > 
> > > > > Another problem:
> > > > > 
> > > > > With your patch, both D3hot and D3cold will evaluate _PS3, right?
> > > > > 
> > > > Yes.
> > > > 
> > > > > Will it have problem on AMD platform if you try to put ODD into D3hot
> > > > > state? _PS3 is evaluated, so it actually enters D3Cold state.
> > > > 
> > > > There is no D3 hot support for this device(from the firmware's
> > > > perspective), either it is at D0(via _PS0), or it will be at D3 cold(via
> > > > _PS3).
> > > > 
> > > I was trying to make a cleanup of the D3/D3_HOT/D3_COLD support in
> > > Linux, and this gives me some clue.
> > This is great, and I would like to help as much as I can.
> > 
> > > 
> > > How about this?
> > > 
> > > We should use the term "D3" in general in Linux.
> > > Without _PR3, OS should *assume* that the power is removed, although it
> > > may be not true.
> > > With _PR3, OS can *assure* that the power is removed, because it knows
> > > how to remove thw power (evaluating _PR3._OFF).
> > > 
> > > So the difference is that OS need to make sure whether to evaluate
> > > _PR3._OFF when _PR3 exists. For example, a device has _PR3, but _S0W
> > > returns 3, OS should not evaluate _PR3._OFF when the device sleeps with
> > > remote wakeup support.
> > > 
> > > what do you think?
> > I agree with Rafeal's ideas.
> 
> Good. :-)
> 
> Thanks,
> Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Sunday, April 01, 2012, Zhang Rui wrote:
> On 日, 2012-04-01 at 09:47 +0200, Rafael J. Wysocki wrote:
> > On Sunday, April 01, 2012, Aaron Lu wrote:
> > > Hi,
> > > 
> > > On Sun, Apr 01, 2012 at 03:03:39PM +0800, Zhang Rui wrote:
> > > > First of all, I agree that we must evaluate _PS3 when setting device to
> > > > either D3_HOT or D3_COLD.
> > > Good.
> > > 
> > > > 
> > > > And here is my understanding about D3/D3_HOT/D3_COLD,
> > > > 
> > > > if _PR3 exists, it means the devices supports both D3_HOT and D3_COLD.
> > > Agree.
> > > 
> > > > 
> > > > if only _PS3 exists, we can only say that the state after evaluating
> > > > _PS3 is D3, it could either be D3_HOT or D3_COLD, and this is device
> > > > specific, which in your case, is D3_COLD.
> > > I prefer Rafeal's definition, let's just *assume* the device is at D3
> > > cold after its _PS3 is executed. Unless it has _PR3, in which case, we
> > > have a chance to put the device into D3 hot instead.
> > > 
> > > > 
> > > > BTW, here is the description of _S0W in ACPI spec,
> > > > If OSPM has not indicated that it supports _PR3 through the OSPM
> > > > Platform-Wide Capabilities (see Section 6.2.10.2), then the value "3"
> > > > corresponds to D3. If it has indicated _PR3 support, the value "3"
> > > > represents D3hot and the value "4" represents D3cold.
> > > > 
> > > > So IMO, the _S0W should return 3 in AMD's implementation as it does not
> > > > have _PR3.
> > > OK, sounds like a firmware bug.
> > > Thanks for identifying this.
> > 
> > I don't think this is a bug.  It actually may return either 3 or 4, because
> > there is no difference between them if there's no _PR3 (i.e. the action to
> > carry out by software would only be different if _PR3 were present).
> > 
> I mean, surely that software should handle this case.

Well, exactly. :-)

> But this is still a violation of ACPI spec, as the device has only one
> D3 state, instead of D3_HOT/D3_COLD,, thus _S0W should return 3 instead.

If software is expected to handle that case anyway, I wouldn't really call
it a violation.  What really matters is what software is supposed to do.

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Aaron Lu]

Hi Rafael and Ming,

Do you have any more comments on this patch?
If not, can I have your ack? Thanks.

-Aaron

On Sun, Apr 01, 2012 at 02:18:30AM +0800, Aaron Lu wrote:
> When entering D3 Cold from a higher device state, evaluate _PS3 first
> and then make the proper power transition.
> This is used to solve the ZPODD problem on AMD's platform, _PS3 on such
> platforms will power off the ODD device and thus make the device enter
> D3 cold state.
> 
> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> Cc: Andiry Xu <andiry.xu@amd.com>
> Cc: Alex He <alex.he@amd.com>
> ---
>  drivers/acpi/bus.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 3263b68..68e593f 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -227,6 +227,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>  	int result = 0;
>  	acpi_status status = AE_OK;
>  	char object_name[5] = { '_', 'P', 'S', '0' + state, '\0' };
> +	struct acpi_device_power_state *ps;
> +	u8 explicit_set;
>  
>  	if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
>  		return -EINVAL;
> @@ -239,7 +241,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>  		return 0;
>  	}
>  
> -	if (!device->power.states[state].flags.valid) {
> +	ps = &device->power.states[state];
> +	if (!ps->flags.valid) {
>  		printk(KERN_WARNING PREFIX "Device does not support D%d\n", state);
>  		return -ENODEV;
>  	}
> @@ -263,7 +266,7 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>  			if (result)
>  				goto end;
>  		}
> -		if (device->power.states[state].flags.explicit_set) {
> +		if (ps->flags.explicit_set) {
>  			status = acpi_evaluate_object(device->handle,
>  						      object_name, NULL, NULL);
>  			if (ACPI_FAILURE(status)) {
> @@ -272,7 +275,15 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>  			}
>  		}
>  	} else {
> -		if (device->power.states[state].flags.explicit_set) {
> +		/* If state is D3 Cold, try to evaluate _PS3 first */
> +		if (state == ACPI_STATE_D3_COLD) {
> +			explicit_set = (ps - 1)->flags.explicit_set;
> +			object_name[3] -= 1;
> +		}
> +		else {
> +			explicit_set = ps->flags.explicit_set;
> +		}
> +		if (explicit_set) {
>  			status = acpi_evaluate_object(device->handle,
>  						      object_name, NULL, NULL);
>  			if (ACPI_FAILURE(status)) {
> -- 
> 1.7.9.2.315.g25a78.dirty
> 

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Monday, April 23, 2012, Aaron Lu wrote:
> Hi Rafael and Ming,
> 
> Do you have any more comments on this patch?
> If not, can I have your ack? Thanks.
> 
> -Aaron
> 
> On Sun, Apr 01, 2012 at 02:18:30AM +0800, Aaron Lu wrote:
> > When entering D3 Cold from a higher device state, evaluate _PS3 first
> > and then make the proper power transition.
> > This is used to solve the ZPODD problem on AMD's platform, _PS3 on such
> > platforms will power off the ODD device and thus make the device enter
> > D3 cold state.
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> > Cc: Andiry Xu <andiry.xu@amd.com>
> > Cc: Alex He <alex.he@amd.com>
> > ---
> >  drivers/acpi/bus.c |   17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 3263b68..68e593f 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -227,6 +227,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> >  	int result = 0;
> >  	acpi_status status = AE_OK;
> >  	char object_name[5] = { '_', 'P', 'S', '0' + state, '\0' };
> > +	struct acpi_device_power_state *ps;
> > +	u8 explicit_set;
> >  
> >  	if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
> >  		return -EINVAL;
> > @@ -239,7 +241,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> >  		return 0;
> >  	}
> >  
> > -	if (!device->power.states[state].flags.valid) {
> > +	ps = &device->power.states[state];
> > +	if (!ps->flags.valid) {
> >  		printk(KERN_WARNING PREFIX "Device does not support D%d\n", state);
> >  		return -ENODEV;
> >  	}
> > @@ -263,7 +266,7 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> >  			if (result)
> >  				goto end;
> >  		}
> > -		if (device->power.states[state].flags.explicit_set) {
> > +		if (ps->flags.explicit_set) {
> >  			status = acpi_evaluate_object(device->handle,
> >  						      object_name, NULL, NULL);
> >  			if (ACPI_FAILURE(status)) {
> > @@ -272,7 +275,15 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> >  			}
> >  		}
> >  	} else {
> > -		if (device->power.states[state].flags.explicit_set) {
> > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > +		if (state == ACPI_STATE_D3_COLD) {
> > +			explicit_set = (ps - 1)->flags.explicit_set;
> > +			object_name[3] -= 1;
> > +		}
> > +		else {
> > +			explicit_set = ps->flags.explicit_set;
> > +		}

I really don't like this.  I think you should modify acpi_bus_get_power_flags(),
on top of the recent Lin Ming's patch, so that it sets flags.explicit_set for
D3_COLD if _PS3 is present and _PR3 is not.

I'm not sure you'll need the $subject patch any more then.

> > +		if (explicit_set) {
> >  			status = acpi_evaluate_object(device->handle,
> >  						      object_name, NULL, NULL);
> >  			if (ACPI_FAILURE(status)) {

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Aaron Lu]

On Mon, Apr 23, 2012 at 01:43:03PM +0200, Rafael J. Wysocki wrote:
> On Monday, April 23, 2012, Aaron Lu wrote:
> > Hi Rafael and Ming,
> > 
> > Do you have any more comments on this patch?
> > If not, can I have your ack? Thanks.
> > 
> > -Aaron
> > 
> > On Sun, Apr 01, 2012 at 02:18:30AM +0800, Aaron Lu wrote:
> > > When entering D3 Cold from a higher device state, evaluate _PS3 first
> > > and then make the proper power transition.
> > > This is used to solve the ZPODD problem on AMD's platform, _PS3 on such
> > > platforms will power off the ODD device and thus make the device enter
> > > D3 cold state.
> > > 
> > > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> > > Cc: Andiry Xu <andiry.xu@amd.com>
> > > Cc: Alex He <alex.he@amd.com>
> > > ---
> > >  drivers/acpi/bus.c |   17 ++++++++++++++---
> > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > index 3263b68..68e593f 100644
> > > --- a/drivers/acpi/bus.c
> > > +++ b/drivers/acpi/bus.c
> > > @@ -227,6 +227,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> > >  	int result = 0;
> > >  	acpi_status status = AE_OK;
> > >  	char object_name[5] = { '_', 'P', 'S', '0' + state, '\0' };
> > > +	struct acpi_device_power_state *ps;
> > > +	u8 explicit_set;
> > >  
> > >  	if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
> > >  		return -EINVAL;
> > > @@ -239,7 +241,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> > >  		return 0;
> > >  	}
> > >  
> > > -	if (!device->power.states[state].flags.valid) {
> > > +	ps = &device->power.states[state];
> > > +	if (!ps->flags.valid) {
> > >  		printk(KERN_WARNING PREFIX "Device does not support D%d\n", state);
> > >  		return -ENODEV;
> > >  	}
> > > @@ -263,7 +266,7 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> > >  			if (result)
> > >  				goto end;
> > >  		}
> > > -		if (device->power.states[state].flags.explicit_set) {
> > > +		if (ps->flags.explicit_set) {
> > >  			status = acpi_evaluate_object(device->handle,
> > >  						      object_name, NULL, NULL);
> > >  			if (ACPI_FAILURE(status)) {
> > > @@ -272,7 +275,15 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> > >  			}
> > >  		}
> > >  	} else {
> > > -		if (device->power.states[state].flags.explicit_set) {
> > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > +		if (state == ACPI_STATE_D3_COLD) {
> > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > +			object_name[3] -= 1;
> > > +		}
> > > +		else {
> > > +			explicit_set = ps->flags.explicit_set;
> > > +		}
> 
> I really don't like this.  I think you should modify acpi_bus_get_power_flags(),
> on top of the recent Lin Ming's patch, so that it sets flags.explicit_set for
> D3_COLD if _PS3 is present and _PR3 is not.
> 

Thanks for your comments.

> I'm not sure you'll need the $subject patch any more then.
> 

A little change is still required. I attached the code below.
BTW, I think _PS3 will also need be executed if both _PS3 and _PR3
available when putting a device into D3cold.

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 3263b68..187433f 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -273,6 +273,9 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
 		}
 	} else {
 		if (device->power.states[state].flags.explicit_set) {
+			/* evaluate _PS3 instead of _PS4 when entering D3Cold */
+			if (state == ACPI_STATE_D3)
+				object_name[3] -= 1;
 			status = acpi_evaluate_object(device->handle,
 						      object_name, NULL, NULL);
 			if (ACPI_FAILURE(status)) {
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 7417267..de2ae10 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -887,8 +887,12 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
 		/* Evaluate "_PSx" to see if we can do explicit sets */
 		object_name[2] = 'S';
 		status = acpi_get_handle(device->handle, object_name, &handle);
-		if (ACPI_SUCCESS(status))
+		if (ACPI_SUCCESS(status)) {
 			ps->flags.explicit_set = 1;
+			/* Also set D3Cold's explicit flag when _PS3 exists */
+			if (i == ACPI_STATE_D3_HOT)
+				(ps+1)->flags.explicit_set = 1;
+		}
 
 		/*
 		 * State is valid if there are means to put the device into it.


Do you like this?

Thanks,
Aaron

> > > +		if (explicit_set) {
> > >  			status = acpi_evaluate_object(device->handle,
> > >  						      object_name, NULL, NULL);
> > >  			if (ACPI_FAILURE(status)) {
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Monday, April 23, 2012, Aaron Lu wrote:
> On Mon, Apr 23, 2012 at 01:43:03PM +0200, Rafael J. Wysocki wrote:
> > On Monday, April 23, 2012, Aaron Lu wrote:
> > > Hi Rafael and Ming,
> > > 
> > > Do you have any more comments on this patch?
> > > If not, can I have your ack? Thanks.
> > > 
> > > -Aaron
> > > 
> > > On Sun, Apr 01, 2012 at 02:18:30AM +0800, Aaron Lu wrote:
> > > > When entering D3 Cold from a higher device state, evaluate _PS3 first
> > > > and then make the proper power transition.
> > > > This is used to solve the ZPODD problem on AMD's platform, _PS3 on such
> > > > platforms will power off the ODD device and thus make the device enter
> > > > D3 cold state.
> > > > 
> > > > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> > > > Cc: Andiry Xu <andiry.xu@amd.com>
> > > > Cc: Alex He <alex.he@amd.com>
> > > > ---
> > > >  drivers/acpi/bus.c |   17 ++++++++++++++---
> > > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > > index 3263b68..68e593f 100644
> > > > --- a/drivers/acpi/bus.c
> > > > +++ b/drivers/acpi/bus.c
> > > > @@ -227,6 +227,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> > > >  	int result = 0;
> > > >  	acpi_status status = AE_OK;
> > > >  	char object_name[5] = { '_', 'P', 'S', '0' + state, '\0' };
> > > > +	struct acpi_device_power_state *ps;
> > > > +	u8 explicit_set;
> > > >  
> > > >  	if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
> > > >  		return -EINVAL;
> > > > @@ -239,7 +241,8 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > -	if (!device->power.states[state].flags.valid) {
> > > > +	ps = &device->power.states[state];
> > > > +	if (!ps->flags.valid) {
> > > >  		printk(KERN_WARNING PREFIX "Device does not support D%d\n", state);
> > > >  		return -ENODEV;
> > > >  	}
> > > > @@ -263,7 +266,7 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> > > >  			if (result)
> > > >  				goto end;
> > > >  		}
> > > > -		if (device->power.states[state].flags.explicit_set) {
> > > > +		if (ps->flags.explicit_set) {
> > > >  			status = acpi_evaluate_object(device->handle,
> > > >  						      object_name, NULL, NULL);
> > > >  			if (ACPI_FAILURE(status)) {
> > > > @@ -272,7 +275,15 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> > > >  			}
> > > >  		}
> > > >  	} else {
> > > > -		if (device->power.states[state].flags.explicit_set) {
> > > > +		/* If state is D3 Cold, try to evaluate _PS3 first */
> > > > +		if (state == ACPI_STATE_D3_COLD) {
> > > > +			explicit_set = (ps - 1)->flags.explicit_set;
> > > > +			object_name[3] -= 1;
> > > > +		}
> > > > +		else {
> > > > +			explicit_set = ps->flags.explicit_set;
> > > > +		}
> > 
> > I really don't like this.  I think you should modify acpi_bus_get_power_flags(),
> > on top of the recent Lin Ming's patch, so that it sets flags.explicit_set for
> > D3_COLD if _PS3 is present and _PR3 is not.
> > 
> 
> Thanks for your comments.
> 
> > I'm not sure you'll need the $subject patch any more then.
> > 
> 
> A little change is still required. I attached the code below.
> BTW, I think _PS3 will also need be executed if both _PS3 and _PR3
> available when putting a device into D3cold.

Yes, ACPI 5.0 seems to indicate so at least.

> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 3263b68..187433f 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -273,6 +273,9 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>  		}
>  	} else {
>  		if (device->power.states[state].flags.explicit_set) {
> +			/* evaluate _PS3 instead of _PS4 when entering D3Cold */
> +			if (state == ACPI_STATE_D3)
> +				object_name[3] -= 1;

Can you just put '3' here directly?  That'll be much cleaner than the
subtraction (that could have been -- as well).

>  			status = acpi_evaluate_object(device->handle,
>  						      object_name, NULL, NULL);
>  			if (ACPI_FAILURE(status)) {
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 7417267..de2ae10 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -887,8 +887,12 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>  		/* Evaluate "_PSx" to see if we can do explicit sets */
>  		object_name[2] = 'S';
>  		status = acpi_get_handle(device->handle, object_name, &handle);
> -		if (ACPI_SUCCESS(status))
> +		if (ACPI_SUCCESS(status)) {
>  			ps->flags.explicit_set = 1;
> +			/* Also set D3Cold's explicit flag when _PS3 exists */
> +			if (i == ACPI_STATE_D3_HOT)
> +				(ps+1)->flags.explicit_set = 1;

Please don't use pointer arithmetics here.  I know it _happens_ to work,
but I don't think it's appropriate in this situation at all.

> +		}
>  
>  		/*
>  		 * State is valid if there are means to put the device into it.

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Aaron Lu]

On Mon, Apr 23, 2012 at 09:50:57PM +0200, Rafael J. Wysocki wrote:
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 3263b68..187433f 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -273,6 +273,9 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> >  		}
> >  	} else {
> >  		if (device->power.states[state].flags.explicit_set) {
> > +			/* evaluate _PS3 instead of _PS4 when entering D3Cold */
> > +			if (state == ACPI_STATE_D3)
> > +				object_name[3] -= 1;
> 
> Can you just put '3' here directly?  That'll be much cleaner than the
> subtraction (that could have been -- as well).
> 
> >  			status = acpi_evaluate_object(device->handle,
> >  						      object_name, NULL, NULL);
> >  			if (ACPI_FAILURE(status)) {
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 7417267..de2ae10 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -887,8 +887,12 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> >  		/* Evaluate "_PSx" to see if we can do explicit sets */
> >  		object_name[2] = 'S';
> >  		status = acpi_get_handle(device->handle, object_name, &handle);
> > -		if (ACPI_SUCCESS(status))
> > +		if (ACPI_SUCCESS(status)) {
> >  			ps->flags.explicit_set = 1;
> > +			/* Also set D3Cold's explicit flag when _PS3 exists */
> > +			if (i == ACPI_STATE_D3_HOT)
> > +				(ps+1)->flags.explicit_set = 1;
> 
> Please don't use pointer arithmetics here.  I know it _happens_ to work,
> but I don't think it's appropriate in this situation at all.
> 

Thanks for the suggestions.

What about this?

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 3263b68..3a7860f 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -273,6 +273,9 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
 		}
 	} else {
 		if (device->power.states[state].flags.explicit_set) {
+			/* Evaluate _PS3 when entering D3cold */
+			if (state == ACPI_STATE_D3)
+				object_name[3] = '3';
 			status = acpi_evaluate_object(device->handle,
 						      object_name, NULL, NULL);
 			if (ACPI_FAILURE(status)) {
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 7417267..734d946 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -908,6 +908,10 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
 	device->power.states[ACPI_STATE_D3].flags.valid = 1;
 	device->power.states[ACPI_STATE_D3].power = 0;
 
+	/* Also set D3cold's explicit flag when _PS3 exists */
+	if (device->power.states[ACPI_STATE_D3_HOT].flags.explicit_set)
+		device->power.states[ACPI_STATE_D3].flags.explicit_set = 1;
+
 	acpi_bus_init_power(device);
 
 	return 0;

Thanks,
Aaron

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Lin Ming]

On Tue, 2012-04-24 at 10:07 +0800, Aaron Lu wrote:
> On Mon, Apr 23, 2012 at 09:50:57PM +0200, Rafael J. Wysocki wrote:
> > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > index 3263b68..187433f 100644
> > > --- a/drivers/acpi/bus.c
> > > +++ b/drivers/acpi/bus.c
> > > @@ -273,6 +273,9 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> > >  		}
> > >  	} else {
> > >  		if (device->power.states[state].flags.explicit_set) {
> > > +			/* evaluate _PS3 instead of _PS4 when entering D3Cold */
> > > +			if (state == ACPI_STATE_D3)
> > > +				object_name[3] -= 1;
> > 
> > Can you just put '3' here directly?  That'll be much cleaner than the
> > subtraction (that could have been -- as well).
> > 
> > >  			status = acpi_evaluate_object(device->handle,
> > >  						      object_name, NULL, NULL);
> > >  			if (ACPI_FAILURE(status)) {
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 7417267..de2ae10 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -887,8 +887,12 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> > >  		/* Evaluate "_PSx" to see if we can do explicit sets */
> > >  		object_name[2] = 'S';
> > >  		status = acpi_get_handle(device->handle, object_name, &handle);
> > > -		if (ACPI_SUCCESS(status))
> > > +		if (ACPI_SUCCESS(status)) {
> > >  			ps->flags.explicit_set = 1;
> > > +			/* Also set D3Cold's explicit flag when _PS3 exists */
> > > +			if (i == ACPI_STATE_D3_HOT)
> > > +				(ps+1)->flags.explicit_set = 1;
> > 
> > Please don't use pointer arithmetics here.  I know it _happens_ to work,
> > but I don't think it's appropriate in this situation at all.
> > 
> 
> Thanks for the suggestions.
> 
> What about this?
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 3263b68..3a7860f 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -273,6 +273,9 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>  		}
>  	} else {
>  		if (device->power.states[state].flags.explicit_set) {
> +			/* Evaluate _PS3 when entering D3cold */
> +			if (state == ACPI_STATE_D3)
> +				object_name[3] = '3';
>  			status = acpi_evaluate_object(device->handle,
>  						      object_name, NULL, NULL);
>  			if (ACPI_FAILURE(status)) {
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 7417267..734d946 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -908,6 +908,10 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>  	device->power.states[ACPI_STATE_D3].flags.valid = 1;
>  	device->power.states[ACPI_STATE_D3].power = 0;
>  
> +	/* Also set D3cold's explicit flag when _PS3 exists */
> +	if (device->power.states[ACPI_STATE_D3_HOT].flags.explicit_set)
> +		device->power.states[ACPI_STATE_D3].flags.explicit_set = 1;

We should use ACPI_STATE_D3_COLD instead of ACPI_STATE_D3 to make thing
clear.

> +
>  	acpi_bus_init_power(device);
>  
>  	return 0;
> 
> Thanks,
> Aaron
> 

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Aaron Lu]

On Tue, Apr 24, 2012 at 10:29:36AM +0800, Lin Ming wrote:
> On Tue, 2012-04-24 at 10:07 +0800, Aaron Lu wrote:
> > On Mon, Apr 23, 2012 at 09:50:57PM +0200, Rafael J. Wysocki wrote:
> > > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > > index 3263b68..187433f 100644
> > > > --- a/drivers/acpi/bus.c
> > > > +++ b/drivers/acpi/bus.c
> > > > @@ -273,6 +273,9 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> > > >  		}
> > > >  	} else {
> > > >  		if (device->power.states[state].flags.explicit_set) {
> > > > +			/* evaluate _PS3 instead of _PS4 when entering D3Cold */
> > > > +			if (state == ACPI_STATE_D3)
> > > > +				object_name[3] -= 1;
> > > 
> > > Can you just put '3' here directly?  That'll be much cleaner than the
> > > subtraction (that could have been -- as well).
> > > 
> > > >  			status = acpi_evaluate_object(device->handle,
> > > >  						      object_name, NULL, NULL);
> > > >  			if (ACPI_FAILURE(status)) {
> > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > > index 7417267..de2ae10 100644
> > > > --- a/drivers/acpi/scan.c
> > > > +++ b/drivers/acpi/scan.c
> > > > @@ -887,8 +887,12 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> > > >  		/* Evaluate "_PSx" to see if we can do explicit sets */
> > > >  		object_name[2] = 'S';
> > > >  		status = acpi_get_handle(device->handle, object_name, &handle);
> > > > -		if (ACPI_SUCCESS(status))
> > > > +		if (ACPI_SUCCESS(status)) {
> > > >  			ps->flags.explicit_set = 1;
> > > > +			/* Also set D3Cold's explicit flag when _PS3 exists */
> > > > +			if (i == ACPI_STATE_D3_HOT)
> > > > +				(ps+1)->flags.explicit_set = 1;
> > > 
> > > Please don't use pointer arithmetics here.  I know it _happens_ to work,
> > > but I don't think it's appropriate in this situation at all.
> > > 
> > 
> > Thanks for the suggestions.
> > 
> > What about this?
> > 
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 3263b68..3a7860f 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -273,6 +273,9 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
> >  		}
> >  	} else {
> >  		if (device->power.states[state].flags.explicit_set) {
> > +			/* Evaluate _PS3 when entering D3cold */
> > +			if (state == ACPI_STATE_D3)
> > +				object_name[3] = '3';
> >  			status = acpi_evaluate_object(device->handle,
> >  						      object_name, NULL, NULL);
> >  			if (ACPI_FAILURE(status)) {
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 7417267..734d946 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -908,6 +908,10 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> >  	device->power.states[ACPI_STATE_D3].flags.valid = 1;
> >  	device->power.states[ACPI_STATE_D3].power = 0;
> >  
> > +	/* Also set D3cold's explicit flag when _PS3 exists */
> > +	if (device->power.states[ACPI_STATE_D3_HOT].flags.explicit_set)
> > +		device->power.states[ACPI_STATE_D3].flags.explicit_set = 1;
> 
> We should use ACPI_STATE_D3_COLD instead of ACPI_STATE_D3 to make thing
> clear.
>
Thanks for your suggestion.

Well, considering all those ACPI_STATE_D3 used in tree, I don't think I
should use ACPI_STATE_D3_COLD here, especially the two lines above are
still using ACPI_STATE_D3.

But if that is desired, we should probably change all the existing
ACPI_STATE_D3 macros to ACPI_STATE_D3_COLD to make things clear.

Or we all use ACPI_STATE_D3, since it is defined as 4 and means D3 cold,
and people will learn this, won't they?

What do you think?
 
Thanks,
Aaron

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Lin Ming]

On Tue, Apr 24, 2012 at 11:10 AM, Aaron Lu <aaron.lu@amd.com> wrote:
> On Tue, Apr 24, 2012 at 10:29:36AM +0800, Lin Ming wrote:
>> On Tue, 2012-04-24 at 10:07 +0800, Aaron Lu wrote:
>> > On Mon, Apr 23, 2012 at 09:50:57PM +0200, Rafael J. Wysocki wrote:
>> > > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> > > > index 3263b68..187433f 100644
>> > > > --- a/drivers/acpi/bus.c
>> > > > +++ b/drivers/acpi/bus.c
>> > > > @@ -273,6 +273,9 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>> > > >                 }
>> > > >         } else {
>> > > >                 if (device->power.states[state].flags.explicit_set) {
>> > > > +                       /* evaluate _PS3 instead of _PS4 when entering D3Cold */
>> > > > +                       if (state == ACPI_STATE_D3)
>> > > > +                               object_name[3] -= 1;
>> > >
>> > > Can you just put '3' here directly?  That'll be much cleaner than the
>> > > subtraction (that could have been -- as well).
>> > >
>> > > >                         status = acpi_evaluate_object(device->handle,
>> > > >                                                       object_name, NULL, NULL);
>> > > >                         if (ACPI_FAILURE(status)) {
>> > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> > > > index 7417267..de2ae10 100644
>> > > > --- a/drivers/acpi/scan.c
>> > > > +++ b/drivers/acpi/scan.c
>> > > > @@ -887,8 +887,12 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>> > > >                 /* Evaluate "_PSx" to see if we can do explicit sets */
>> > > >                 object_name[2] = 'S';
>> > > >                 status = acpi_get_handle(device->handle, object_name, &handle);
>> > > > -               if (ACPI_SUCCESS(status))
>> > > > +               if (ACPI_SUCCESS(status)) {
>> > > >                         ps->flags.explicit_set = 1;
>> > > > +                       /* Also set D3Cold's explicit flag when _PS3 exists */
>> > > > +                       if (i == ACPI_STATE_D3_HOT)
>> > > > +                               (ps+1)->flags.explicit_set = 1;
>> > >
>> > > Please don't use pointer arithmetics here.  I know it _happens_ to work,
>> > > but I don't think it's appropriate in this situation at all.
>> > >
>> >
>> > Thanks for the suggestions.
>> >
>> > What about this?
>> >
>> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> > index 3263b68..3a7860f 100644
>> > --- a/drivers/acpi/bus.c
>> > +++ b/drivers/acpi/bus.c
>> > @@ -273,6 +273,9 @@ static int __acpi_bus_set_power(struct acpi_device *device, int state)
>> >             }
>> >     } else {
>> >             if (device->power.states[state].flags.explicit_set) {
>> > +                   /* Evaluate _PS3 when entering D3cold */
>> > +                   if (state == ACPI_STATE_D3)
>> > +                           object_name[3] = '3';
>> >                     status = acpi_evaluate_object(device->handle,
>> >                                                   object_name, NULL, NULL);
>> >                     if (ACPI_FAILURE(status)) {
>> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> > index 7417267..734d946 100644
>> > --- a/drivers/acpi/scan.c
>> > +++ b/drivers/acpi/scan.c
>> > @@ -908,6 +908,10 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>> >     device->power.states[ACPI_STATE_D3].flags.valid = 1;
>> >     device->power.states[ACPI_STATE_D3].power = 0;
>> >
>> > +   /* Also set D3cold's explicit flag when _PS3 exists */
>> > +   if (device->power.states[ACPI_STATE_D3_HOT].flags.explicit_set)
>> > +           device->power.states[ACPI_STATE_D3].flags.explicit_set = 1;
>>
>> We should use ACPI_STATE_D3_COLD instead of ACPI_STATE_D3 to make thing
>> clear.
>>
> Thanks for your suggestion.
>
> Well, considering all those ACPI_STATE_D3 used in tree, I don't think I
> should use ACPI_STATE_D3_COLD here, especially the two lines above are
> still using ACPI_STATE_D3.
>
> But if that is desired, we should probably change all the existing
> ACPI_STATE_D3 macros to ACPI_STATE_D3_COLD to make things clear.
>
> Or we all use ACPI_STATE_D3, since it is defined as 4 and means D3 cold,
> and people will learn this, won't they?
>
> What do you think?

OK.

But we may do more cleanup later.

>
> Thanks,
> Aaron

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Aaron Lu]

On Tue, Apr 24, 2012 at 09:15:37PM +0800, Lin Ming wrote:
> >> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >> > index 7417267..734d946 100644
> >> > --- a/drivers/acpi/scan.c
> >> > +++ b/drivers/acpi/scan.c
> >> > @@ -908,6 +908,10 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> >> >     device->power.states[ACPI_STATE_D3].flags.valid = 1;
> >> >     device->power.states[ACPI_STATE_D3].power = 0;
> >> >
> >> > +   /* Also set D3cold's explicit flag when _PS3 exists */
> >> > +   if (device->power.states[ACPI_STATE_D3_HOT].flags.explicit_set)
> >> > +           device->power.states[ACPI_STATE_D3].flags.explicit_set = 1;
> >>
> >> We should use ACPI_STATE_D3_COLD instead of ACPI_STATE_D3 to make thing
> >> clear.
> >>
> > Thanks for your suggestion.
> >
> > Well, considering all those ACPI_STATE_D3 used in tree, I don't think I
> > should use ACPI_STATE_D3_COLD here, especially the two lines above are
> > still using ACPI_STATE_D3.
> >
> > But if that is desired, we should probably change all the existing
> > ACPI_STATE_D3 macros to ACPI_STATE_D3_COLD to make things clear.
> >
> > Or we all use ACPI_STATE_D3, since it is defined as 4 and means D3 cold,
> > and people will learn this, won't they?
> >
> > What do you think?
> 
> OK.
> 
> But we may do more cleanup later.
> 

Sure :-)
I think the first thing to do is to agree on which macro should be used,
ACPI_STATE_D3 or ACPI_STATE_D3_COLD. And then replace the other one
altogether.

Thanks,
Aaron

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Tuesday, April 24, 2012, Aaron Lu wrote:
> On Tue, Apr 24, 2012 at 09:15:37PM +0800, Lin Ming wrote:
> > >> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > >> > index 7417267..734d946 100644
> > >> > --- a/drivers/acpi/scan.c
> > >> > +++ b/drivers/acpi/scan.c
> > >> > @@ -908,6 +908,10 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> > >> >     device->power.states[ACPI_STATE_D3].flags.valid = 1;
> > >> >     device->power.states[ACPI_STATE_D3].power = 0;
> > >> >
> > >> > +   /* Also set D3cold's explicit flag when _PS3 exists */
> > >> > +   if (device->power.states[ACPI_STATE_D3_HOT].flags.explicit_set)
> > >> > +           device->power.states[ACPI_STATE_D3].flags.explicit_set = 1;
> > >>
> > >> We should use ACPI_STATE_D3_COLD instead of ACPI_STATE_D3 to make thing
> > >> clear.
> > >>
> > > Thanks for your suggestion.
> > >
> > > Well, considering all those ACPI_STATE_D3 used in tree, I don't think I
> > > should use ACPI_STATE_D3_COLD here, especially the two lines above are
> > > still using ACPI_STATE_D3.
> > >
> > > But if that is desired, we should probably change all the existing
> > > ACPI_STATE_D3 macros to ACPI_STATE_D3_COLD to make things clear.
> > >
> > > Or we all use ACPI_STATE_D3, since it is defined as 4 and means D3 cold,
> > > and people will learn this, won't they?
> > >
> > > What do you think?
> > 
> > OK.
> > 
> > But we may do more cleanup later.
> > 
> 
> Sure :-)
> I think the first thing to do is to agree on which macro should be used,
> ACPI_STATE_D3 or ACPI_STATE_D3_COLD. And then replace the other one
> altogether.

I'd prefer to use ACPI_STATE_D3 wherever it makes sense, because that's
also well defined in older versions of ACPI (think pre-ACPI-4.0 BIOSes).

Thanks,
Rafael

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[huang ying]

On Wed, Apr 25, 2012 at 5:15 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday, April 24, 2012, Aaron Lu wrote:
>> On Tue, Apr 24, 2012 at 09:15:37PM +0800, Lin Ming wrote:
>> > >> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> > >> > index 7417267..734d946 100644
>> > >> > --- a/drivers/acpi/scan.c
>> > >> > +++ b/drivers/acpi/scan.c
>> > >> > @@ -908,6 +908,10 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>> > >> >     device->power.states[ACPI_STATE_D3].flags.valid = 1;
>> > >> >     device->power.states[ACPI_STATE_D3].power = 0;
>> > >> >
>> > >> > +   /* Also set D3cold's explicit flag when _PS3 exists */
>> > >> > +   if (device->power.states[ACPI_STATE_D3_HOT].flags.explicit_set)
>> > >> > +           device->power.states[ACPI_STATE_D3].flags.explicit_set = 1;
>> > >>
>> > >> We should use ACPI_STATE_D3_COLD instead of ACPI_STATE_D3 to make thing
>> > >> clear.
>> > >>
>> > > Thanks for your suggestion.
>> > >
>> > > Well, considering all those ACPI_STATE_D3 used in tree, I don't think I
>> > > should use ACPI_STATE_D3_COLD here, especially the two lines above are
>> > > still using ACPI_STATE_D3.
>> > >
>> > > But if that is desired, we should probably change all the existing
>> > > ACPI_STATE_D3 macros to ACPI_STATE_D3_COLD to make things clear.
>> > >
>> > > Or we all use ACPI_STATE_D3, since it is defined as 4 and means D3 cold,
>> > > and people will learn this, won't they?
>> > >
>> > > What do you think?
>> >
>> > OK.
>> >
>> > But we may do more cleanup later.
>> >
>>
>> Sure :-)
>> I think the first thing to do is to agree on which macro should be used,
>> ACPI_STATE_D3 or ACPI_STATE_D3_COLD. And then replace the other one
>> altogether.
>
> I'd prefer to use ACPI_STATE_D3 wherever it makes sense, because that's
> also well defined in older versions of ACPI (think pre-ACPI-4.0 BIOSes).

Because the relationship between ACPI_STATE_D3 and ACPI_STATE_D3_COLD
is confusing, I suggest to add some comments whenever will refer to D3
or D3_COLD.

Best Regards,
Huang Ying

Re: [PATCH] ACPI: evaluate _PS3 when entering D3 Cold

[Rafael J. Wysocki]

On Thursday, April 26, 2012, huang ying wrote:
> On Wed, Apr 25, 2012 at 5:15 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Tuesday, April 24, 2012, Aaron Lu wrote:
> >> On Tue, Apr 24, 2012 at 09:15:37PM +0800, Lin Ming wrote:
> >> > >> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >> > >> > index 7417267..734d946 100644
> >> > >> > --- a/drivers/acpi/scan.c
> >> > >> > +++ b/drivers/acpi/scan.c
> >> > >> > @@ -908,6 +908,10 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> >> > >> >     device->power.states[ACPI_STATE_D3].flags.valid = 1;
> >> > >> >     device->power.states[ACPI_STATE_D3].power = 0;
> >> > >> >
> >> > >> > +   /* Also set D3cold's explicit flag when _PS3 exists */
> >> > >> > +   if (device->power.states[ACPI_STATE_D3_HOT].flags.explicit_set)
> >> > >> > +           device->power.states[ACPI_STATE_D3].flags.explicit_set = 1;
> >> > >>
> >> > >> We should use ACPI_STATE_D3_COLD instead of ACPI_STATE_D3 to make thing
> >> > >> clear.
> >> > >>
> >> > > Thanks for your suggestion.
> >> > >
> >> > > Well, considering all those ACPI_STATE_D3 used in tree, I don't think I
> >> > > should use ACPI_STATE_D3_COLD here, especially the two lines above are
> >> > > still using ACPI_STATE_D3.
> >> > >
> >> > > But if that is desired, we should probably change all the existing
> >> > > ACPI_STATE_D3 macros to ACPI_STATE_D3_COLD to make things clear.
> >> > >
> >> > > Or we all use ACPI_STATE_D3, since it is defined as 4 and means D3 cold,
> >> > > and people will learn this, won't they?
> >> > >
> >> > > What do you think?
> >> >
> >> > OK.
> >> >
> >> > But we may do more cleanup later.
> >> >
> >>
> >> Sure :-)
> >> I think the first thing to do is to agree on which macro should be used,
> >> ACPI_STATE_D3 or ACPI_STATE_D3_COLD. And then replace the other one
> >> altogether.
> >
> > I'd prefer to use ACPI_STATE_D3 wherever it makes sense, because that's
> > also well defined in older versions of ACPI (think pre-ACPI-4.0 BIOSes).
> 
> Because the relationship between ACPI_STATE_D3 and ACPI_STATE_D3_COLD
> is confusing, I suggest to add some comments whenever will refer to D3
> or D3_COLD.

Oh well.

This is how it works: D3 _is_ D3_COLD except when D3_HOT is supported
(ie. _PR3 is present) in which cases saying just "D3" may be ambiguous (quite
obviously).  I think that's clear enough.

Thanks,
Rafael