[PATCH 0/6]: ACPI D3 state cleanup

[PATCH 0/6]: ACPI D3 state cleanup

[Lin Ming]

Hi,

Currently there are 2 ACPI D3 states defined:
ACPI_STATE_D3 and ACPI_STATE_D3_COLD.

But the uses of these 2 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 set try to clean it up.

[PATCH 1/6] ACPI: D3cold state is always valid
[PATCH 2/6] ACPI: Set D3cold state as default device sleep state
[PATCH 3/6] ACPI: Set D3cold state as default inferred state
[PATCH 4/6] ACPI: Fix power resource's device power state when it's off
[PATCH 5/6] PCI/ACPI: Map PCI D3cold state to ACPI D3cold state
[PATCH 6/6] ACPI: Rename ACPI_STATE_D3 to ACPI_STATE_D3_HOT

 drivers/acpi/fan.c         |    4 ++--
 drivers/acpi/power.c       |   10 +++++-----
 drivers/acpi/scan.c        |   13 +++----------
 drivers/acpi/sleep.c       |    2 +-
 drivers/ata/libata-acpi.c  |    4 ++--
 drivers/ide/ide-acpi.c     |    4 ++--
 drivers/pci/pci-acpi.c     |    6 +++---
 drivers/pnp/pnpacpi/core.c |    4 ++--
 include/acpi/acpi_bus.h    |    2 +-
 include/acpi/actypes.h     |    2 +-
 10 files changed, 22 insertions(+), 29 deletions(-)

Thanks,
Lin Ming

[PATCH 1/6] ACPI: D3cold state is always valid

[Lin Ming]

ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
Instead, ACPI D3cold is always valid.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/acpi/scan.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 767e2dc..fb56388 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -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);
@@ -908,8 +901,8 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
 	/* Set defaults for D0 and D3 states (always valid) */
 	device->power.states[ACPI_STATE_D0].flags.valid = 1;
 	device->power.states[ACPI_STATE_D0].power = 100;
-	device->power.states[ACPI_STATE_D3].flags.valid = 1;
-	device->power.states[ACPI_STATE_D3].power = 0;
+	device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
+	device->power.states[ACPI_STATE_D3_COLD].power = 0;
 
 	acpi_bus_init_power(device);
 
-- 
1.7.2.5

[PATCH 2/6] ACPI: Set D3cold state as default device sleep state

[Lin Ming]

D3cold state is returned if no other preferred power state is found.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/acpi/sleep.c    |    2 +-
 include/acpi/acpi_bus.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 1d661b5..1fcfb78 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -705,7 +705,7 @@ int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p)
 	 * _S0W, we will use the value from _S0W
 	 */
 	d_min = ACPI_STATE_D0;
-	d_max = ACPI_STATE_D3;
+	d_max = ACPI_STATE_D3_COLD;
 
 	/*
 	 * If present, _SxD methods return the minimum D-state (highest power
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index f1c8ca6..2db51aa 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -389,7 +389,7 @@ static inline int acpi_pm_device_sleep_state(struct device *d, int *p)
 {
 	if (p)
 		*p = ACPI_STATE_D0;
-	return ACPI_STATE_D3;
+	return ACPI_STATE_D3_COLD;
 }
 #endif
 
-- 
1.7.2.5

[PATCH 3/6] ACPI: Set D3cold state as default inferred state

[Lin Ming]

ACPI_STATE_D3 means ACPI D3hot state which should only be returned when
power resources listed in _PR3 are 'on'.

Return ACPI D3cold state if no other state is known.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/acpi/power.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index 7049a7d..27a2bad 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -646,7 +646,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)
 		}
 	}
 
-	*state = ACPI_STATE_D3;
+	*state = ACPI_STATE_D3_COLD;
 	return 0;
 }
 
-- 
1.7.2.5

[PATCH 4/6] ACPI: Fix power resource's device power state when it's off

[Lin Ming]

Power resource's device power state should be
ACPI_STATE_D3_COLD when it's off.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/acpi/power.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index 27a2bad..d6065110 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -300,7 +300,7 @@ static int acpi_power_off(acpi_handle handle)
 		result = -ENODEV;
 	} else {
 		/* Update the power resource's _device_ power state */
