[PATCH v2] ACPI: D3 states cleanup

[PATCH v2] ACPI: D3 states cleanup

[Lin Ming]

There are two ACPI D3 states defined now:
ACPI_STATE_D3 and ACPI_STATE_D3_COLD.

But the uses of these states are not clear/correct in some code.
For example, some code refer ACPI_STATE_D3 as D3hot and others refer
it as D3cold.

This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/acpi/power.c   |    2 +-
 drivers/acpi/scan.c    |    9 +--------
 drivers/pci/pci-acpi.c |    4 ++--
 include/acpi/actypes.h |    7 ++++---
 4 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index 7049a7d..330bb4d 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -631,7 +631,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)
 	 * We know a device's inferred power state when all the resources
 	 * required for a given D-state are 'on'.
 	 */
-	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3; i++) {
+	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3_HOT; i++) {
 		list = &device->power.states[i].resources;
 		if (list->count < 1)
 			continue;
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 767e2dc..fbf38ad 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -869,7 +869,7 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
 	/*
 	 * Enumerate supported power management states
 	 */
-	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3; i++) {
+	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3_HOT; i++) {
 		struct acpi_device_power_state *ps = &device->power.states[i];
 		char object_name[5] = { '_', 'P', 'R', '0' + i, '\0' };
 
@@ -884,13 +884,6 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
 				acpi_bus_add_power_resource(ps->resources.handles[j]);
 		}
 
-		/* The exist of _PR3 indicates D3Cold support */
-		if (i == ACPI_STATE_D3) {
-			status = acpi_get_handle(device->handle, object_name, &handle);
-			if (ACPI_SUCCESS(status))
-				device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
-		}
-
 		/* Evaluate "_PSx" to see if we can do explicit sets */
 		object_name[2] = 'S';
 		status = acpi_get_handle(device->handle, object_name, &handle);
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 0f150f2..1929c0c 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -200,7 +200,7 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
 		return PCI_D1;
 	case ACPI_STATE_D2:
 		return PCI_D2;
-	case ACPI_STATE_D3:
+	case ACPI_STATE_D3_HOT:
 		return PCI_D3hot;
 	case ACPI_STATE_D3_COLD:
 		return PCI_D3cold;
@@ -223,7 +223,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		[PCI_D0] = ACPI_STATE_D0,
 		[PCI_D1] = ACPI_STATE_D1,
 		[PCI_D2] = ACPI_STATE_D2,
-		[PCI_D3hot] = ACPI_STATE_D3,
+		[PCI_D3hot] = ACPI_STATE_D3_HOT,
 		[PCI_D3cold] = ACPI_STATE_D3
 	};
 	int error = -EINVAL;
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index eba6604..e8bcc47 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -499,9 +499,10 @@ typedef u64 acpi_integer;
 #define ACPI_STATE_D0                   (u8) 0
 #define ACPI_STATE_D1                   (u8) 1
 #define ACPI_STATE_D2                   (u8) 2
-#define ACPI_STATE_D3                   (u8) 3
-#define ACPI_STATE_D3_COLD              (u8) 4
-#define ACPI_D_STATES_MAX               ACPI_STATE_D3_COLD
+#define ACPI_STATE_D3_HOT               (u8) 3
+#define ACPI_STATE_D3                   (u8) 4
+#define ACPI_STATE_D3_COLD              ACPI_STATE_D3
+#define ACPI_D_STATES_MAX               ACPI_STATE_D3
 #define ACPI_D_STATE_COUNT              5
 
 #define ACPI_STATE_C0                   (u8) 0
-- 
1.7.2.5

Re: [PATCH v2] ACPI: D3 states cleanup

[Aaron Lu]

Hi,

On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote:
> There are two ACPI D3 states defined now:
> ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
> 
> But the uses of these states are not clear/correct in some code.
> For example, some code refer ACPI_STATE_D3 as D3hot and others refer
> it as D3cold.
> 
> This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
> And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
> Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
> 

With this patch now, if a device has _PS3, then we will set its D3hot
flag valid. This doesn't feel right to me, since per our discussion the
other day, we should assume _PS3 will put the device into D3cold.

Or do you mean: if _PS3 is available, then both D3hot and D3cold is
valid from the perspective of acpi, it is the individual driver's
responsibility to decide which state is actually valid and will be used.

-Aaron

> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  drivers/acpi/power.c   |    2 +-
>  drivers/acpi/scan.c    |    9 +--------
>  drivers/pci/pci-acpi.c |    4 ++--
>  include/acpi/actypes.h |    7 ++++---
>  4 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> index 7049a7d..330bb4d 100644
> --- a/drivers/acpi/power.c
> +++ b/drivers/acpi/power.c
> @@ -631,7 +631,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)
>  	 * We know a device's inferred power state when all the resources
>  	 * required for a given D-state are 'on'.
>  	 */
> -	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3; i++) {
> +	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3_HOT; i++) {
>  		list = &device->power.states[i].resources;
>  		if (list->count < 1)
>  			continue;
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 767e2dc..fbf38ad 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -869,7 +869,7 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>  	/*
>  	 * Enumerate supported power management states
>  	 */
> -	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3; i++) {
> +	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3_HOT; i++) {
>  		struct acpi_device_power_state *ps = &device->power.states[i];
>  		char object_name[5] = { '_', 'P', 'R', '0' + i, '\0' };
>  
> @@ -884,13 +884,6 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>  				acpi_bus_add_power_resource(ps->resources.handles[j]);
>  		}
>  
> -		/* The exist of _PR3 indicates D3Cold support */
> -		if (i == ACPI_STATE_D3) {
> -			status = acpi_get_handle(device->handle, object_name, &handle);
> -			if (ACPI_SUCCESS(status))
> -				device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> -		}
> -
>  		/* Evaluate "_PSx" to see if we can do explicit sets */
>  		object_name[2] = 'S';
>  		status = acpi_get_handle(device->handle, object_name, &handle);
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 0f150f2..1929c0c 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -200,7 +200,7 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
>  		return PCI_D1;
>  	case ACPI_STATE_D2:
>  		return PCI_D2;
> -	case ACPI_STATE_D3:
> +	case ACPI_STATE_D3_HOT:
>  		return PCI_D3hot;
>  	case ACPI_STATE_D3_COLD:
>  		return PCI_D3cold;
> @@ -223,7 +223,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>  		[PCI_D0] = ACPI_STATE_D0,
>  		[PCI_D1] = ACPI_STATE_D1,
>  		[PCI_D2] = ACPI_STATE_D2,
> -		[PCI_D3hot] = ACPI_STATE_D3,
> +		[PCI_D3hot] = ACPI_STATE_D3_HOT,
>  		[PCI_D3cold] = ACPI_STATE_D3
>  	};
>  	int error = -EINVAL;
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index eba6604..e8bcc47 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -499,9 +499,10 @@ typedef u64 acpi_integer;
>  #define ACPI_STATE_D0                   (u8) 0
>  #define ACPI_STATE_D1                   (u8) 1
>  #define ACPI_STATE_D2                   (u8) 2
> -#define ACPI_STATE_D3                   (u8) 3
> -#define ACPI_STATE_D3_COLD              (u8) 4
> -#define ACPI_D_STATES_MAX               ACPI_STATE_D3_COLD
> +#define ACPI_STATE_D3_HOT               (u8) 3
> +#define ACPI_STATE_D3                   (u8) 4
> +#define ACPI_STATE_D3_COLD              ACPI_STATE_D3
> +#define ACPI_D_STATES_MAX               ACPI_STATE_D3
>  #define ACPI_D_STATE_COUNT              5
>  
>  #define ACPI_STATE_C0                   (u8) 0
> -- 
> 1.7.2.5
> 
> 
> 
> 

