Skip to contentSkip to conversation

ACPI: D3 states cleanup

PATCH In review Lin Ming 4 participants 5 messages Apr 23, 2012 – May 5, 2012

Subsystems (3)
lore
Share & tools
lore

Lin Ming

author
#0 lore raw

PATCH v3 1/1 ACPI: D3 states cleanup

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. D3hot is only valid if _PR3 present.
And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.

---

v3: D3hot is only valid if _PR3 present

 drivers/acpi/power.c   |    2 +-
 drivers/acpi/scan.c    |   17 +++++++----------
 drivers/pci/pci-acpi.c |    4 ++--
 include/acpi/actypes.h |    7 ++++---
 4 files changed, 14 insertions(+), 16 deletions(-)
drivers/acpi/power.c +1 -1
index 7049a7d..330bb4d 100644
@@ -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;
drivers/acpi/scan.c +7 -10
index 767e2dc..7417267 100644
@@ -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,21 +884,18 @@ 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);
 		if (ACPI_SUCCESS(status))
 			ps->flags.explicit_set = 1;
 
-		/* State is valid if we have some power control */
-		if (ps->resources.count || ps->flags.explicit_set)
+		/*
+		 * State is valid if 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;
 
 		ps->power = -1;	/* Unknown - driver assigned */
drivers/pci/pci-acpi.c +2 -2
index 0f150f2..1929c0c 100644
@@ -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;
include/acpi/actypes.h +4 -3
index eba6604..e8bcc47 100644
@@ -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
Signature · 1 line
1.7.2.5
Reviewed-by on v3 1/1
#1 lore raw
Quoted 12 lines · 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. D3hot is only valid if _PR3 present.
And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.

Thanks,
Aaron

Quoted 112 lines

---

v3: D3hot is only valid if _PR3 present

 drivers/acpi/power.c   |    2 +-
 drivers/acpi/scan.c    |   17 +++++++----------
 drivers/pci/pci-acpi.c |    4 ++--
 include/acpi/actypes.h |    7 ++++---
 4 files changed, 14 insertions(+), 16 deletions(-)
drivers/acpi/power.c +1 -1
index 7049a7d..330bb4d 100644
@@ -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;
drivers/acpi/scan.c +7 -10
index 767e2dc..7417267 100644
@@ -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,21 +884,18 @@ 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);
 		if (ACPI_SUCCESS(status))
 			ps->flags.explicit_set = 1;
 
-		/* State is valid if we have some power control */
-		if (ps->resources.count || ps->flags.explicit_set)
+		/*
+		 * State is valid if 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;
 
 		ps->power = -1;	/* Unknown - driver assigned */
drivers/pci/pci-acpi.c +2 -2
index 0f150f2..1929c0c 100644
@@ -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;
include/acpi/actypes.h +4 -3
index eba6604..e8bcc47 100644
@@ -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
Signature · 1 line
1.7.2.5
author
#2 lore raw
Quoted 14 lines · Aaron Lu
Quoted 12 lines · 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. D3hot is only valid if _PR3 present.
And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.

Quoted 6 lines

Thanks,
Aaron

Quoted 1 line

Len,

Will you apply this patch?

Thanks,
Lin Ming

Quoted 104 lines
Quoted 104 lines

---

v3: D3hot is only valid if _PR3 present

 drivers/acpi/power.c   |    2 +-
 drivers/acpi/scan.c    |   17 +++++++----------
 drivers/pci/pci-acpi.c |    4 ++--
 include/acpi/actypes.h |    7 ++++---

 4 files changed, 14 insertions(+), 16 deletions(-)
drivers/acpi/power.c +1 -1
index 7049a7d..330bb4d 100644
@@ -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;
drivers/acpi/scan.c +7 -10
index 767e2dc..7417267 100644
@@ -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,21 +884,18 @@ 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);
               if (ACPI_SUCCESS(status))
                       ps->flags.explicit_set = 1;
 
-             /* State is valid if we have some power control */
-             if (ps->resources.count || ps->flags.explicit_set)
+             /*
+              * State is valid if 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;
 

              ps->power = -1; /* Unknown - driver assigned */

drivers/pci/pci-acpi.c +0 -0
index 0f150f2..1929c0c 100644
@@ -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;

drivers/pci/pci-acpi.c +1 -1
@@ -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;