-		resource->device->power.state = ACPI_STATE_D3;
+		resource->device->power.state = ACPI_STATE_D3_COLD;
 
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Power resource [%s] turned off\n",
@@ -735,7 +735,7 @@ static int acpi_power_add(struct acpi_device *device)
 		device->power.state = ACPI_STATE_D0;
 		break;
 	case ACPI_POWER_RESOURCE_STATE_OFF:
-		device->power.state = ACPI_STATE_D3;
+		device->power.state = ACPI_STATE_D3_COLD;
 		break;
 	default:
 		device->power.state = ACPI_STATE_UNKNOWN;
-- 
1.7.2.5

[PATCH 5/6] PCI/ACPI: Map PCI D3cold state to ACPI D3cold state

[Lin Ming]

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/pci/pci-acpi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 0f150f2..e641953 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -224,7 +224,7 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		[PCI_D1] = ACPI_STATE_D1,
 		[PCI_D2] = ACPI_STATE_D2,
 		[PCI_D3hot] = ACPI_STATE_D3,
-		[PCI_D3cold] = ACPI_STATE_D3
+		[PCI_D3cold] = ACPI_STATE_D3_COLD
 	};
 	int error = -EINVAL;
 
-- 
1.7.2.5

[PATCH 6/6] ACPI: Rename ACPI_STATE_D3 to ACPI_STATE_D3_HOT

[Lin Ming]

So now there are 2 clear ACPI D3 states:
ACPI_STATE_D3_HOT and ACPI_STATE_D3_COLD.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/acpi/fan.c         |    4 ++--
 drivers/acpi/power.c       |    4 ++--
 drivers/acpi/scan.c        |    2 +-
 drivers/ata/libata-acpi.c  |    4 ++--
 drivers/ide/ide-acpi.c     |    4 ++--
 drivers/pci/pci-acpi.c     |    4 ++--
 drivers/pnp/pnpacpi/core.c |    4 ++--
 include/acpi/actypes.h     |    2 +-
 8 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index 0f0356c..6f57272 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -90,7 +90,7 @@ static int fan_get_cur_state(struct thermal_cooling_device *cdev, unsigned long
 	if (result)
 		return result;
 
-	*state = (acpi_state == ACPI_STATE_D3 ? 0 :
+	*state = (acpi_state == ACPI_STATE_D3_HOT ? 0 :
 		 (acpi_state == ACPI_STATE_D0 ? 1 : -1));
 	return 0;
 }
@@ -105,7 +105,7 @@ fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
 		return -EINVAL;
 
 	result = acpi_bus_set_power(device->handle,
-				state ? ACPI_STATE_D0 : ACPI_STATE_D3);
+				state ? ACPI_STATE_D0 : ACPI_STATE_D3_HOT);
 
 	return result;
 }
diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index d6065110..e84759f 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;
@@ -652,7 +652,7 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state)
 
 int acpi_power_on_resources(struct acpi_device *device, int state)
 {
-	if (!device || state < ACPI_STATE_D0 || state > ACPI_STATE_D3)
+	if (!device || state < ACPI_STATE_D0 || state > ACPI_STATE_D3_HOT)
 		return -EINVAL;
 
 	return acpi_power_on_list(&device->power.states[state].resources);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index fb56388..1fbb2a2 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' };
 
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index bb7c5f1..27e0e56 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -920,10 +920,10 @@ void ata_acpi_set_state(struct ata_port *ap, pm_message_t state)
 		if (dev->acpi_handle)
 			acpi_bus_set_power(dev->acpi_handle,
 				state.event == PM_EVENT_ON ?
-					ACPI_STATE_D0 : ACPI_STATE_D3);
+					ACPI_STATE_D0 : ACPI_STATE_D3_HOT);
 	}
 	if (state.event != PM_EVENT_ON)