Re: [PATCH v2] ACPI: D3 states cleanup

[Aaron Lu]

Hi,

On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote:
> There are two ACPI D3 states defined now:
> ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
> 
> But the uses of these states are not clear/correct in some code.
> For example, some code refer ACPI_STATE_D3 as D3hot and others refer
> it as D3cold.
> 
> This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
> And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
> Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
> 

With this patch now, if a device has _PS3, then we will set its D3hot
flag valid. This doesn't feel right to me, since per our discussion the
other day, we should assume _PS3 will put the device into D3cold.

Or do you mean: if _PS3 is available, then both D3hot and D3cold is
valid from the perspective of acpi, it is the individual driver's
responsibility to decide which state is actually valid and will be used.

-Aaron

> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  drivers/acpi/power.c   |    2 +-
>  drivers/acpi/scan.c    |    9 +--------
>  drivers/pci/pci-acpi.c |    4 ++--
>  include/acpi/actypes.h |    7 ++++---
>  4 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> index 7049a7d..330bb4d 100644
> --- a/drivers/acpi/power.c
> +++ b/drivers/acpi/power.c
> @@ -631,7 +631,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)
>  	 * We know a device's inferred power state when all the resources
>  	 * required for a given D-state are 'on'.
>  	 */
> -	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3; i++) {
> +	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3_HOT; i++) {
>  		list = &device->power.states[i].resources;
>  		if (list->count < 1)
>  			continue;
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 767e2dc..fbf38ad 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -869,7 +869,7 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>  	/*
>  	 * Enumerate supported power management states
>  	 */
> -	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3; i++) {
> +	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3_HOT; i++) {
>  		struct acpi_device_power_state *ps = &device->power.states[i];
>  		char object_name[5] = { '_', 'P', 'R', '0' + i, '\0' };
>  
> @@ -884,13 +884,6 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>  				acpi_bus_add_power_resource(ps->resources.handles[j]);
>  		}
>  
> -		/* The exist of _PR3 indicates D3Cold support */
> -		if (i == ACPI_STATE_D3) {
> -			status = acpi_get_handle(device->handle, object_name, &handle);
> -			if (ACPI_SUCCESS(status))
> -				device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> -		}
> -
>  		/* Evaluate "_PSx" to see if we can do explicit sets */
>  		object_name[2] = 'S';
>  		status = acpi_get_handle(device->handle, object_name, &handle);
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 0f150f2..1929c0c 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -200,7 +200,7 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
>  		return PCI_D1;
>  	case ACPI_STATE_D2:
>  		return PCI_D2;
> -	case ACPI_STATE_D3:
> +	case ACPI_STATE_D3_HOT:
>  		return PCI_D3hot;
>  	case ACPI_STATE_D3_COLD:
>  		return PCI_D3cold;
> @@ -223,7 +223,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>  		[PCI_D0] = ACPI_STATE_D0,
>  		[PCI_D1] = ACPI_STATE_D1,
>  		[PCI_D2] = ACPI_STATE_D2,
> -		[PCI_D3hot] = ACPI_STATE_D3,
> +		[PCI_D3hot] = ACPI_STATE_D3_HOT,
>  		[PCI_D3cold] = ACPI_STATE_D3
>  	};
>  	int error = -EINVAL;
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index eba6604..e8bcc47 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -499,9 +499,10 @@ typedef u64 acpi_integer;
>  #define ACPI_STATE_D0                   (u8) 0
>  #define ACPI_STATE_D1                   (u8) 1
>  #define ACPI_STATE_D2                   (u8) 2
> -#define ACPI_STATE_D3                   (u8) 3
> -#define ACPI_STATE_D3_COLD              (u8) 4
> -#define ACPI_D_STATES_MAX               ACPI_STATE_D3_COLD
> +#define ACPI_STATE_D3_HOT               (u8) 3
> +#define ACPI_STATE_D3                   (u8) 4
> +#define ACPI_STATE_D3_COLD              ACPI_STATE_D3
> +#define ACPI_D_STATES_MAX               ACPI_STATE_D3
>  #define ACPI_D_STATE_COUNT              5
>  
>  #define ACPI_STATE_C0                   (u8) 0
> -- 
> 1.7.2.5
> 
> 
> 
> 

Re: [PATCH v2] ACPI: D3 states cleanup

[Lin Ming]

On Fri, 2012-04-20 at 10:23 +0800, Aaron Lu wrote:
> Hi,
> 
> On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote:
> > There are two ACPI D3 states defined now:
> > ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
> > 
> > But the uses of these states are not clear/correct in some code.
> > For example, some code refer ACPI_STATE_D3 as D3hot and others refer
> > it as D3cold.
> > 
> > This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
> > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
> > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
> > 
> 
> With this patch now, if a device has _PS3, then we will set its D3hot
> flag valid. This doesn't feel right to me, since per our discussion the
> other day, we should assume _PS3 will put the device into D3cold.
> 
> Or do you mean: if _PS3 is available, then both D3hot and D3cold is
> valid from the perspective of acpi, it is the individual driver's
> responsibility to decide which state is actually valid and will be used.

Right.

ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.

Lin Ming