include/acpi/actypes.h +4 -3
index eba6604..e8bcc47 100644
@@ -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
 
Quoted 12 lines
Quoted 7 lines
include/acpi/actypes.h +0 -1
  #define ACPI_STATE_C0                   (u8) 0
--

1.7.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
In reply to Lin Ming

Hi, Ming,

Quoted 11 lines · 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. D3hot is only valid if _PR3 present.
And changes ACPI_STATE_D3 to refer to ACPI D3cold state only. Also redefines ACPI_STATE_D3_COLD the same as ACPI_STATE_D3.

The changelog should say why we make the change instead of how we make the change. The code itself can tell me how we have done well. I prefer to see the description about

- the history of ACPI_STATE_D3, ACPI_STATE_D3_HOT, ACPI_STATE_D3_COLD
- if no _PR3, _PS3 may put device into D3_HOT or D3_COLD, decided by platform. But we will assume _PS3 will put device into D3_COLD, because it is safe to treat device in D3_HOT as D3_COLD, but not in contrary way.

And I think it will be more clear if we list the power state supporting under various combination, for example,

- has _PS3, no _PR3
- Valid states: D3_COLD
- Set power state
- D3_COLD: exec _PS3

Quoted 54 lines

---

v3: D3hot is only valid if _PR3 present

 drivers/acpi/power.c   |    2 +-
 drivers/acpi/scan.c    |   17 +++++++----------
 drivers/pci/pci-acpi.c |    4 ++--
 include/acpi/actypes.h |    7 ++++---

 4 files changed, 14 insertions(+), 16 deletions(-)
drivers/acpi/power.c +1 -1
index 7049a7d..330bb4d 100644
@@ -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;
drivers/acpi/scan.c +1 -8
index 767e2dc..7417267 100644
@@ -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,21 +884,18 @@ 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);
                if (ACPI_SUCCESS(status))
                        ps->flags.explicit_set = 1;

drivers/acpi/scan.c static int acpi_bus_get_power_flags(struct acpi_device *device) · line 899

Quoted 10 lines
drivers/acpi/scan.c +6 -2
 
-               /* State is valid if we have some power control */
-               if (ps->resources.count || ps->flags.explicit_set)
+               /*
+                * State is valid if 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;

I think we should add some comments here about why we do that. Why D3_HOT is not valid if only _PS3 presents.

Quoted 31 lines

               ps->power = -1; /* Unknown - driver assigned */

drivers/pci/pci-acpi.c +0 -0
index 0f150f2..1929c0c 100644
@@ -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;

drivers/pci/pci-acpi.c +1 -1
@@ -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;

include/acpi/actypes.h +0 -0
index eba6604..e8bcc47 100644
@@ -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

include/acpi/actypes.h typedef u64 acpi_integer; · line 508

Quoted 10 lines
include/acpi/actypes.h +4 -3
-#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

Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

reviewer Acked-by Rafael J. Wysocki Reviewed-by Aaron Lu on v3 1/1
#4 lore raw
In reply to Lin Ming

Applied using updated check-in comment below.

thanks,
-Len Brown, Intel Open Source Technology Center

From: Lin Ming <[email protected]>
Subject: [PATCH] ACPI: Fix D3hot v D3cold confusion

Before this patch, ACPI_STATE_D3 incorrectly referenced D3hot in some places, but D3cold in other places.

After this patch, ACPI_STATE_D3 always means ACPI_STATE_D3_COLD; and all references to D3hot use ACPI_STATE_D3_HOT.

ACPI's _PR3 method is used to enter both D3hot and D3cold states. What distinguishes D3hot from D3cold is the presence _PR3 (Power Resources for D3hot) If these resources are all ON, then the state is D3hot. If _PR3 is not present,
or all _PR0 resources for the devices are OFF,
then the state is D3cold.

This patch applies after Linux-3.4-rc1.
A future syntax cleanup may remove ACPI_STATE_D3 to emphasize that it always means ACPI_STATE_D3_COLD.

---
 drivers/acpi/power.c   |  2 +-
 drivers/acpi/scan.c    | 17 +++++++----------
 drivers/pci/pci-acpi.c |  4 ++--
 include/acpi/actypes.h |  7 ++++---
 4 files changed, 14 insertions(+), 16 deletions(-)