-		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D3);
+		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D3_HOT);
 }
 
 /**
diff --git a/drivers/ide/ide-acpi.c b/drivers/ide/ide-acpi.c
index f1a6796b..7524831 100644
--- a/drivers/ide/ide-acpi.c
+++ b/drivers/ide/ide-acpi.c
@@ -520,11 +520,11 @@ void ide_acpi_set_state(ide_hwif_t *hwif, int on)
 	ide_port_for_each_present_dev(i, drive, hwif) {
 		if (drive->acpidata->obj_handle)
 			acpi_bus_set_power(drive->acpidata->obj_handle,
-					   on ? ACPI_STATE_D0 : ACPI_STATE_D3);
+					   on ? ACPI_STATE_D0 : ACPI_STATE_D3_HOT);
 	}
 
 	if (!on)
-		acpi_bus_set_power(hwif->acpidata->obj_handle, ACPI_STATE_D3);
+		acpi_bus_set_power(hwif->acpidata->obj_handle, ACPI_STATE_D3_HOT);
 }
 
 /**
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index e641953..77aa4e1 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_COLD
 	};
 	int error = -EINVAL;
diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index d21e8f5..9448ab9 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -128,7 +128,7 @@ static int pnpacpi_disable_resources(struct pnp_dev *dev)
 	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
 	ret = 0;
 	if (acpi_bus_power_manageable(handle))
-		acpi_bus_set_power(handle, ACPI_STATE_D3);
+		acpi_bus_set_power(handle, ACPI_STATE_D3_HOT);
 		/* continue even if acpi_bus_set_power() fails */
 	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DIS", NULL, NULL)))
 		ret = -ENODEV;
@@ -174,7 +174,7 @@ static int pnpacpi_suspend(struct pnp_dev *dev, pm_message_t state)
 
 		if (power_state < 0)
 			power_state = (state.event == PM_EVENT_ON) ?
-					ACPI_STATE_D0 : ACPI_STATE_D3;
+					ACPI_STATE_D0 : ACPI_STATE_D3_HOT;
 
 		/*
 		 * acpi_bus_set_power() often fails (keyboard port can't be
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index eba6604..e3b7cac 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -499,7 +499,7 @@ 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_HOT               (u8) 3
 #define ACPI_STATE_D3_COLD              (u8) 4
 #define ACPI_D_STATES_MAX               ACPI_STATE_D3_COLD
 #define ACPI_D_STATE_COUNT              5
-- 
1.7.2.5

Re: [PATCH 1/6] ACPI: D3cold state is always valid

[huang ying]

On Tue, Apr 17, 2012 at 1:47 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
> Instead, ACPI D3cold is always valid.
>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  drivers/acpi/scan.c |   11 ++---------
>  1 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 767e2dc..fb56388 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -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);
> @@ -908,8 +901,8 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>        /* Set defaults for D0 and D3 states (always valid) */
>        device->power.states[ACPI_STATE_D0].flags.valid = 1;
>        device->power.states[ACPI_STATE_D0].power = 100;
> -       device->power.states[ACPI_STATE_D3].flags.valid = 1;
> -       device->power.states[ACPI_STATE_D3].power = 0;
> +       device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> +       device->power.states[ACPI_STATE_D3_COLD].power = 0;
>
>        acpi_bus_init_power(device);

I think D3_HOT should be always valid, while D3_COLD should be valid
for some situation.

 - has _PS3, no _PR3
   - support D3_HOT, D3_COLD
   - set state
     - D3_HOT: do nothing in ACPI
     - D3_COLD: _PS3

 - has _PR3, no _PS3
   - support D3_HOT, D3_COLD
   - set state
     - D3_HOT: _PR3 on, _PR0 off
     - D3_COLD: _PR3 on, _PR0 off, _PR3 off

 - has _PS3, has _PR3
   - support D3_HOT, D3_COLD
   - set state
     - D3_HOT: _PS3, _PR3 on, _PR0 off
     - D3_COLD: _PS3, _PR3 on, _PR0 off, _PR3 off

 - no _PS3, no _PR3
   - support D3_HOT
   - set state
     - D3_HOT: do nothing in ACPI