> 
> -Aaron
> 
> > Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > ---
> >  drivers/acpi/power.c   |    2 +-
> >  drivers/acpi/scan.c    |    9 +--------
> >  drivers/pci/pci-acpi.c |    4 ++--
> >  include/acpi/actypes.h |    7 ++++---
> >  4 files changed, 8 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> > index 7049a7d..330bb4d 100644
> > --- a/drivers/acpi/power.c
> > +++ b/drivers/acpi/power.c
> > @@ -631,7 +631,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)
> >  	 * We know a device's inferred power state when all the resources
> >  	 * required for a given D-state are 'on'.
> >  	 */
> > -	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3; i++) {
> > +	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3_HOT; i++) {
> >  		list = &device->power.states[i].resources;
> >  		if (list->count < 1)
> >  			continue;
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 767e2dc..fbf38ad 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -869,7 +869,7 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> >  	/*
> >  	 * Enumerate supported power management states
> >  	 */
> > -	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3; i++) {
> > +	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3_HOT; i++) {
> >  		struct acpi_device_power_state *ps = &device->power.states[i];
> >  		char object_name[5] = { '_', 'P', 'R', '0' + i, '\0' };
> >  
> > @@ -884,13 +884,6 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> >  				acpi_bus_add_power_resource(ps->resources.handles[j]);
> >  		}
> >  
> > -		/* The exist of _PR3 indicates D3Cold support */
> > -		if (i == ACPI_STATE_D3) {
> > -			status = acpi_get_handle(device->handle, object_name, &handle);
> > -			if (ACPI_SUCCESS(status))
> > -				device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> > -		}
> > -
> >  		/* Evaluate "_PSx" to see if we can do explicit sets */
> >  		object_name[2] = 'S';
> >  		status = acpi_get_handle(device->handle, object_name, &handle);
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 0f150f2..1929c0c 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -200,7 +200,7 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
> >  		return PCI_D1;
> >  	case ACPI_STATE_D2:
> >  		return PCI_D2;
> > -	case ACPI_STATE_D3:
> > +	case ACPI_STATE_D3_HOT:
> >  		return PCI_D3hot;
> >  	case ACPI_STATE_D3_COLD:
> >  		return PCI_D3cold;
> > @@ -223,7 +223,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> >  		[PCI_D0] = ACPI_STATE_D0,
> >  		[PCI_D1] = ACPI_STATE_D1,
> >  		[PCI_D2] = ACPI_STATE_D2,
> > -		[PCI_D3hot] = ACPI_STATE_D3,
> > +		[PCI_D3hot] = ACPI_STATE_D3_HOT,
> >  		[PCI_D3cold] = ACPI_STATE_D3
> >  	};
> >  	int error = -EINVAL;
> > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> > index eba6604..e8bcc47 100644
> > --- a/include/acpi/actypes.h
> > +++ b/include/acpi/actypes.h
> > @@ -499,9 +499,10 @@ typedef u64 acpi_integer;
> >  #define ACPI_STATE_D0                   (u8) 0
> >  #define ACPI_STATE_D1                   (u8) 1
> >  #define ACPI_STATE_D2                   (u8) 2
> > -#define ACPI_STATE_D3                   (u8) 3
> > -#define ACPI_STATE_D3_COLD              (u8) 4
> > -#define ACPI_D_STATES_MAX               ACPI_STATE_D3_COLD
> > +#define ACPI_STATE_D3_HOT               (u8) 3
> > +#define ACPI_STATE_D3                   (u8) 4
> > +#define ACPI_STATE_D3_COLD              ACPI_STATE_D3
> > +#define ACPI_D_STATES_MAX               ACPI_STATE_D3
> >  #define ACPI_D_STATE_COUNT              5
> >  
> >  #define ACPI_STATE_C0                   (u8) 0
> > -- 
> > 1.7.2.5
> > 
> > 
> > 
> > 
> 

Re: [PATCH v2] ACPI: D3 states cleanup

[Aaron Lu]

On Fri, Apr 20, 2012 at 11:19:13AM +0800, Lin Ming wrote:
> On Fri, 2012-04-20 at 10:23 +0800, Aaron Lu wrote:
> > Hi,
> > 
> > On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote:
> > > There are two ACPI D3 states defined now:
> > > ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
> > > 
> > > But the uses of these states are not clear/correct in some code.
> > > For example, some code refer ACPI_STATE_D3 as D3hot and others refer
> > > it as D3cold.
> > > 
> > > This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
> > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
> > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
> > > 
> > 
> > With this patch now, if a device has _PS3, then we will set its D3hot
> > flag valid. This doesn't feel right to me, since per our discussion the
> > other day, we should assume _PS3 will put the device into D3cold.
> > 
> > Or do you mean: if _PS3 is available, then both D3hot and D3cold is
> > valid from the perspective of acpi, it is the individual driver's
> > responsibility to decide which state is actually valid and will be used.
> 
> Right.
> 
> ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
> 

I mean, if _PS3 is available, can we say D3hot is valid?

-Aaron

> Lin Ming
> 
> > 
> > -Aaron
> > 
> > > Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > > ---
> > >  drivers/acpi/power.c   |    2 +-
> > >  drivers/acpi/scan.c    |    9 +--------
> > >  drivers/pci/pci-acpi.c |    4 ++--
> > >  include/acpi/actypes.h |    7 ++++---
> > >  4 files changed, 8 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> > > index 7049a7d..330bb4d 100644
> > > --- a/drivers/acpi/power.c
> > > +++ b/drivers/acpi/power.c
> > > @@ -631,7 +631,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)
> > >  	 * We know a device's inferred power state when all the resources
> > >  	 * required for a given D-state are 'on'.
> > >  	 */
> > > -	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3; i++) {
> > > +	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3_HOT; i++) {
> > >  		list = &device->power.states[i].resources;
> > >  		if (list->count < 1)
> > >  			continue;
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 767e2dc..fbf38ad 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -869,7 +869,7 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> > >  	/*
> > >  	 * Enumerate supported power management states
> > >  	 */
> > > -	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3; i++) {
> > > +	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3_HOT; i++) {
> > >  		struct acpi_device_power_state *ps = &device->power.states[i];
> > >  		char object_name[5] = { '_', 'P', 'R', '0' + i, '\0' };
> > >  
> > > @@ -884,13 +884,6 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> > >  				acpi_bus_add_power_resource(ps->resources.handles[j]);
> > >  		}
> > >  
> > > -		/* The exist of _PR3 indicates D3Cold support */
> > > -		if (i == ACPI_STATE_D3) {
> > > -			status = acpi_get_handle(device->handle, object_name, &handle);
> > > -			if (ACPI_SUCCESS(status))
> > > -				device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> > > -		}
> > > -
> > >  		/* Evaluate "_PSx" to see if we can do explicit sets */
> > >  		object_name[2] = 'S';
> > >  		status = acpi_get_handle(device->handle, object_name, &handle);
> > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > index 0f150f2..1929c0c 100644
> > > --- a/drivers/pci/pci-acpi.c
> > > +++ b/drivers/pci/pci-acpi.c
> > > @@ -200,7 +200,7 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
> > >  		return PCI_D1;
> > >  	case ACPI_STATE_D2:
> > >  		return PCI_D2;
> > > -	case ACPI_STATE_D3:
> > > +	case ACPI_STATE_D3_HOT:
> > >  		return PCI_D3hot;
> > >  	case ACPI_STATE_D3_COLD:
> > >  		return PCI_D3cold;
> > > @@ -223,7 +223,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> > >  		[PCI_D0] = ACPI_STATE_D0,
> > >  		[PCI_D1] = ACPI_STATE_D1,
> > >  		[PCI_D2] = ACPI_STATE_D2,
> > > -		[PCI_D3hot] = ACPI_STATE_D3,
> > > +		[PCI_D3hot] = ACPI_STATE_D3_HOT,
> > >  		[PCI_D3cold] = ACPI_STATE_D3
> > >  	};
> > >  	int error = -EINVAL;
> > > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> > > index eba6604..e8bcc47 100644
> > > --- a/include/acpi/actypes.h
> > > +++ b/include/acpi/actypes.h
> > > @@ -499,9 +499,10 @@ typedef u64 acpi_integer;
> > >  #define ACPI_STATE_D0                   (u8) 0
> > >  #define ACPI_STATE_D1                   (u8) 1
> > >  #define ACPI_STATE_D2                   (u8) 2
> > > -#define ACPI_STATE_D3                   (u8) 3
> > > -#define ACPI_STATE_D3_COLD              (u8) 4
> > > -#define ACPI_D_STATES_MAX               ACPI_STATE_D3_COLD
> > > +#define ACPI_STATE_D3_HOT               (u8) 3
> > > +#define ACPI_STATE_D3                   (u8) 4
> > > +#define ACPI_STATE_D3_COLD              ACPI_STATE_D3
> > > +#define ACPI_D_STATES_MAX               ACPI_STATE_D3
> > >  #define ACPI_D_STATE_COUNT              5
> > >  
> > >  #define ACPI_STATE_C0                   (u8) 0
> > > -- 
> > > 1.7.2.5
> > > 
> > > 
> > > 
> > > 
> > 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH v2] ACPI: D3 states cleanup

[Aaron Lu]

On Fri, Apr 20, 2012 at 11:19:13AM +0800, Lin Ming wrote:
> On Fri, 2012-04-20 at 10:23 +0800, Aaron Lu wrote:
> > Hi,
> > 
> > On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote:
> > > There are two ACPI D3 states defined now:
> > > ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
> > > 
> > > But the uses of these states are not clear/correct in some code.
> > > For example, some code refer ACPI_STATE_D3 as D3hot and others refer
> > > it as D3cold.
> > > 
> > > This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
> > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
> > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
> > > 
> > 
> > With this patch now, if a device has _PS3, then we will set its D3hot
> > flag valid. This doesn't feel right to me, since per our discussion the
> > other day, we should assume _PS3 will put the device into D3cold.
> > 
> > Or do you mean: if _PS3 is available, then both D3hot and D3cold is
> > valid from the perspective of acpi, it is the individual driver's
> > responsibility to decide which state is actually valid and will be used.
> 
> Right.
> 
> ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
> 

I mean, if _PS3 is available, can we say D3hot is valid?

-Aaron

> Lin Ming
> 
> > 
> > -Aaron
> > 
> > > Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > > ---
> > >  drivers/acpi/power.c   |    2 +-
> > >  drivers/acpi/scan.c    |    9 +--------
> > >  drivers/pci/pci-acpi.c |    4 ++--
> > >  include/acpi/actypes.h |    7 ++++---
> > >  4 files changed, 8 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> > > index 7049a7d..330bb4d 100644
> > > --- a/drivers/acpi/power.c
> > > +++ b/drivers/acpi/power.c
> > > @@ -631,7 +631,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)
> > >  	 * We know a device's inferred power state when all the resources
> > >  	 * required for a given D-state are 'on'.
> > >  	 */
> > > -	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3; i++) {
> > > +	for (i = ACPI_STATE_D0; i < ACPI_STATE_D3_HOT; i++) {
> > >  		list = &device->power.states[i].resources;
> > >  		if (list->count < 1)
> > >  			continue;
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 767e2dc..fbf38ad 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -869,7 +869,7 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> > >  	/*
> > >  	 * Enumerate supported power management states
> > >  	 */
> > > -	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3; i++) {
> > > +	for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3_HOT; i++) {
> > >  		struct acpi_device_power_state *ps = &device->power.states[i];
> > >  		char object_name[5] = { '_', 'P', 'R', '0' + i, '\0' };
> > >  
> > > @@ -884,13 +884,6 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> > >  				acpi_bus_add_power_resource(ps->resources.handles[j]);
> > >  		}
> > >  
> > > -		/* The exist of _PR3 indicates D3Cold support */
> > > -		if (i == ACPI_STATE_D3) {
> > > -			status = acpi_get_handle(device->handle, object_name, &handle);
> > > -			if (ACPI_SUCCESS(status))
> > > -				device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> > > -		}
> > > -
> > >  		/* Evaluate "_PSx" to see if we can do explicit sets */
> > >  		object_name[2] = 'S';
> > >  		status = acpi_get_handle(device->handle, object_name, &handle);
> > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > index 0f150f2..1929c0c 100644
> > > --- a/drivers/pci/pci-acpi.c
> > > +++ b/drivers/pci/pci-acpi.c
> > > @@ -200,7 +200,7 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
> > >  		return PCI_D1;
> > >  	case ACPI_STATE_D2:
> > >  		return PCI_D2;
> > > -	case ACPI_STATE_D3:
> > > +	case ACPI_STATE_D3_HOT:
> > >  		return PCI_D3hot;
> > >  	case ACPI_STATE_D3_COLD:
> > >  		return PCI_D3cold;
> > > @@ -223,7 +223,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> > >  		[PCI_D0] = ACPI_STATE_D0,
> > >  		[PCI_D1] = ACPI_STATE_D1,
> > >  		[PCI_D2] = ACPI_STATE_D2,
> > > -		[PCI_D3hot] = ACPI_STATE_D3,
> > > +		[PCI_D3hot] = ACPI_STATE_D3_HOT,
> > >  		[PCI_D3cold] = ACPI_STATE_D3
> > >  	};
> > >  	int error = -EINVAL;
> > > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> > > index eba6604..e8bcc47 100644
> > > --- a/include/acpi/actypes.h
> > > +++ b/include/acpi/actypes.h
> > > @@ -499,9 +499,10 @@ typedef u64 acpi_integer;
> > >  #define ACPI_STATE_D0                   (u8) 0
> > >  #define ACPI_STATE_D1                   (u8) 1
> > >  #define ACPI_STATE_D2                   (u8) 2
> > > -#define ACPI_STATE_D3                   (u8) 3
> > > -#define ACPI_STATE_D3_COLD              (u8) 4
> > > -#define ACPI_D_STATES_MAX               ACPI_STATE_D3_COLD
> > > +#define ACPI_STATE_D3_HOT               (u8) 3
> > > +#define ACPI_STATE_D3                   (u8) 4
> > > +#define ACPI_STATE_D3_COLD              ACPI_STATE_D3
> > > +#define ACPI_D_STATES_MAX               ACPI_STATE_D3
> > >  #define ACPI_D_STATE_COUNT              5
> > >  
> > >  #define ACPI_STATE_C0                   (u8) 0
> > > -- 
> > > 1.7.2.5
> > > 
> > > 
> > > 
> > > 
> > 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH v2] ACPI: D3 states cleanup