Best Regards,
Huang Ying

Re: [PATCH 1/6] ACPI: D3cold state is always valid

[Rafael J. Wysocki]

On Tuesday, April 17, 2012, Lin Ming wrote:
> ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
> Instead, ACPI D3cold is always valid.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>

Can you just introduce ACPI_STATE_D3_HOT and redefine D3_COLD so that
it's the same as D3, as I said?

Rafael


> ---
>  drivers/acpi/scan.c |   11 ++---------
>  1 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 767e2dc..fb56388 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -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);
> @@ -908,8 +901,8 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>  	/* Set defaults for D0 and D3 states (always valid) */
>  	device->power.states[ACPI_STATE_D0].flags.valid = 1;
>  	device->power.states[ACPI_STATE_D0].power = 100;
> -	device->power.states[ACPI_STATE_D3].flags.valid = 1;
> -	device->power.states[ACPI_STATE_D3].power = 0;
> +	device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> +	device->power.states[ACPI_STATE_D3_COLD].power = 0;
>  
>  	acpi_bus_init_power(device);
>  
> 

Re: [PATCH 1/6] ACPI: D3cold state is always valid

[Lin Ming]

(fix Len's address)

On Tue, 2012-04-17 at 22:29 +0200, Rafael J. Wysocki wrote:
> On Tuesday, April 17, 2012, Lin Ming wrote:
> > ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
> > Instead, ACPI D3cold is always valid.
> > 
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> 
> Can you just introduce ACPI_STATE_D3_HOT and redefine D3_COLD so that
> it's the same as D3, as I said?

Do you mean below?

diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index eba6604..f0b25c8 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -499,9 +499,9 @@ 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_D_STATES_MAX               ACPI_STATE_D3
 #define ACPI_D_STATE_COUNT              5
 
 #define ACPI_STATE_C0                   (u8) 0

Re: [PATCH 1/6] ACPI: D3cold state is always valid

[Lin Ming]

On Wed, 2012-04-18 at 10:15 +0800, Lin Ming wrote:
> (fix Len's address)
> 
> On Tue, 2012-04-17 at 22:29 +0200, Rafael J. Wysocki wrote:
> > On Tuesday, April 17, 2012, Lin Ming wrote:
> > > ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
> > > Instead, ACPI D3cold is always valid.
> > > 
> > > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > 
> > Can you just introduce ACPI_STATE_D3_HOT and redefine D3_COLD so that
> > it's the same as D3, as I said?
> 
> Do you mean below?

Or you mean below?

diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index eba6604..5852b8a 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -499,8 +499,9 @@ 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_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_COLD
 #define ACPI_D_STATE_COUNT              5
 


> 
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index eba6604..f0b25c8 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -499,9 +499,9 @@ 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_D_STATES_MAX               ACPI_STATE_D3
>  #define ACPI_D_STATE_COUNT              5
>  
>  #define ACPI_STATE_C0                   (u8) 0
> 

Re: [PATCH 1/6] ACPI: D3cold state is always valid

[Rafael J. Wysocki]

On Wednesday, April 18, 2012, Lin Ming wrote:
> On Wed, 2012-04-18 at 10:15 +0800, Lin Ming wrote:
> > (fix Len's address)
> > 
> > On Tue, 2012-04-17 at 22:29 +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, April 17, 2012, Lin Ming wrote:
> > > > ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
> > > > Instead, ACPI D3cold is always valid.
> > > > 
> > > > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > > 
> > > Can you just introduce ACPI_STATE_D3_HOT and redefine D3_COLD so that
> > > it's the same as D3, as I said?
> > 
> > Do you mean below?
> 
> Or you mean below?

This one, with a minor nit.

> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index eba6604..5852b8a 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -499,8 +499,9 @@ 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_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_COLD

I'd say

  #define ACPI_D_STATES_MAX               ACPI_STATE_D3

>  #define ACPI_D_STATE_COUNT              5

Thanks,
Rafael

Re: [PATCH 1/6] ACPI: D3cold state is always valid

[Rafael J. Wysocki]

On Tuesday, April 17, 2012, huang ying wrote:
> On Tue, Apr 17, 2012 at 1:47 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> > ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
> > Instead, ACPI D3cold is always valid.
> >
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > ---
> >  drivers/acpi/scan.c |   11 ++---------
> >  1 files changed, 2 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 767e2dc..fb56388 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -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);
> > @@ -908,8 +901,8 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> >        /* Set defaults for D0 and D3 states (always valid) */
> >        device->power.states[ACPI_STATE_D0].flags.valid = 1;
> >        device->power.states[ACPI_STATE_D0].power = 100;
> > -       device->power.states[ACPI_STATE_D3].flags.valid = 1;
> > -       device->power.states[ACPI_STATE_D3].power = 0;
> > +       device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> > +       device->power.states[ACPI_STATE_D3_COLD].power = 0;
> >
> >        acpi_bus_init_power(device);
> 
> I think D3_HOT should be always valid, while D3_COLD should be valid
> for some situation.

This need not be PCI, mind you.

>  - has _PS3, no _PR3
>    - support D3_HOT, D3_COLD

Nope.  D3_HOT cannot be supported in that case at the ACPI level.

>    - set state
>      - D3_HOT: do nothing in ACPI

That is not D3_HOT, then, from the ACPI point of view.  It is a different
power state.

Suppose you have a non-PCI device that can be only power-manageable via ACPI
and that device has only _PS0 and _PS3.  How would you put it into D3_HOT,
in particular?

>      - D3_COLD: _PS3

Thanks,
Rafael

Re: [PATCH 1/6] ACPI: D3cold state is always valid

[huang ying]

On Thu, Apr 19, 2012 at 5:13 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday, April 17, 2012, huang ying wrote:
>> On Tue, Apr 17, 2012 at 1:47 PM, Lin Ming <ming.m.lin@intel.com> wrote:
>> > ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
>> > Instead, ACPI D3cold is always valid.
>> >
>> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
>> > ---
>> >  drivers/acpi/scan.c |   11 ++---------
>> >  1 files changed, 2 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> > index 767e2dc..fb56388 100644
>> > --- a/drivers/acpi/scan.c
>> > +++ b/drivers/acpi/scan.c
>> > @@ -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);
>> > @@ -908,8 +901,8 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
>> >        /* Set defaults for D0 and D3 states (always valid) */
>> >        device->power.states[ACPI_STATE_D0].flags.valid = 1;
>> >        device->power.states[ACPI_STATE_D0].power = 100;
>> > -       device->power.states[ACPI_STATE_D3].flags.valid = 1;
>> > -       device->power.states[ACPI_STATE_D3].power = 0;
>> > +       device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
>> > +       device->power.states[ACPI_STATE_D3_COLD].power = 0;
>> >
>> >        acpi_bus_init_power(device);
>>
>> I think D3_HOT should be always valid, while D3_COLD should be valid
>> for some situation.
>
> This need not be PCI, mind you.
>
>>  - has _PS3, no _PR3
>>    - support D3_HOT, D3_COLD
>
> Nope.  D3_HOT cannot be supported in that case at the ACPI level.
>
>>    - set state
>>      - D3_HOT: do nothing in ACPI
>
> That is not D3_HOT, then, from the ACPI point of view.  It is a different
> power state.
>
> Suppose you have a non-PCI device that can be only power-manageable via ACPI
> and that device has only _PS0 and _PS3.  How would you put it into D3_HOT,
> in particular?

Normally, we will put it into D3_COLD (via _PS3).

If it is prevented to be put in D3_COLD,
  - If D3_HOT is not marked as supported, we will keep it in D0
  - otherwise, we advocate we put it into D3_HOT, but in fact, it is in D0.

The result is same, but with wrong name.

But there will be some real problem if we have something like CPU
governor.  Because governor may choose D3_HOT for device.

But for PCI device, D3_HOT is supported for the device.

So the bus layer should combine the information from native power
state supported and ACPI power state supported to determine which
power states are supported?  For example, for a PCI device, ACPI
advocates D0 and D3_COLD are supported, and PCI layer may advocate D0,
D3_HOT and D3_COLD are supported.

Best Regards,
Huang Ying

Re: [PATCH 1/6] ACPI: D3cold state is always valid

[Rafael J. Wysocki]

On Thursday, April 19, 2012, huang ying wrote:
> On Thu, Apr 19, 2012 at 5:13 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Tuesday, April 17, 2012, huang ying wrote:
> >> On Tue, Apr 17, 2012 at 1:47 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> >> > ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
> >> > Instead, ACPI D3cold is always valid.
> >> >
> >> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> >> > ---
> >> >  drivers/acpi/scan.c |   11 ++---------
> >> >  1 files changed, 2 insertions(+), 9 deletions(-)
> >> >
> >> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >> > index 767e2dc..fb56388 100644
> >> > --- a/drivers/acpi/scan.c
> >> > +++ b/drivers/acpi/scan.c
> >> > @@ -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);
> >> > @@ -908,8 +901,8 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> >> >        /* Set defaults for D0 and D3 states (always valid) */
> >> >        device->power.states[ACPI_STATE_D0].flags.valid = 1;
> >> >        device->power.states[ACPI_STATE_D0].power = 100;
> >> > -       device->power.states[ACPI_STATE_D3].flags.valid = 1;
> >> > -       device->power.states[ACPI_STATE_D3].power = 0;
> >> > +       device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> >> > +       device->power.states[ACPI_STATE_D3_COLD].power = 0;
> >> >
> >> >        acpi_bus_init_power(device);
> >>
> >> I think D3_HOT should be always valid, while D3_COLD should be valid
> >> for some situation.
> >
> > This need not be PCI, mind you.
> >
> >>  - has _PS3, no _PR3
> >>    - support D3_HOT, D3_COLD
> >
> > Nope.  D3_HOT cannot be supported in that case at the ACPI level.
> >
> >>    - set state
> >>      - D3_HOT: do nothing in ACPI
> >
> > That is not D3_HOT, then, from the ACPI point of view.  It is a different
> > power state.
> >
> > Suppose you have a non-PCI device that can be only power-manageable via ACPI
> > and that device has only _PS0 and _PS3.  How would you put it into D3_HOT,
> > in particular?
> 
> Normally, we will put it into D3_COLD (via _PS3).
> 
> If it is prevented to be put in D3_COLD,
>   - If D3_HOT is not marked as supported, we will keep it in D0
>   - otherwise, we advocate we put it into D3_HOT, but in fact, it is in D0.
> 
> The result is same, but with wrong name.
> 
> But there will be some real problem if we have something like CPU
> governor.  Because governor may choose D3_HOT for device.