[Lin Ming]

On Fri, 2012-04-20 at 13:23 +0800, Aaron Lu wrote:
> On Fri, Apr 20, 2012 at 11:19:13AM +0800, Lin Ming wrote:
> > On Fri, 2012-04-20 at 10:23 +0800, Aaron Lu wrote:
> > > Hi,
> > > 
> > > On Fri, Apr 20, 2012 at 09:19:09AM +0800, Lin Ming wrote:
> > > > There are two ACPI D3 states defined now:
> > > > ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
> > > > 
> > > > But the uses of these states are not clear/correct in some code.
> > > > For example, some code refer ACPI_STATE_D3 as D3hot and others refer
> > > > it as D3cold.
> > > > 
> > > > This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
> > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
> > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
> > > > 
> > > 
> > > With this patch now, if a device has _PS3, then we will set its D3hot
> > > flag valid. This doesn't feel right to me, since per our discussion the
> > > other day, we should assume _PS3 will put the device into D3cold.
> > > 
> > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is
> > > valid from the perspective of acpi, it is the individual driver's
> > > responsibility to decide which state is actually valid and will be used.
> > 
> > Right.
> > 
> > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
> > 
> 
> I mean, if _PS3 is available, can we say D3hot is valid?

Yes.

Lin Ming

Re: [PATCH v2] ACPI: D3 states cleanup

[Aaron Lu]

Hi,

On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote:
> > > > > There are two ACPI D3 states defined now:
> > > > > ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
> > > > > 
> > > > > But the uses of these states are not clear/correct in some code.
> > > > > For example, some code refer ACPI_STATE_D3 as D3hot and others refer
> > > > > it as D3cold.
> > > > > 
> > > > > This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
> > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
> > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
> > > > > 
> > > > 
> > > > With this patch now, if a device has _PS3, then we will set its D3hot
> > > > flag valid. This doesn't feel right to me, since per our discussion the
> > > > other day, we should assume _PS3 will put the device into D3cold.
> > > > 
> > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is
> > > > valid from the perspective of acpi, it is the individual driver's
> > > > responsibility to decide which state is actually valid and will be used.
> > > 
> > > Right.
> > > 
> > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
> > > 
> > 
> > I mean, if _PS3 is available, can we say D3hot is valid?
> 
> Yes.
> 

OK, now I'm confused...

First, let me try to clarify the meaning of acpi power state's valid
flag.

By valid, I suppose it means the device can be in that state, instead of
we have a way to program this device to go into that state.

e.g. D0 is valid means the device can be in D0 state, and D3_cold is
valid means the device can be in D3_cold state. We unconditionally set
these two states as valid, because we know every device supports these
two states. But we might not be able to put the device into that state
in software, since we might not have _PS0 or _PS3 control methods for it.

And if we do have the _PSx or _PRx control methods, we knows we have a
way to put the device into that state, and hence the device should be
able to support that power state, so we will set that state as valid too.

Is this correct?

For D3hot, obviously not all device supports this state, so we will need
to figure it out through the acpi table.
I remembered Rafael said the following words the other day in a reply to
my evaluate_ps3_when_entering_d3_cold_patch:

-----------------------------------------------------------------------
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 I think here is a problem, that if a device has only _PS3, why should
we say D3hot is supported? Is there a reason for this that I missed?

Explanation is appreciated, thanks.

-Aaron

Re: [PATCH v2] ACPI: D3 states cleanup

[Aaron Lu]

Hi,

On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote:
> > > > > There are two ACPI D3 states defined now:
> > > > > ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
> > > > > 
> > > > > But the uses of these states are not clear/correct in some code.
> > > > > For example, some code refer ACPI_STATE_D3 as D3hot and others refer
> > > > > it as D3cold.
> > > > > 
> > > > > This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
> > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
> > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
> > > > > 
> > > > 
> > > > With this patch now, if a device has _PS3, then we will set its D3hot
> > > > flag valid. This doesn't feel right to me, since per our discussion the
> > > > other day, we should assume _PS3 will put the device into D3cold.
> > > > 
> > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is
> > > > valid from the perspective of acpi, it is the individual driver's
> > > > responsibility to decide which state is actually valid and will be used.
> > > 
> > > Right.
> > > 
> > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
> > > 
> > 
> > I mean, if _PS3 is available, can we say D3hot is valid?
> 
> Yes.
> 

OK, now I'm confused...

First, let me try to clarify the meaning of acpi power state's valid
flag.

By valid, I suppose it means the device can be in that state, instead of
we have a way to program this device to go into that state.

e.g. D0 is valid means the device can be in D0 state, and D3_cold is
valid means the device can be in D3_cold state. We unconditionally set
these two states as valid, because we know every device supports these
two states. But we might not be able to put the device into that state
in software, since we might not have _PS0 or _PS3 control methods for it.

And if we do have the _PSx or _PRx control methods, we knows we have a
way to put the device into that state, and hence the device should be
able to support that power state, so we will set that state as valid too.

Is this correct?

For D3hot, obviously not all device supports this state, so we will need
to figure it out through the acpi table.
I remembered Rafael said the following words the other day in a reply to
my evaluate_ps3_when_entering_d3_cold_patch:

-----------------------------------------------------------------------
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 I think here is a problem, that if a device has only _PS3, why should
we say D3hot is supported? Is there a reason for this that I missed?

Explanation is appreciated, thanks.

-Aaron

Re: [PATCH v2] ACPI: D3 states cleanup

[Rafael J. Wysocki]

On Friday, April 20, 2012, Aaron Lu wrote:
> Hi,
> 
> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote:
> > > > > > There are two ACPI D3 states defined now:
> > > > > > ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
> > > > > > 
> > > > > > But the uses of these states are not clear/correct in some code.
> > > > > > For example, some code refer ACPI_STATE_D3 as D3hot and others refer
> > > > > > it as D3cold.
> > > > > > 
> > > > > > This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
> > > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
> > > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
> > > > > > 
> > > > > 
> > > > > With this patch now, if a device has _PS3, then we will set its D3hot
> > > > > flag valid. This doesn't feel right to me, since per our discussion the
> > > > > other day, we should assume _PS3 will put the device into D3cold.
> > > > > 
> > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is
> > > > > valid from the perspective of acpi, it is the individual driver's
> > > > > responsibility to decide which state is actually valid and will be used.
> > > > 
> > > > Right.
> > > > 
> > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
> > > > 
> > > 
> > > I mean, if _PS3 is available, can we say D3hot is valid?
> > 
> > Yes.
> > 
> 
> OK, now I'm confused...
> 
> First, let me try to clarify the meaning of acpi power state's valid
> flag.
> 
> By valid, I suppose it means the device can be in that state, instead of
> we have a way to program this device to go into that state.
> 
> e.g. D0 is valid means the device can be in D0 state, and D3_cold is
> valid means the device can be in D3_cold state. We unconditionally set
> these two states as valid, because we know every device supports these
> two states. But we might not be able to put the device into that state
> in software, since we might not have _PS0 or _PS3 control methods for it.
> 
> And if we do have the _PSx or _PRx control methods, we knows we have a
> way to put the device into that state, and hence the device should be
> able to support that power state, so we will set that state as valid too.
> 
> Is this correct?
> 
> For D3hot, obviously not all device supports this state, so we will need
> to figure it out through the acpi table.
> I remembered Rafael said the following words the other day in a reply to
> my evaluate_ps3_when_entering_d3_cold_patch:
> 
> -----------------------------------------------------------------------
> 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 I think here is a problem, that if a device has only _PS3, why should
> we say D3hot is supported? Is there a reason for this that I missed?

OK, I agree.  We need to special case the situation in which _PR3 is not
present, but _PS3 is.  IOW, we should do something like this in the loop in
acpi_bus_get_power_flags():


		/* State is valid if we have some power control */
		if (ps->resources.count
		    || (ps->flags.explicit_set && i < ACPI_STATE_D3))
			ps->flags.valid = 1;

Thanks,
Rafael

Re: [PATCH v2] ACPI: D3 states cleanup

[Lin Ming]

On Fri, Apr 20, 2012 at 7:12 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, April 20, 2012, Aaron Lu wrote:
>> Hi,
>>
>> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote:
>> > > > > > There are two ACPI D3 states defined now:
>> > > > > > ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
>> > > > > >
>> > > > > > But the uses of these states are not clear/correct in some code.
>> > > > > > For example, some code refer ACPI_STATE_D3 as D3hot and others refer
>> > > > > > it as D3cold.
>> > > > > >
>> > > > > > This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
>> > > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
>> > > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
>> > > > > >
>> > > > >
>> > > > > With this patch now, if a device has _PS3, then we will set its D3hot
>> > > > > flag valid. This doesn't feel right to me, since per our discussion the
>> > > > > other day, we should assume _PS3 will put the device into D3cold.
>> > > > >
>> > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is
>> > > > > valid from the perspective of acpi, it is the individual driver's
>> > > > > responsibility to decide which state is actually valid and will be used.
>> > > >
>> > > > Right.
>> > > >
>> > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
>> > > >
>> > >
>> > > I mean, if _PS3 is available, can we say D3hot is valid?
>> >
>> > Yes.
>> >
>>
>> OK, now I'm confused...
>>
>> First, let me try to clarify the meaning of acpi power state's valid
>> flag.
>>
>> By valid, I suppose it means the device can be in that state, instead of
>> we have a way to program this device to go into that state.
>>
>> e.g. D0 is valid means the device can be in D0 state, and D3_cold is
>> valid means the device can be in D3_cold state. We unconditionally set
>> these two states as valid, because we know every device supports these
>> two states. But we might not be able to put the device into that state
>> in software, since we might not have _PS0 or _PS3 control methods for it.
>>
>> And if we do have the _PSx or _PRx control methods, we knows we have a
>> way to put the device into that state, and hence the device should be
>> able to support that power state, so we will set that state as valid too.
>>
>> Is this correct?
>>
>> For D3hot, obviously not all device supports this state, so we will need
>> to figure it out through the acpi table.
>> I remembered Rafael said the following words the other day in a reply to
>> my evaluate_ps3_when_entering_d3_cold_patch:
>>
>> -----------------------------------------------------------------------
>> 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 I think here is a problem, that if a device has only _PS3, why should
>> we say D3hot is supported? Is there a reason for this that I missed?
>
> OK, I agree.  We need to special case the situation in which _PR3 is not
> present, but _PS3 is.  IOW, we should do something like this in the loop in
> acpi_bus_get_power_flags():
>
>
>                /* State is valid if we have some power control */
>                if (ps->resources.count
>                    || (ps->flags.explicit_set && i < ACPI_STATE_D3))
>                        ps->flags.valid = 1;

Will add below change.

-               /* State is valid if we have some power control */
-               if (ps->resources.count || ps->flags.explicit_set)
+               /*
+                * State is valid if we have some power control
+                * D3hot state is only valid if _PR3 present
+                */
+               if (ps->resources.count ||
+                   (ps->flags.explicit_set && i < ACPI_STATE_D3_HOT))
                        ps->flags.valid = 1;

Rafael,

can I still add your ACK when send updated patch?

Thanks,
Lin Ming

>
> Thanks,
> Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] ACPI: D3 states cleanup

[Lin Ming]

On Fri, Apr 20, 2012 at 7:12 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, April 20, 2012, Aaron Lu wrote:
>> Hi,
>>
>> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote:
>> > > > > > There are two ACPI D3 states defined now:
>> > > > > > ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
>> > > > > >
>> > > > > > But the uses of these states are not clear/correct in some code.
>> > > > > > For example, some code refer ACPI_STATE_D3 as D3hot and others refer
>> > > > > > it as D3cold.
>> > > > > >
>> > > > > > This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
>> > > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
>> > > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
>> > > > > >
>> > > > >
>> > > > > With this patch now, if a device has _PS3, then we will set its D3hot
>> > > > > flag valid. This doesn't feel right to me, since per our discussion the
>> > > > > other day, we should assume _PS3 will put the device into D3cold.
>> > > > >
>> > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is
>> > > > > valid from the perspective of acpi, it is the individual driver's
>> > > > > responsibility to decide which state is actually valid and will be used.
>> > > >
>> > > > Right.
>> > > >
>> > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
>> > > >
>> > >
>> > > I mean, if _PS3 is available, can we say D3hot is valid?
>> >
>> > Yes.
>> >
>>
>> OK, now I'm confused...
>>
>> First, let me try to clarify the meaning of acpi power state's valid
>> flag.
>>
>> By valid, I suppose it means the device can be in that state, instead of
>> we have a way to program this device to go into that state.
>>
>> e.g. D0 is valid means the device can be in D0 state, and D3_cold is
>> valid means the device can be in D3_cold state. We unconditionally set
>> these two states as valid, because we know every device supports these
>> two states. But we might not be able to put the device into that state
>> in software, since we might not have _PS0 or _PS3 control methods for it.
>>
>> And if we do have the _PSx or _PRx control methods, we knows we have a
>> way to put the device into that state, and hence the device should be
>> able to support that power state, so we will set that state as valid too.
>>
>> Is this correct?
>>
>> For D3hot, obviously not all device supports this state, so we will need
>> to figure it out through the acpi table.
>> I remembered Rafael said the following words the other day in a reply to
>> my evaluate_ps3_when_entering_d3_cold_patch:
>>
>> -----------------------------------------------------------------------
>> 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 I think here is a problem, that if a device has only _PS3, why should
>> we say D3hot is supported? Is there a reason for this that I missed?
>
> OK, I agree.  We need to special case the situation in which _PR3 is not
> present, but _PS3 is.  IOW, we should do something like this in the loop in
> acpi_bus_get_power_flags():
>
>
>                /* State is valid if we have some power control */
>                if (ps->resources.count
>                    || (ps->flags.explicit_set && i < ACPI_STATE_D3))
>                        ps->flags.valid = 1;

Will add below change.

-               /* State is valid if we have some power control */
-               if (ps->resources.count || ps->flags.explicit_set)
+               /*
+                * State is valid if we have some power control
+                * D3hot state is only valid if _PR3 present
+                */
+               if (ps->resources.count ||
+                   (ps->flags.explicit_set && i < ACPI_STATE_D3_HOT))
                        ps->flags.valid = 1;

Rafael,

can I still add your ACK when send updated patch?

Thanks,
Lin Ming

>
> Thanks,
> Rafael

Re: [PATCH v2] ACPI: D3 states cleanup

[Lu, Aaron]

在 2012-4-20，下午7:08，"Rafael J. Wysocki" <rjw@sisk.pl> 写道：

> On Friday, April 20, 2012, Aaron Lu wrote:
>> Hi,
>> 
>> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote:
>>>>>>> There are two ACPI D3 states defined now:
>>>>>>> ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
>>>>>>> 
>>>>>>> But the uses of these states are not clear/correct in some code.
>>>>>>> For example, some code refer ACPI_STATE_D3 as D3hot and others refer
>>>>>>> it as D3cold.
>>>>>>> 
>>>>>>> This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
>>>>>>> And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
>>>>>>> Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
>>>>>>> 
>>>>>> 
>>>>>> With this patch now, if a device has _PS3, then we will set its D3hot
>>>>>> flag valid. This doesn't feel right to me, since per our discussion the
>>>>>> other day, we should assume _PS3 will put the device into D3cold.
>>>>>> 
>>>>>> Or do you mean: if _PS3 is available, then both D3hot and D3cold is
>>>>>> valid from the perspective of acpi, it is the individual driver's
>>>>>> responsibility to decide which state is actually valid and will be used.
>>>>> 
>>>>> Right.
>>>>> 
>>>>> ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
>>>>> 
>>>> 
>>>> I mean, if _PS3 is available, can we say D3hot is valid?
>>> 
>>> Yes.
>>> 
>> 
>> OK, now I'm confused...
>> 
>> First, let me try to clarify the meaning of acpi power state's valid
>> flag.
>> 
>> By valid, I suppose it means the device can be in that state, instead of
>> we have a way to program this device to go into that state.
>> 
>> e.g. D0 is valid means the device can be in D0 state, and D3_cold is
>> valid means the device can be in D3_cold state. We unconditionally set
>> these two states as valid, because we know every device supports these
>> two states. But we might not be able to put the device into that state
>> in software, since we might not have _PS0 or _PS3 control methods for it.
>> 
>> And if we do have the _PSx or _PRx control methods, we knows we have a
>> way to put the device into that state, and hence the device should be
>> able to support that power state, so we will set that state as valid too.
>> 
>> Is this correct?
>> 
>> For D3hot, obviously not all device supports this state, so we will need
>> to figure it out through the acpi table.
>> I remembered Rafael said the following words the other day in a reply to
>> my evaluate_ps3_when_entering_d3_cold_patch:
>> 
>> -----------------------------------------------------------------------
>> 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 I think here is a problem, that if a device has only _PS3, why should
>> we say D3hot is supported? Is there a reason for this that I missed?
> 
> OK, I agree.  We need to special case the situation in which _PR3 is not
> present, but _PS3 is.  IOW, we should do something like this in the loop in
> acpi_bus_get_power_flags():
> 
> 
>        /* State is valid if we have some power control */
>        if (ps->resources.count
>            || (ps->flags.explicit_set && i < ACPI_STATE_D3))
>            ps->flags.valid = 1;
> 

Looks good to me, thanks.

-Aaron

Re: [PATCH v2] ACPI: D3 states cleanup

[Lu, Aaron]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 3575 bytes --]


ÔÚ 2012-4-20£¬ÏÂÎç7:08£¬"Rafael J. Wysocki" <rjw@sisk.pl> дµÀ£º