In that case the ACPI layer should simply return an error code indicating
that the requested state is not available _from_ _it_.  If there are
more layers, however, they may be able to change the power state of
the device.

> But for PCI device, D3_HOT is supported for the device.

Yes, if the device supports native PM.  However, PCI D-states and ACPI D-states
are different things.  We kind of combine them in our PCI bus type.

> So the bus layer should combine the information from native power
> state supported and ACPI power state supported to determine which
> power states are supported?

Yes, we do that all the time, with the exception of D3 cold/hot.

> For example, for a PCI device, ACPI advocates D0 and D3_COLD are supported,
> and PCI layer may advocate D0, D3_HOT and D3_COLD are supported.

Exactly.  All of the available power states of the device depend on both
the device's own capabilities (ie. what PCI D-states the device may be
programmed into by register writes) and whatever is available from ACPI.

On embedded systems there are other factors as well.

Thanks,
Rafael

Re: [PATCH 1/6] ACPI: D3cold state is always valid

[Lin Ming]

On Thu, 2012-04-19 at 13:51 +0200, Rafael J. Wysocki wrote:
> On Thursday, April 19, 2012, huang ying wrote:
> > On Thu, Apr 19, 2012 at 5:13 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Tuesday, April 17, 2012, huang ying wrote:
> > >> On Tue, Apr 17, 2012 at 1:47 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> > >> > ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
> > >> > Instead, ACPI D3cold is always valid.
> > >> >
> > >> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > >> > ---
> > >> >  drivers/acpi/scan.c |   11 ++---------
> > >> >  1 files changed, 2 insertions(+), 9 deletions(-)
> > >> >
> > >> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > >> > index 767e2dc..fb56388 100644
> > >> > --- a/drivers/acpi/scan.c
> > >> > +++ b/drivers/acpi/scan.c
> > >> > @@ -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);
> > >> > @@ -908,8 +901,8 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> > >> >        /* Set defaults for D0 and D3 states (always valid) */
> > >> >        device->power.states[ACPI_STATE_D0].flags.valid = 1;
> > >> >        device->power.states[ACPI_STATE_D0].power = 100;
> > >> > -       device->power.states[ACPI_STATE_D3].flags.valid = 1;
> > >> > -       device->power.states[ACPI_STATE_D3].power = 0;
> > >> > +       device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> > >> > +       device->power.states[ACPI_STATE_D3_COLD].power = 0;
> > >> >
> > >> >        acpi_bus_init_power(device);
> > >>
> > >> I think D3_HOT should be always valid, while D3_COLD should be valid
> > >> for some situation.
> > >
> > > This need not be PCI, mind you.
> > >
> > >>  - has _PS3, no _PR3
> > >>    - support D3_HOT, D3_COLD
> > >
> > > Nope.  D3_HOT cannot be supported in that case at the ACPI level.
> > >
> > >>    - set state
> > >>      - D3_HOT: do nothing in ACPI
> > >
> > > That is not D3_HOT, then, from the ACPI point of view.  It is a different
> > > power state.
> > >
> > > Suppose you have a non-PCI device that can be only power-manageable via ACPI
> > > and that device has only _PS0 and _PS3.  How would you put it into D3_HOT,
> > > in particular?
> > 
> > Normally, we will put it into D3_COLD (via _PS3).
> > 
> > If it is prevented to be put in D3_COLD,
> >   - If D3_HOT is not marked as supported, we will keep it in D0
> >   - otherwise, we advocate we put it into D3_HOT, but in fact, it is in D0.
> > 
> > The result is same, but with wrong name.
> > 
> > But there will be some real problem if we have something like CPU
> > governor.  Because governor may choose D3_HOT for device.
> 
> In that case the ACPI layer should simply return an error code indicating
> that the requested state is not available _from_ _it_.  If there are
> more layers, however, they may be able to change the power state of
> the device.
> 
> > But for PCI device, D3_HOT is supported for the device.
> 
> Yes, if the device supports native PM.  However, PCI D-states and ACPI D-states
> are different things.  We kind of combine them in our PCI bus type.
> 
> > So the bus layer should combine the information from native power
> > state supported and ACPI power state supported to determine which
> > power states are supported?
> 
> Yes, we do that all the time, with the exception of D3 cold/hot.
> 
> > For example, for a PCI device, ACPI advocates D0 and D3_COLD are supported,
> > and PCI layer may advocate D0, D3_HOT and D3_COLD are supported.
> 
> Exactly.  All of the available power states of the device depend on both
> the device's own capabilities (ie. what PCI D-states the device may be
> programmed into by register writes) and whatever is available from ACPI.
> 
> On embedded systems there are other factors as well.