> On Friday, April 20, 2012, Aaron Lu wrote:
>> Hi,
>> 
>> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote:
>>>>>>> There are two ACPI D3 states defined now:
>>>>>>> ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
>>>>>>> 
>>>>>>> But the uses of these states are not clear/correct in some code.
>>>>>>> For example, some code refer ACPI_STATE_D3 as D3hot and others refer
>>>>>>> it as D3cold.
>>>>>>> 
>>>>>>> This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
>>>>>>> And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
>>>>>>> Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
>>>>>>> 
>>>>>> 
>>>>>> With this patch now, if a device has _PS3, then we will set its D3hot
>>>>>> flag valid. This doesn't feel right to me, since per our discussion the
>>>>>> other day, we should assume _PS3 will put the device into D3cold.
>>>>>> 
>>>>>> Or do you mean: if _PS3 is available, then both D3hot and D3cold is
>>>>>> valid from the perspective of acpi, it is the individual driver's
>>>>>> responsibility to decide which state is actually valid and will be used.
>>>>> 
>>>>> Right.
>>>>> 
>>>>> ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
>>>>> 
>>>> 
>>>> I mean, if _PS3 is available, can we say D3hot is valid?
>>> 
>>> Yes.
>>> 
>> 
>> OK, now I'm confused...
>> 
>> First, let me try to clarify the meaning of acpi power state's valid
>> flag.
>> 
>> By valid, I suppose it means the device can be in that state, instead of
>> we have a way to program this device to go into that state.
>> 
>> e.g. D0 is valid means the device can be in D0 state, and D3_cold is
>> valid means the device can be in D3_cold state. We unconditionally set
>> these two states as valid, because we know every device supports these
>> two states. But we might not be able to put the device into that state
>> in software, since we might not have _PS0 or _PS3 control methods for it.
>> 
>> And if we do have the _PSx or _PRx control methods, we knows we have a
>> way to put the device into that state, and hence the device should be
>> able to support that power state, so we will set that state as valid too.
>> 
>> Is this correct?
>> 
>> For D3hot, obviously not all device supports this state, so we will need
>> to figure it out through the acpi table.
>> I remembered Rafael said the following words the other day in a reply to
>> my evaluate_ps3_when_entering_d3_cold_patch:
>> 
>> -----------------------------------------------------------------------
>> 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 I think here is a problem, that if a device has only _PS3, why should
>> we say D3hot is supported? Is there a reason for this that I missed?
> 
> OK, I agree.  We need to special case the situation in which _PR3 is not
> present, but _PS3 is.  IOW, we should do something like this in the loop in
> acpi_bus_get_power_flags():
> 
> 
>        /* State is valid if we have some power control */
>        if (ps->resources.count
>            || (ps->flags.explicit_set && i < ACPI_STATE_D3))
>            ps->flags.valid = 1;
> 

Looks good to me, thanks.

-Aaron

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] ACPI: D3 states cleanup

[Rafael J. Wysocki]

On Friday, April 20, 2012, Lin Ming wrote:
> On Fri, Apr 20, 2012 at 7:12 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday, April 20, 2012, Aaron Lu wrote:
> >> Hi,
> >>
> >> On Fri, Apr 20, 2012 at 01:37:35PM +0800, Lin Ming wrote:
> >> > > > > > There are two ACPI D3 states defined now:
> >> > > > > > ACPI_STATE_D3 and ACPI_STATE_D3_COLD.
> >> > > > > >
> >> > > > > > But the uses of these states are not clear/correct in some code.
> >> > > > > > For example, some code refer ACPI_STATE_D3 as D3hot and others refer
> >> > > > > > it as D3cold.
> >> > > > > >
> >> > > > > > This patch introduces ACPI_STATE_D3_HOT to refer to ACPI D3hot state.
> >> > > > > > And changes ACPI_STATE_D3 to refer to ACPI D3cold state only.
> >> > > > > > Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.
> >> > > > > >
> >> > > > >
> >> > > > > With this patch now, if a device has _PS3, then we will set its D3hot
> >> > > > > flag valid. This doesn't feel right to me, since per our discussion the
> >> > > > > other day, we should assume _PS3 will put the device into D3cold.
> >> > > > >
> >> > > > > Or do you mean: if _PS3 is available, then both D3hot and D3cold is
> >> > > > > valid from the perspective of acpi, it is the individual driver's
> >> > > > > responsibility to decide which state is actually valid and will be used.
> >> > > >
> >> > > > Right.
> >> > > >
> >> > > > ACPI_STATE_D3(same as ACPI_STATE_D3_COLD now) is always valid.
> >> > > >
> >> > >
> >> > > I mean, if _PS3 is available, can we say D3hot is valid?
> >> >
> >> > Yes.
> >> >
> >>
> >> OK, now I'm confused...
> >>
> >> First, let me try to clarify the meaning of acpi power state's valid
> >> flag.
> >>
> >> By valid, I suppose it means the device can be in that state, instead of
> >> we have a way to program this device to go into that state.
> >>
> >> e.g. D0 is valid means the device can be in D0 state, and D3_cold is
> >> valid means the device can be in D3_cold state. We unconditionally set
> >> these two states as valid, because we know every device supports these
> >> two states. But we might not be able to put the device into that state
> >> in software, since we might not have _PS0 or _PS3 control methods for it.
> >>
> >> And if we do have the _PSx or _PRx control methods, we knows we have a
> >> way to put the device into that state, and hence the device should be
> >> able to support that power state, so we will set that state as valid too.
> >>
> >> Is this correct?
> >>
> >> For D3hot, obviously not all device supports this state, so we will need
> >> to figure it out through the acpi table.
> >> I remembered Rafael said the following words the other day in a reply to
> >> my evaluate_ps3_when_entering_d3_cold_patch:
> >>
> >> -----------------------------------------------------------------------
> >> 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 I think here is a problem, that if a device has only _PS3, why should
> >> we say D3hot is supported? Is there a reason for this that I missed?
> >
> > OK, I agree.  We need to special case the situation in which _PR3 is not
> > present, but _PS3 is.  IOW, we should do something like this in the loop in
> > acpi_bus_get_power_flags():
> >
> >
> >                /* State is valid if we have some power control */
> >                if (ps->resources.count
> >                    || (ps->flags.explicit_set && i < ACPI_STATE_D3))
> >                        ps->flags.valid = 1;
> 
> Will add below change.
>
> -               /* State is valid if we have some power control */
> -               if (ps->resources.count || ps->flags.explicit_set)
> +               /*
> +                * State is valid if we have some power control
> +                * D3hot state is only valid if _PR3 present
> +                */

Well, I'd say:

 +               /*
 +                * State is valid there are means to put the device into it.
 +                * D3hot is only valid if _PR3 present.
 +                */

> +               if (ps->resources.count ||
> +                   (ps->flags.explicit_set && i < ACPI_STATE_D3_HOT))
>                         ps->flags.valid = 1;
> 
> Rafael,
> 
> can I still add your ACK when send updated patch?

Sure.

Thanks,
Rafael