So seems the patch is much simpler.

How about below?

 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

Re: [PATCH 1/6] ACPI: D3cold state is always valid

[Rafael J. Wysocki]

On Thursday, April 19, 2012, Lin Ming wrote:
> On Thu, 2012-04-19 at 13:51 +0200, Rafael J. Wysocki wrote:
> > On Thursday, April 19, 2012, huang ying wrote:
> > > On Thu, Apr 19, 2012 at 5:13 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > On Tuesday, April 17, 2012, huang ying wrote:
> > > >> On Tue, Apr 17, 2012 at 1:47 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> > > >> > ACPI_STATE_D3 actually means ACPI D3hot which is not always valid.
> > > >> > Instead, ACPI D3cold is always valid.
> > > >> >
> > > >> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > > >> > ---
> > > >> >  drivers/acpi/scan.c |   11 ++---------
> > > >> >  1 files changed, 2 insertions(+), 9 deletions(-)
> > > >> >
> > > >> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > >> > index 767e2dc..fb56388 100644
> > > >> > --- a/drivers/acpi/scan.c
> > > >> > +++ b/drivers/acpi/scan.c
> > > >> > @@ -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);
> > > >> > @@ -908,8 +901,8 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
> > > >> >        /* Set defaults for D0 and D3 states (always valid) */
> > > >> >        device->power.states[ACPI_STATE_D0].flags.valid = 1;
> > > >> >        device->power.states[ACPI_STATE_D0].power = 100;
> > > >> > -       device->power.states[ACPI_STATE_D3].flags.valid = 1;
> > > >> > -       device->power.states[ACPI_STATE_D3].power = 0;
> > > >> > +       device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
> > > >> > +       device->power.states[ACPI_STATE_D3_COLD].power = 0;
> > > >> >
> > > >> >        acpi_bus_init_power(device);
> > > >>
> > > >> I think D3_HOT should be always valid, while D3_COLD should be valid
> > > >> for some situation.
> > > >
> > > > This need not be PCI, mind you.
> > > >
> > > >>  - has _PS3, no _PR3
> > > >>    - support D3_HOT, D3_COLD
> > > >
> > > > Nope.  D3_HOT cannot be supported in that case at the ACPI level.
> > > >
> > > >>    - set state
> > > >>      - D3_HOT: do nothing in ACPI
> > > >
> > > > That is not D3_HOT, then, from the ACPI point of view.  It is a different
> > > > power state.
> > > >
> > > > Suppose you have a non-PCI device that can be only power-manageable via ACPI
> > > > and that device has only _PS0 and _PS3.  How would you put it into D3_HOT,
> > > > in particular?
> > > 
> > > Normally, we will put it into D3_COLD (via _PS3).
> > > 
> > > If it is prevented to be put in D3_COLD,
> > >   - If D3_HOT is not marked as supported, we will keep it in D0
> > >   - otherwise, we advocate we put it into D3_HOT, but in fact, it is in D0.
> > > 
> > > The result is same, but with wrong name.
> > > 
> > > But there will be some real problem if we have something like CPU
> > > governor.  Because governor may choose D3_HOT for device.
> > 
> > In that case the ACPI layer should simply return an error code indicating
> > that the requested state is not available _from_ _it_.  If there are
> > more layers, however, they may be able to change the power state of
> > the device.
> > 
> > > But for PCI device, D3_HOT is supported for the device.
> > 
> > Yes, if the device supports native PM.  However, PCI D-states and ACPI D-states
> > are different things.  We kind of combine them in our PCI bus type.
> > 
> > > So the bus layer should combine the information from native power
> > > state supported and ACPI power state supported to determine which
> > > power states are supported?
> > 
> > Yes, we do that all the time, with the exception of D3 cold/hot.
> > 
> > > For example, for a PCI device, ACPI advocates D0 and D3_COLD are supported,
> > > and PCI layer may advocate D0, D3_HOT and D3_COLD are supported.
> > 
> > Exactly.  All of the available power states of the device depend on both
> > the device's own capabilities (ie. what PCI D-states the device may be
> > programmed into by register writes) and whatever is available from ACPI.
> > 
> > On embedded systems there are other factors as well.
> 
> So seems the patch is much simpler.

Yes, it should be simpler.

> How about below?

It looks good to me.  Feel free to add my ACK to the final version.

Thanks,
Rafael


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