[PATCH 0/4] ACPI / PM: Cleanups and device power state selection rework

[PATCH 0/4] ACPI / PM: Cleanups and device power state selection rework

[Rafael J. Wysocki]

Hi All,

The following series of patches cleans up the ACPI device PM core code and
reworks the routine used for selecting power states to put devices into.

[1/4] Rename a function whose name is the same as the name of a data type and
      make it static.

[2/4] Replace ACPI_STATE_D3 with ACPI_STATE_D3_COLD in device_pm.c.

[3/4] Rework functions used for selecting power states to put devices into.

[4/4] Drop some unused code.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 1/4] ACPI / PM: Rename function acpi_device_power_state() and make it static

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There is a name clash between function acpi_device_power_state()
defined in drivers/acpi/device_pm.c and structure data type
acpi_device_power_state defined in include/acpi/acpi_bus.h, which
may be resolved by renaming the function.  Additionally, that
funtion may be made static, because it is not used anywhere outside
of the file it is defined in.

Rename acpi_device_power_state() to acpi_dev_pm_get_state(), which
better reflects its purpose, and make it static.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   15 +++++++--------
 include/acpi/acpi_bus.h  |   16 ++--------------
 2 files changed, 9 insertions(+), 22 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -460,8 +460,6 @@ acpi_status acpi_add_pm_notifier(struct
 				 acpi_notify_handler handler, void *context);
 acpi_status acpi_remove_pm_notifier(struct acpi_device *adev,
 				    acpi_notify_handler handler);
-int acpi_device_power_state(struct device *dev, struct acpi_device *adev,
-			    u32 target_state, int d_max_in, int *d_min_p);
 int acpi_pm_device_sleep_state(struct device *, int *, int);
 void acpi_dev_pm_add_dependent(acpi_handle handle, struct device *depdev);
 void acpi_dev_pm_remove_dependent(acpi_handle handle, struct device *depdev);
@@ -477,23 +475,13 @@ static inline acpi_status acpi_remove_pm
 {
 	return AE_SUPPORT;
 }
-static inline int __acpi_device_power_state(int m, int *p)
+static inline int acpi_pm_device_sleep_state(struct device *d, int *p, int m)
 {
 	if (p)
 		*p = ACPI_STATE_D0;
+
 	return (m >= ACPI_STATE_D0 && m <= ACPI_STATE_D3) ? m : ACPI_STATE_D0;
 }
-static inline int acpi_device_power_state(struct device *dev,
-					  struct acpi_device *adev,
-					  u32 target_state, int d_max_in,
-					  int *d_min_p)
-{
-	return __acpi_device_power_state(d_max_in, d_min_p);
-}
-static inline int acpi_pm_device_sleep_state(struct device *d, int *p, int m)
-{
-	return __acpi_device_power_state(m, p);
-}
 static inline void acpi_dev_pm_add_dependent(acpi_handle handle,
 					     struct device *depdev) {}
 static inline void acpi_dev_pm_remove_dependent(acpi_handle handle,
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -399,7 +399,7 @@ bool acpi_bus_can_wakeup(acpi_handle han
 EXPORT_SYMBOL(acpi_bus_can_wakeup);
 
 /**
- * acpi_device_power_state - Get preferred power state of ACPI device.
+ * acpi_dev_pm_get_state - Get preferred power state of ACPI device.
  * @dev: Device whose preferred target power state to return.
  * @adev: ACPI device node corresponding to @dev.
  * @target_state: System state to match the resultant device state.
@@ -417,8 +417,8 @@ EXPORT_SYMBOL(acpi_bus_can_wakeup);
  * Callers must ensure that @dev and @adev are valid pointers and that @adev
  * actually corresponds to @dev before using this function.
  */
-int acpi_device_power_state(struct device *dev, struct acpi_device *adev,
-			    u32 target_state, int d_max_in, int *d_min_p)
+static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
+				 u32 target_state, int d_max_in, int *d_min_p)
 {
 	char acpi_method[] = "_SxD";
 	unsigned long long d_min, d_max;
@@ -501,7 +501,6 @@ int acpi_device_power_state(struct devic
 	}
 	return d_max;
 }
-EXPORT_SYMBOL_GPL(acpi_device_power_state);
 
 /**
  * acpi_pm_device_sleep_state - Get preferred power state of ACPI device.
@@ -523,8 +522,8 @@ int acpi_pm_device_sleep_state(struct de
 		return -ENODEV;
 	}
 
-	return acpi_device_power_state(dev, adev, acpi_target_system_state(),
-				       d_max_in, d_min_p);
+	return acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
+				     d_max_in, d_min_p);
 }
 EXPORT_SYMBOL(acpi_pm_device_sleep_state);
 
@@ -680,8 +679,8 @@ static int acpi_dev_pm_low_power(struct
 	if (!acpi_device_power_manageable(adev))
 		return 0;
 
-	power_state = acpi_device_power_state(dev, adev, system_state,
-					      ACPI_STATE_D3, NULL);
+	power_state = acpi_dev_pm_get_state(dev, adev, system_state,
+					    ACPI_STATE_D3, NULL);
 	if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3)
 		return -EIO;
 

[PATCH 2/4] ACPI / PM: Replace ACPI_STATE_D3 with ACPI_STATE_D3_COLD in device_pm.c

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The two symbols ACPI_STATE_D3 and ACPI_STATE_D3_COLD actually
represent the same number (4), but ACPI_STATE_D3 is slightly
ambigugous, because it may not be clear that it really means D3cold
and not D3hot at first sight.

Remove that ambiguity from drivers/acpi/device_pm.c by making it
use ACPI_STATE_D3_COLD everywhere instead of ACPI_STATE_D3.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -424,7 +424,7 @@ static int acpi_dev_pm_get_state(struct
 	unsigned long long d_min, d_max;
 	bool wakeup = false;
 
-	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3)
+	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
 		return -EINVAL;
 
 	if (d_max_in > ACPI_STATE_D3_HOT) {
@@ -443,7 +443,7 @@ static int acpi_dev_pm_get_state(struct
 	 * the lowest limit with the specified one.
 	 */
 	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
@@ -680,8 +680,8 @@ static int acpi_dev_pm_low_power(struct
 		return 0;
 
 	power_state = acpi_dev_pm_get_state(dev, adev, system_state,
-					    ACPI_STATE_D3, NULL);
-	if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3)
+					    ACPI_STATE_D3_COLD, NULL);
+	if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3_COLD)
 		return -EIO;
 
 	return acpi_device_set_power(adev, power_state);

[PATCH 3/4] ACPI / PM: Rework and clean up acpi_dev_pm_get_state()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The acpi_dev_pm_get_state() function defined in device_pm.c is quite
convoluted, which isn't really necessary, and it doesn't validate the
values returned by the ACPI methods executed by it appropriately.

To address these shortcomings modify it in the following way.

 (1) Make its return value only mean whether or not it succeeded and
     pass the device power states determined by it through pointers.

 (2) Drop the d_max_in argument, used by only one of its callers,
     from it, and move the code related to d_max_in into that caller,
     acpi_pm_device_sleep_state().

 (3) Make it always check the return value of acpi_evaluate_integer()
     and handle failures as appropriate.  Moreover, make it check if
     the values returned by the executed ACPI methods are not out of
     range.

 (4) Make it check if the values returned by the executed ACPI
     methods represent valid power states of the given device and
     handle situations in which that's not the case gracefully.

Also update the kerneldoc comments of acpi_dev_pm_get_state() and
acpi_pm_device_sleep_state() to reflect the code changes.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |  158 +++++++++++++++++++++++++++--------------------
 1 file changed, 92 insertions(+), 66 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -403,44 +403,37 @@ EXPORT_SYMBOL(acpi_bus_can_wakeup);
  * @dev: Device whose preferred target power state to return.
  * @adev: ACPI device node corresponding to @dev.
  * @target_state: System state to match the resultant device state.
- * @d_max_in: Deepest low-power state to take into consideration.
- * @d_min_p: Location to store the upper limit of the allowed states range.
- * Return value: Preferred power state of the device on success, -ENODEV
- * (if there's no 'struct acpi_device' for @dev) or -EINVAL on failure
+ * @d_min_p: Location to store the highest power state available to the device.
+ * @d_max_p: Location to store the lowest power state available to the device.
  *
- * Find the lowest power (highest number) ACPI device power state that the
- * device can be in while the system is in the state represented by
- * @target_state.  If @d_min_p is set, the highest power (lowest number) device
- * power state that @dev can be in for the given system sleep state is stored
- * at the location pointed to by it.
+ * Find the lowest power (highest number) and highest power (lowest number) ACPI
+ * device power states that the device can be in while the system is in the
+ * state represented by @target_state.  Store the integer numbers representing
+ * those stats in the memory locations pointed to by @d_max_p and @d_min_p,
+ * respectively.
  *
  * Callers must ensure that @dev and @adev are valid pointers and that @adev
  * actually corresponds to @dev before using this function.
+ *
+ * Returns 0 on success or -ENODATA when one of the ACPI methods fails or
+ * returns a value that doesn't make sense.  The memory locations pointed to by
+ * @d_max_p and @d_min_p are only modified on success.
  */
 static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
-				 u32 target_state, int d_max_in, int *d_min_p)
+				 u32 target_state, int *d_min_p, int *d_max_p)
 {
-	char acpi_method[] = "_SxD";
-	unsigned long long d_min, d_max;
+	char method[] = { '_', 'S', '0' + target_state, 'D', '\0' };
+	acpi_handle handle = adev->handle;
+	unsigned long long ret;
+	int d_min, d_max;
 	bool wakeup = false;
+	acpi_status status;
 
-	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
-		return -EINVAL;
-
-	if (d_max_in > ACPI_STATE_D3_HOT) {
-		enum pm_qos_flags_status stat;
-
-		stat = dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF);
-		if (stat == PM_QOS_FLAGS_ALL)
-			d_max_in = ACPI_STATE_D3_HOT;
-	}
-
-	acpi_method[2] = '0' + target_state;
 	/*
-	 * If the sleep state is S0, the lowest limit from ACPI is D3,
-	 * but if the device has _S0W, we will use the value from _S0W
-	 * as the lowest limit from ACPI.  Finally, we will constrain
-	 * the lowest limit with the specified one.
+	 * If the system state is S0, the lowest power state the device can be
+	 * in is D3cold, unless the device has _S0W and is supposed to signal
+	 * wakeup, in which case the return value of _S0W has to be used as the
+	 * lowest power state available to the device.
 	 */
 	d_min = ACPI_STATE_D0;
 	d_max = ACPI_STATE_D3_COLD;
@@ -449,12 +442,30 @@ static int acpi_dev_pm_get_state(struct
 	 * If present, _SxD methods return the minimum D-state (highest power
 	 * state) we can use for the corresponding S-states.  Otherwise, the
 	 * minimum D-state is D0 (ACPI 3.x).
-	 *
-	 * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
-	 * provided -- that's our fault recovery, we ignore retval.
 	 */
 	if (target_state > ACPI_STATE_S0) {
-		acpi_evaluate_integer(adev->handle, acpi_method, NULL, &d_min);
+		/*
+		 * We rely on acpi_evaluate_integer() not clobbering the integer
+		 * provided if AE_NOT_FOUND is returned.
+		 */
+		ret = d_min;
+		status = acpi_evaluate_integer(handle, method, NULL, &ret);
+		if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+		    || ret > ACPI_STATE_D3_COLD)
+			return -ENODATA;
+
+		/*
+		 * We need to handle legacy systems where D3hot and D3cold are
+		 * the same and 3 is returned in both cases, so fall back to
+		 * D3cold if D3hot is not a valid state.
+		 */
+		if (!adev->power.states[ret].flags.valid) {
+			if (ret == ACPI_STATE_D3_HOT)
+				ret = ACPI_STATE_D3_COLD;
+			else
+				return -ENODATA;
+		}
+		d_min = ret;
 		wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
 			&& adev->wakeup.sleep_state >= target_state;
 	} else if (dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP) !=
@@ -470,36 +481,29 @@ static int acpi_dev_pm_get_state(struct
 	 * can wake the system.  _S0W may be valid, too.
 	 */
 	if (wakeup) {
-		acpi_status status;
-
-		acpi_method[3] = 'W';
-		status = acpi_evaluate_integer(adev->handle, acpi_method, NULL,
-						&d_max);
-		if (ACPI_FAILURE(status)) {
-			if (target_state != ACPI_STATE_S0 ||
-			    status != AE_NOT_FOUND)
+		method[3] = 'W';
+		status = acpi_evaluate_integer(handle, method, NULL, &ret);
+		if (status == AE_NOT_FOUND) {
+			if (target_state > ACPI_STATE_S0)
 				d_max = d_min;
-		} else if (d_max < d_min) {
-			/* Warn the user of the broken DSDT */
-			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
-				acpi_method);
-			/* Sanitize it */
-			d_min = d_max;
+		} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
+			/* Fall back to D3cold if ret is not a valid state. */
+			if (!adev->power.states[ret].flags.valid)
+				ret = ACPI_STATE_D3_COLD;
+
+			d_max = ret > d_min ? ret : d_min;
+		} else {
+			return -ENODATA;
 		}
 	}
 
-	if (d_max_in < d_min)
-		return -EINVAL;
 	if (d_min_p)
 		*d_min_p = d_min;
-	/* constrain d_max with specified lowest limit (max number) */
-	if (d_max > d_max_in) {
-		for (d_max = d_max_in; d_max > d_min; d_max--) {
-			if (adev->power.states[d_max].flags.valid)
-				break;
-		}
-	}
-	return d_max;
+
+	if (d_max_p)
+		*d_max_p = d_max;
+
+	return 0;
 }
 
 /**
@@ -508,7 +512,8 @@ static int acpi_dev_pm_get_state(struct
  * @d_min_p: Location to store the upper limit of the allowed states range.
  * @d_max_in: Deepest low-power state to take into consideration.
  * Return value: Preferred power state of the device on success, -ENODEV
- * (if there's no 'struct acpi_device' for @dev) or -EINVAL on failure
+ * if there's no 'struct acpi_device' for @dev, -EINVAL if @d_max_in is
+ * incorrect, or -ENODATA on ACPI method failure.
  *
  * The caller must ensure that @dev is valid before using this function.
  */
@@ -516,14 +521,39 @@ int acpi_pm_device_sleep_state(struct de
 {
 	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
 	struct acpi_device *adev;
+	int ret, d_max;
+
+	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
+		return -EINVAL;
+
+	if (d_max_in > ACPI_STATE_D3_HOT) {
+		enum pm_qos_flags_status stat;
+
+		stat = dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF);
+		if (stat == PM_QOS_FLAGS_ALL)
+			d_max_in = ACPI_STATE_D3_HOT;
+	}
 
 	if (!handle || acpi_bus_get_device(handle, &adev)) {
 		dev_dbg(dev, "ACPI handle without context in %s!\n", __func__);
 		return -ENODEV;
 	}
 
-	return acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
-				     d_max_in, d_min_p);
+	ret = acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
+				    d_min_p, &d_max);
+	if (ret)
+		return ret;
+
+	if (d_max_in < *d_min_p)
+		return -EINVAL;
+
+	if (d_max > d_max_in) {
+		for (d_max = d_max_in; d_max > *d_min_p; d_max--) {
+			if (adev->power.states[d_max].flags.valid)
+				break;
+		}
+	}
+	return d_max;
 }
 EXPORT_SYMBOL(acpi_pm_device_sleep_state);
 
@@ -674,17 +704,13 @@ struct acpi_device *acpi_dev_pm_get_node
 static int acpi_dev_pm_low_power(struct device *dev, struct acpi_device *adev,
 				 u32 system_state)
 {
-	int power_state;
+	int ret, state;
 
 	if (!acpi_device_power_manageable(adev))
 		return 0;
 
-	power_state = acpi_dev_pm_get_state(dev, adev, system_state,
-					    ACPI_STATE_D3_COLD, NULL);
-	if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3_COLD)
-		return -EIO;
-
-	return acpi_device_set_power(adev, power_state);
+	ret = acpi_dev_pm_get_state(dev, adev, system_state, NULL, &state);
+	return ret ? ret : acpi_device_set_power(adev, state);
 }
 
 /**

[PATCH 4/4] ACPI / PM: Drop two functions that are not used any more

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Two functions defined in device_pm.c, acpi_dev_pm_add_dependent()
and acpi_dev_pm_remove_dependent(), have no callers and may be
dropped, so drop them.

Moreover, they are the only functions adding entries to and removing
entries from the power_dependent list in struct acpi_device, so drop
that list and the loop walking it in acpi_power_resume_dependent()
too.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   56 -----------------------------------------------
 drivers/acpi/power.c     |    3 --
 drivers/acpi/scan.c      |    1 
 include/acpi/acpi_bus.h  |    7 -----
 4 files changed, 67 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -308,7 +308,6 @@ struct acpi_device {
 	struct list_head physical_node_list;
 	struct mutex physical_node_lock;
 	DECLARE_BITMAP(physical_node_id_bitmap, ACPI_MAX_PHYSICAL_NODE);
-	struct list_head power_dependent;
 	void (*remove)(struct acpi_device *);
 };
 
@@ -461,8 +460,6 @@ acpi_status acpi_add_pm_notifier(struct
 acpi_status acpi_remove_pm_notifier(struct acpi_device *adev,
 				    acpi_notify_handler handler);
 int acpi_pm_device_sleep_state(struct device *, int *, int);
-void acpi_dev_pm_add_dependent(acpi_handle handle, struct device *depdev);
-void acpi_dev_pm_remove_dependent(acpi_handle handle, struct device *depdev);
 #else
 static inline acpi_status acpi_add_pm_notifier(struct acpi_device *adev,
 					       acpi_notify_handler handler,
@@ -482,10 +479,6 @@ static inline int acpi_pm_device_sleep_s
 
 	return (m >= ACPI_STATE_D0 && m <= ACPI_STATE_D3) ? m : ACPI_STATE_D0;
 }
-static inline void acpi_dev_pm_add_dependent(acpi_handle handle,
-					     struct device *depdev) {}
-static inline void acpi_dev_pm_remove_dependent(acpi_handle handle,
-						struct device *depdev) {}
 #endif
 
 #ifdef CONFIG_PM_RUNTIME
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -994,60 +994,4 @@ void acpi_dev_pm_detach(struct device *d
 	}
 }
 EXPORT_SYMBOL_GPL(acpi_dev_pm_detach);
-
-/**
- * acpi_dev_pm_add_dependent - Add physical device depending for PM.
- * @handle: Handle of ACPI device node.
- * @depdev: Device depending on that node for PM.
- */
-void acpi_dev_pm_add_dependent(acpi_handle handle, struct device *depdev)
-{
-	struct acpi_device_physical_node *dep;
-	struct acpi_device *adev;
-
-	if (!depdev || acpi_bus_get_device(handle, &adev))
-		return;
-
-	mutex_lock(&adev->physical_node_lock);
-
-	list_for_each_entry(dep, &adev->power_dependent, node)
-		if (dep->dev == depdev)
-			goto out;
-
-	dep = kzalloc(sizeof(*dep), GFP_KERNEL);
-	if (dep) {
-		dep->dev = depdev;
-		list_add_tail(&dep->node, &adev->power_dependent);
-	}
-
- out:
-	mutex_unlock(&adev->physical_node_lock);
-}
-EXPORT_SYMBOL_GPL(acpi_dev_pm_add_dependent);
-
-/**
- * acpi_dev_pm_remove_dependent - Remove physical device depending for PM.
- * @handle: Handle of ACPI device node.
- * @depdev: Device depending on that node for PM.
- */
-void acpi_dev_pm_remove_dependent(acpi_handle handle, struct device *depdev)
-{
-	struct acpi_device_physical_node *dep;
-	struct acpi_device *adev;
-
-	if (!depdev || acpi_bus_get_device(handle, &adev))
-		return;
-
-	mutex_lock(&adev->physical_node_lock);
-
-	list_for_each_entry(dep, &adev->power_dependent, node)
-		if (dep->dev == depdev) {
-			list_del(&dep->node);
-			kfree(dep);
-			break;
-		}
-
-	mutex_unlock(&adev->physical_node_lock);
-}
-EXPORT_SYMBOL_GPL(acpi_dev_pm_remove_dependent);
 #endif /* CONFIG_PM */
Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -253,9 +253,6 @@ static void acpi_power_resume_dependent(
 	list_for_each_entry(pn, &adev->physical_node_list, node)
 		pm_request_resume(pn->dev);
 
-	list_for_each_entry(pn, &adev->power_dependent, node)
-		pm_request_resume(pn->dev);
-
 	mutex_unlock(&adev->physical_node_lock);
 }
 
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1018,7 +1018,6 @@ int acpi_device_add(struct acpi_device *
 	INIT_LIST_HEAD(&device->wakeup_list);
 	INIT_LIST_HEAD(&device->physical_node_list);
 	mutex_init(&device->physical_node_lock);
-	INIT_LIST_HEAD(&device->power_dependent);
 
 	new_bus_id = kzalloc(sizeof(struct acpi_device_bus_id), GFP_KERNEL);
 	if (!new_bus_id) {

Re: [PATCH 4/4] ACPI / PM: Drop two functions that are not used any more

[Aaron Lu]

On 06/14/2013 08:33 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Two functions defined in device_pm.c, acpi_dev_pm_add_dependent()
> and acpi_dev_pm_remove_dependent(), have no callers and may be
> dropped, so drop them.

Please hold on for a moment, I would like to discuss the ATA ACPI
binding with you and Tejun, the current binding with SCSI device tree
doesn't buy us anything but introduced some nasty consequences. I think
we should bind ACPI handle with ATA port/device, I have a draft patch
for this and will send out soon. If we go binding with ATA port/device,
then the two functions will be needed :-)

Thanks,
Aaron

> 
> Moreover, they are the only functions adding entries to and removing
> entries from the power_dependent list in struct acpi_device, so drop
> that list and the loop walking it in acpi_power_resume_dependent()
> too.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/device_pm.c |   56 -----------------------------------------------
>  drivers/acpi/power.c     |    3 --
>  drivers/acpi/scan.c      |    1 
>  include/acpi/acpi_bus.h  |    7 -----
>  4 files changed, 67 deletions(-)
> 
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -308,7 +308,6 @@ struct acpi_device {
>  	struct list_head physical_node_list;
>  	struct mutex physical_node_lock;
>  	DECLARE_BITMAP(physical_node_id_bitmap, ACPI_MAX_PHYSICAL_NODE);
> -	struct list_head power_dependent;
>  	void (*remove)(struct acpi_device *);
>  };
>  
> @@ -461,8 +460,6 @@ acpi_status acpi_add_pm_notifier(struct
>  acpi_status acpi_remove_pm_notifier(struct acpi_device *adev,
>  				    acpi_notify_handler handler);
>  int acpi_pm_device_sleep_state(struct device *, int *, int);
> -void acpi_dev_pm_add_dependent(acpi_handle handle, struct device *depdev);
> -void acpi_dev_pm_remove_dependent(acpi_handle handle, struct device *depdev);
>  #else
>  static inline acpi_status acpi_add_pm_notifier(struct acpi_device *adev,
>  					       acpi_notify_handler handler,
> @@ -482,10 +479,6 @@ static inline int acpi_pm_device_sleep_s
>  
>  	return (m >= ACPI_STATE_D0 && m <= ACPI_STATE_D3) ? m : ACPI_STATE_D0;
>  }
> -static inline void acpi_dev_pm_add_dependent(acpi_handle handle,
> -					     struct device *depdev) {}
> -static inline void acpi_dev_pm_remove_dependent(acpi_handle handle,
> -						struct device *depdev) {}
>  #endif
>  
>  #ifdef CONFIG_PM_RUNTIME
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -994,60 +994,4 @@ void acpi_dev_pm_detach(struct device *d
>  	}
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_pm_detach);
> -
> -/**
> - * acpi_dev_pm_add_dependent - Add physical device depending for PM.
> - * @handle: Handle of ACPI device node.
> - * @depdev: Device depending on that node for PM.
> - */
> -void acpi_dev_pm_add_dependent(acpi_handle handle, struct device *depdev)
> -{
> -	struct acpi_device_physical_node *dep;
> -	struct acpi_device *adev;
> -
> -	if (!depdev || acpi_bus_get_device(handle, &adev))
> -		return;
> -
> -	mutex_lock(&adev->physical_node_lock);
> -
> -	list_for_each_entry(dep, &adev->power_dependent, node)
> -		if (dep->dev == depdev)
> -			goto out;
> -
> -	dep = kzalloc(sizeof(*dep), GFP_KERNEL);
> -	if (dep) {
> -		dep->dev = depdev;
> -		list_add_tail(&dep->node, &adev->power_dependent);
> -	}
> -
> - out:
> -	mutex_unlock(&adev->physical_node_lock);
> -}
> -EXPORT_SYMBOL_GPL(acpi_dev_pm_add_dependent);
> -
> -/**
> - * acpi_dev_pm_remove_dependent - Remove physical device depending for PM.
> - * @handle: Handle of ACPI device node.
> - * @depdev: Device depending on that node for PM.
> - */
> -void acpi_dev_pm_remove_dependent(acpi_handle handle, struct device *depdev)
> -{
> -	struct acpi_device_physical_node *dep;
> -	struct acpi_device *adev;
> -
> -	if (!depdev || acpi_bus_get_device(handle, &adev))
> -		return;
> -
> -	mutex_lock(&adev->physical_node_lock);
> -
> -	list_for_each_entry(dep, &adev->power_dependent, node)
> -		if (dep->dev == depdev) {
> -			list_del(&dep->node);
> -			kfree(dep);
> -			break;
> -		}
> -
> -	mutex_unlock(&adev->physical_node_lock);
> -}
> -EXPORT_SYMBOL_GPL(acpi_dev_pm_remove_dependent);
>  #endif /* CONFIG_PM */
> Index: linux-pm/drivers/acpi/power.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/power.c
> +++ linux-pm/drivers/acpi/power.c
> @@ -253,9 +253,6 @@ static void acpi_power_resume_dependent(
>  	list_for_each_entry(pn, &adev->physical_node_list, node)
>  		pm_request_resume(pn->dev);
>  
> -	list_for_each_entry(pn, &adev->power_dependent, node)
> -		pm_request_resume(pn->dev);
> -
>  	mutex_unlock(&adev->physical_node_lock);
>  }
>  
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1018,7 +1018,6 @@ int acpi_device_add(struct acpi_device *
>  	INIT_LIST_HEAD(&device->wakeup_list);
>  	INIT_LIST_HEAD(&device->physical_node_list);
>  	mutex_init(&device->physical_node_lock);
> -	INIT_LIST_HEAD(&device->power_dependent);
>  
>  	new_bus_id = kzalloc(sizeof(struct acpi_device_bus_id), GFP_KERNEL);
>  	if (!new_bus_id) {
> 

Re: [PATCH 4/4] ACPI / PM: Drop two functions that are not used any more

[Rafael J. Wysocki]

On Tuesday, June 18, 2013 05:20:36 PM Aaron Lu wrote:
> On 06/14/2013 08:33 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Two functions defined in device_pm.c, acpi_dev_pm_add_dependent()
> > and acpi_dev_pm_remove_dependent(), have no callers and may be
> > dropped, so drop them.
> 
> Please hold on for a moment, I would like to discuss the ATA ACPI
> binding with you and Tejun, the current binding with SCSI device tree
> doesn't buy us anything but introduced some nasty consequences. I think
> we should bind ACPI handle with ATA port/device, I have a draft patch
> for this and will send out soon. If we go binding with ATA port/device,
> then the two functions will be needed :-)

OK, this change can wait.  I'll drop [4/4] for now.

Thanks,
Rafael


> > Moreover, they are the only functions adding entries to and removing
> > entries from the power_dependent list in struct acpi_device, so drop
> > that list and the loop walking it in acpi_power_resume_dependent()
> > too.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/device_pm.c |   56 -----------------------------------------------
> >  drivers/acpi/power.c     |    3 --
> >  drivers/acpi/scan.c      |    1 
> >  include/acpi/acpi_bus.h  |    7 -----
> >  4 files changed, 67 deletions(-)
> > 
> > Index: linux-pm/include/acpi/acpi_bus.h
> > ===================================================================
> > --- linux-pm.orig/include/acpi/acpi_bus.h
> > +++ linux-pm/include/acpi/acpi_bus.h
> > @@ -308,7 +308,6 @@ struct acpi_device {
> >  	struct list_head physical_node_list;
> >  	struct mutex physical_node_lock;
> >  	DECLARE_BITMAP(physical_node_id_bitmap, ACPI_MAX_PHYSICAL_NODE);
> > -	struct list_head power_dependent;
> >  	void (*remove)(struct acpi_device *);
> >  };
> >  
> > @@ -461,8 +460,6 @@ acpi_status acpi_add_pm_notifier(struct
> >  acpi_status acpi_remove_pm_notifier(struct acpi_device *adev,
> >  				    acpi_notify_handler handler);
> >  int acpi_pm_device_sleep_state(struct device *, int *, int);
> > -void acpi_dev_pm_add_dependent(acpi_handle handle, struct device *depdev);
> > -void acpi_dev_pm_remove_dependent(acpi_handle handle, struct device *depdev);
> >  #else
> >  static inline acpi_status acpi_add_pm_notifier(struct acpi_device *adev,
> >  					       acpi_notify_handler handler,
> > @@ -482,10 +479,6 @@ static inline int acpi_pm_device_sleep_s
> >  
> >  	return (m >= ACPI_STATE_D0 && m <= ACPI_STATE_D3) ? m : ACPI_STATE_D0;
> >  }
> > -static inline void acpi_dev_pm_add_dependent(acpi_handle handle,
> > -					     struct device *depdev) {}
> > -static inline void acpi_dev_pm_remove_dependent(acpi_handle handle,
> > -						struct device *depdev) {}
> >  #endif
> >  
> >  #ifdef CONFIG_PM_RUNTIME
> > Index: linux-pm/drivers/acpi/device_pm.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/device_pm.c
> > +++ linux-pm/drivers/acpi/device_pm.c
> > @@ -994,60 +994,4 @@ void acpi_dev_pm_detach(struct device *d
> >  	}
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_dev_pm_detach);
> > -
> > -/**
> > - * acpi_dev_pm_add_dependent - Add physical device depending for PM.
> > - * @handle: Handle of ACPI device node.
> > - * @depdev: Device depending on that node for PM.
> > - */
> > -void acpi_dev_pm_add_dependent(acpi_handle handle, struct device *depdev)
> > -{
> > -	struct acpi_device_physical_node *dep;
> > -	struct acpi_device *adev;
> > -
> > -	if (!depdev || acpi_bus_get_device(handle, &adev))
> > -		return;
> > -
> > -	mutex_lock(&adev->physical_node_lock);
> > -
> > -	list_for_each_entry(dep, &adev->power_dependent, node)
> > -		if (dep->dev == depdev)
> > -			goto out;
> > -
> > -	dep = kzalloc(sizeof(*dep), GFP_KERNEL);
> > -	if (dep) {
> > -		dep->dev = depdev;
> > -		list_add_tail(&dep->node, &adev->power_dependent);
> > -	}
> > -
> > - out:
> > -	mutex_unlock(&adev->physical_node_lock);
> > -}
> > -EXPORT_SYMBOL_GPL(acpi_dev_pm_add_dependent);
> > -
> > -/**
> > - * acpi_dev_pm_remove_dependent - Remove physical device depending for PM.
> > - * @handle: Handle of ACPI device node.
> > - * @depdev: Device depending on that node for PM.
> > - */
> > -void acpi_dev_pm_remove_dependent(acpi_handle handle, struct device *depdev)
> > -{
> > -	struct acpi_device_physical_node *dep;
> > -	struct acpi_device *adev;
> > -
> > -	if (!depdev || acpi_bus_get_device(handle, &adev))
> > -		return;
> > -
> > -	mutex_lock(&adev->physical_node_lock);
> > -
> > -	list_for_each_entry(dep, &adev->power_dependent, node)
> > -		if (dep->dev == depdev) {
> > -			list_del(&dep->node);
> > -			kfree(dep);
> > -			break;
> > -		}
> > -
> > -	mutex_unlock(&adev->physical_node_lock);
> > -}
> > -EXPORT_SYMBOL_GPL(acpi_dev_pm_remove_dependent);
> >  #endif /* CONFIG_PM */
> > Index: linux-pm/drivers/acpi/power.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/power.c
> > +++ linux-pm/drivers/acpi/power.c
> > @@ -253,9 +253,6 @@ static void acpi_power_resume_dependent(
> >  	list_for_each_entry(pn, &adev->physical_node_list, node)
> >  		pm_request_resume(pn->dev);
> >  
> > -	list_for_each_entry(pn, &adev->power_dependent, node)
> > -		pm_request_resume(pn->dev);
> > -
> >  	mutex_unlock(&adev->physical_node_lock);
> >  }
> >  
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -1018,7 +1018,6 @@ int acpi_device_add(struct acpi_device *
> >  	INIT_LIST_HEAD(&device->wakeup_list);
> >  	INIT_LIST_HEAD(&device->physical_node_list);
> >  	mutex_init(&device->physical_node_lock);
> > -	INIT_LIST_HEAD(&device->power_dependent);
> >  
> >  	new_bus_id = kzalloc(sizeof(struct acpi_device_bus_id), GFP_KERNEL);
> >  	if (!new_bus_id) {
> > 
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/4] ACPI / PM: Rework and clean up acpi_dev_pm_get_state()

[Aaron Lu]

On 06/14/2013 08:32 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The acpi_dev_pm_get_state() function defined in device_pm.c is quite
> convoluted, which isn't really necessary, and it doesn't validate the
> values returned by the ACPI methods executed by it appropriately.
> 
> To address these shortcomings modify it in the following way.
> 
>  (1) Make its return value only mean whether or not it succeeded and
>      pass the device power states determined by it through pointers.
> 
>  (2) Drop the d_max_in argument, used by only one of its callers,
>      from it, and move the code related to d_max_in into that caller,
>      acpi_pm_device_sleep_state().
> 
>  (3) Make it always check the return value of acpi_evaluate_integer()
>      and handle failures as appropriate.  Moreover, make it check if
>      the values returned by the executed ACPI methods are not out of
>      range.
> 
>  (4) Make it check if the values returned by the executed ACPI
>      methods represent valid power states of the given device and
>      handle situations in which that's not the case gracefully.
> 
> Also update the kerneldoc comments of acpi_dev_pm_get_state() and
> acpi_pm_device_sleep_state() to reflect the code changes.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/device_pm.c |  158 +++++++++++++++++++++++++++--------------------
>  1 file changed, 92 insertions(+), 66 deletions(-)
> 
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -403,44 +403,37 @@ EXPORT_SYMBOL(acpi_bus_can_wakeup);
>   * @dev: Device whose preferred target power state to return.
>   * @adev: ACPI device node corresponding to @dev.
>   * @target_state: System state to match the resultant device state.
> - * @d_max_in: Deepest low-power state to take into consideration.
> - * @d_min_p: Location to store the upper limit of the allowed states range.
> - * Return value: Preferred power state of the device on success, -ENODEV
> - * (if there's no 'struct acpi_device' for @dev) or -EINVAL on failure
> + * @d_min_p: Location to store the highest power state available to the device.
> + * @d_max_p: Location to store the lowest power state available to the device.
>   *
> - * Find the lowest power (highest number) ACPI device power state that the
> - * device can be in while the system is in the state represented by
> - * @target_state.  If @d_min_p is set, the highest power (lowest number) device
> - * power state that @dev can be in for the given system sleep state is stored
> - * at the location pointed to by it.
> + * Find the lowest power (highest number) and highest power (lowest number) ACPI
> + * device power states that the device can be in while the system is in the
> + * state represented by @target_state.  Store the integer numbers representing
> + * those stats in the memory locations pointed to by @d_max_p and @d_min_p,
> + * respectively.
>   *
>   * Callers must ensure that @dev and @adev are valid pointers and that @adev
>   * actually corresponds to @dev before using this function.
> + *
> + * Returns 0 on success or -ENODATA when one of the ACPI methods fails or
> + * returns a value that doesn't make sense.  The memory locations pointed to by
> + * @d_max_p and @d_min_p are only modified on success.
>   */
>  static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
> -				 u32 target_state, int d_max_in, int *d_min_p)
> +				 u32 target_state, int *d_min_p, int *d_max_p)
>  {
> -	char acpi_method[] = "_SxD";
> -	unsigned long long d_min, d_max;
> +	char method[] = { '_', 'S', '0' + target_state, 'D', '\0' };
> +	acpi_handle handle = adev->handle;
> +	unsigned long long ret;
> +	int d_min, d_max;
>  	bool wakeup = false;
> +	acpi_status status;
>  
> -	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
> -		return -EINVAL;
> -
> -	if (d_max_in > ACPI_STATE_D3_HOT) {
> -		enum pm_qos_flags_status stat;
> -
> -		stat = dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF);
> -		if (stat == PM_QOS_FLAGS_ALL)
> -			d_max_in = ACPI_STATE_D3_HOT;
> -	}
> -
> -	acpi_method[2] = '0' + target_state;
>  	/*
> -	 * If the sleep state is S0, the lowest limit from ACPI is D3,
> -	 * but if the device has _S0W, we will use the value from _S0W
> -	 * as the lowest limit from ACPI.  Finally, we will constrain
> -	 * the lowest limit with the specified one.
> +	 * If the system state is S0, the lowest power state the device can be
> +	 * in is D3cold, unless the device has _S0W and is supposed to signal
> +	 * wakeup, in which case the return value of _S0W has to be used as the
> +	 * lowest power state available to the device.
>  	 */
>  	d_min = ACPI_STATE_D0;
>  	d_max = ACPI_STATE_D3_COLD;
> @@ -449,12 +442,30 @@ static int acpi_dev_pm_get_state(struct
>  	 * If present, _SxD methods return the minimum D-state (highest power
>  	 * state) we can use for the corresponding S-states.  Otherwise, the
>  	 * minimum D-state is D0 (ACPI 3.x).
> -	 *
> -	 * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
> -	 * provided -- that's our fault recovery, we ignore retval.
>  	 */
>  	if (target_state > ACPI_STATE_S0) {
> -		acpi_evaluate_integer(adev->handle, acpi_method, NULL, &d_min);
> +		/*
> +		 * We rely on acpi_evaluate_integer() not clobbering the integer
> +		 * provided if AE_NOT_FOUND is returned.
> +		 */
> +		ret = d_min;
> +		status = acpi_evaluate_integer(handle, method, NULL, &ret);
> +		if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> +		    || ret > ACPI_STATE_D3_COLD)
> +			return -ENODATA;
> +
> +		/*
> +		 * We need to handle legacy systems where D3hot and D3cold are
> +		 * the same and 3 is returned in both cases, so fall back to
> +		 * D3cold if D3hot is not a valid state.
> +		 */
> +		if (!adev->power.states[ret].flags.valid) {
> +			if (ret == ACPI_STATE_D3_HOT)
> +				ret = ACPI_STATE_D3_COLD;
> +			else
> +				return -ENODATA;
> +		}
> +		d_min = ret;
>  		wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
>  			&& adev->wakeup.sleep_state >= target_state;
>  	} else if (dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP) !=
> @@ -470,36 +481,29 @@ static int acpi_dev_pm_get_state(struct
>  	 * can wake the system.  _S0W may be valid, too.
>  	 */
>  	if (wakeup) {
> -		acpi_status status;
> -
> -		acpi_method[3] = 'W';
> -		status = acpi_evaluate_integer(adev->handle, acpi_method, NULL,
> -						&d_max);
> -		if (ACPI_FAILURE(status)) {
> -			if (target_state != ACPI_STATE_S0 ||
> -			    status != AE_NOT_FOUND)
> +		method[3] = 'W';
> +		status = acpi_evaluate_integer(handle, method, NULL, &ret);
> +		if (status == AE_NOT_FOUND) {
> +			if (target_state > ACPI_STATE_S0)
>  				d_max = d_min;
> -		} else if (d_max < d_min) {
> -			/* Warn the user of the broken DSDT */
> -			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
> -				acpi_method);
> -			/* Sanitize it */
> -			d_min = d_max;
> +		} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
> +			/* Fall back to D3cold if ret is not a valid state. */
> +			if (!adev->power.states[ret].flags.valid)
> +				ret = ACPI_STATE_D3_COLD;
> +
> +			d_max = ret > d_min ? ret : d_min;
> +		} else {
> +			return -ENODATA;
>  		}
>  	}
>  
> -	if (d_max_in < d_min)
> -		return -EINVAL;
>  	if (d_min_p)
>  		*d_min_p = d_min;
> -	/* constrain d_max with specified lowest limit (max number) */
> -	if (d_max > d_max_in) {
> -		for (d_max = d_max_in; d_max > d_min; d_max--) {
> -			if (adev->power.states[d_max].flags.valid)
> -				break;
> -		}
> -	}
> -	return d_max;
> +
> +	if (d_max_p)
> +		*d_max_p = d_max;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -508,7 +512,8 @@ static int acpi_dev_pm_get_state(struct
>   * @d_min_p: Location to store the upper limit of the allowed states range.
>   * @d_max_in: Deepest low-power state to take into consideration.
>   * Return value: Preferred power state of the device on success, -ENODEV
> - * (if there's no 'struct acpi_device' for @dev) or -EINVAL on failure
> + * if there's no 'struct acpi_device' for @dev, -EINVAL if @d_max_in is
> + * incorrect, or -ENODATA on ACPI method failure.
>   *
>   * The caller must ensure that @dev is valid before using this function.
>   */
> @@ -516,14 +521,39 @@ int acpi_pm_device_sleep_state(struct de
>  {
>  	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
>  	struct acpi_device *adev;
> +	int ret, d_max;
> +
> +	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
> +		return -EINVAL;
> +
> +	if (d_max_in > ACPI_STATE_D3_HOT) {
> +		enum pm_qos_flags_status stat;
> +
> +		stat = dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF);
> +		if (stat == PM_QOS_FLAGS_ALL)
> +			d_max_in = ACPI_STATE_D3_HOT;
> +	}
>  
>  	if (!handle || acpi_bus_get_device(handle, &adev)) {
>  		dev_dbg(dev, "ACPI handle without context in %s!\n", __func__);
>  		return -ENODEV;
>  	}
>  
> -	return acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
> -				     d_max_in, d_min_p);
> +	ret = acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
> +				    d_min_p, &d_max);
> +	if (ret)
> +		return ret;
> +
> +	if (d_max_in < *d_min_p)
> +		return -EINVAL;

d_min_p can be NULL here.

Call trace:
acpi_pm_device_sleep_state, where d_min_p is passed as NULL
acpi_pci_choose_state
pci_choose_state
atl1e_suspend
atl1e_shutdown
pci_device_shutdown
device_shutdown

Thanks,
Aaron

> +
> +	if (d_max > d_max_in) {
> +		for (d_max = d_max_in; d_max > *d_min_p; d_max--) {
> +			if (adev->power.states[d_max].flags.valid)
> +				break;
> +		}
> +	}
> +	return d_max;
>  }
>  EXPORT_SYMBOL(acpi_pm_device_sleep_state);
>  
> @@ -674,17 +704,13 @@ struct acpi_device *acpi_dev_pm_get_node
>  static int acpi_dev_pm_low_power(struct device *dev, struct acpi_device *adev,
>  				 u32 system_state)
>  {
> -	int power_state;
> +	int ret, state;
>  
>  	if (!acpi_device_power_manageable(adev))
>  		return 0;
>  
> -	power_state = acpi_dev_pm_get_state(dev, adev, system_state,
> -					    ACPI_STATE_D3_COLD, NULL);
> -	if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3_COLD)
> -		return -EIO;
> -
> -	return acpi_device_set_power(adev, power_state);
> +	ret = acpi_dev_pm_get_state(dev, adev, system_state, NULL, &state);
> +	return ret ? ret : acpi_device_set_power(adev, state);
>  }
>  
>  /**
> 

Re: [PATCH 3/4] ACPI / PM: Rework and clean up acpi_dev_pm_get_state()

[Rafael J. Wysocki]

On Thursday, June 27, 2013 04:11:22 PM Aaron Lu wrote:
> On 06/14/2013 08:32 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > The acpi_dev_pm_get_state() function defined in device_pm.c is quite
> > convoluted, which isn't really necessary, and it doesn't validate the
> > values returned by the ACPI methods executed by it appropriately.
> > 
> > To address these shortcomings modify it in the following way.
> > 
> >  (1) Make its return value only mean whether or not it succeeded and
> >      pass the device power states determined by it through pointers.
> > 
> >  (2) Drop the d_max_in argument, used by only one of its callers,
> >      from it, and move the code related to d_max_in into that caller,
> >      acpi_pm_device_sleep_state().
> > 
> >  (3) Make it always check the return value of acpi_evaluate_integer()
> >      and handle failures as appropriate.  Moreover, make it check if
> >      the values returned by the executed ACPI methods are not out of
> >      range.
> > 
> >  (4) Make it check if the values returned by the executed ACPI
> >      methods represent valid power states of the given device and
> >      handle situations in which that's not the case gracefully.
> > 
> > Also update the kerneldoc comments of acpi_dev_pm_get_state() and
> > acpi_pm_device_sleep_state() to reflect the code changes.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/device_pm.c |  158 +++++++++++++++++++++++++++--------------------
> >  1 file changed, 92 insertions(+), 66 deletions(-)
> > 
> > Index: linux-pm/drivers/acpi/device_pm.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/device_pm.c
> > +++ linux-pm/drivers/acpi/device_pm.c
> > @@ -403,44 +403,37 @@ EXPORT_SYMBOL(acpi_bus_can_wakeup);
> >   * @dev: Device whose preferred target power state to return.
> >   * @adev: ACPI device node corresponding to @dev.
> >   * @target_state: System state to match the resultant device state.
> > - * @d_max_in: Deepest low-power state to take into consideration.
> > - * @d_min_p: Location to store the upper limit of the allowed states range.
> > - * Return value: Preferred power state of the device on success, -ENODEV
> > - * (if there's no 'struct acpi_device' for @dev) or -EINVAL on failure
> > + * @d_min_p: Location to store the highest power state available to the device.
> > + * @d_max_p: Location to store the lowest power state available to the device.
> >   *
> > - * Find the lowest power (highest number) ACPI device power state that the
> > - * device can be in while the system is in the state represented by
> > - * @target_state.  If @d_min_p is set, the highest power (lowest number) device
> > - * power state that @dev can be in for the given system sleep state is stored
> > - * at the location pointed to by it.
> > + * Find the lowest power (highest number) and highest power (lowest number) ACPI
> > + * device power states that the device can be in while the system is in the
> > + * state represented by @target_state.  Store the integer numbers representing
> > + * those stats in the memory locations pointed to by @d_max_p and @d_min_p,
> > + * respectively.
> >   *
> >   * Callers must ensure that @dev and @adev are valid pointers and that @adev
> >   * actually corresponds to @dev before using this function.
> > + *
> > + * Returns 0 on success or -ENODATA when one of the ACPI methods fails or
> > + * returns a value that doesn't make sense.  The memory locations pointed to by
> > + * @d_max_p and @d_min_p are only modified on success.
> >   */
> >  static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
> > -				 u32 target_state, int d_max_in, int *d_min_p)
> > +				 u32 target_state, int *d_min_p, int *d_max_p)
> >  {
> > -	char acpi_method[] = "_SxD";
> > -	unsigned long long d_min, d_max;
> > +	char method[] = { '_', 'S', '0' + target_state, 'D', '\0' };
> > +	acpi_handle handle = adev->handle;
> > +	unsigned long long ret;
> > +	int d_min, d_max;
> >  	bool wakeup = false;
> > +	acpi_status status;
> >  
> > -	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
> > -		return -EINVAL;
> > -
> > -	if (d_max_in > ACPI_STATE_D3_HOT) {
> > -		enum pm_qos_flags_status stat;
> > -
> > -		stat = dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF);
> > -		if (stat == PM_QOS_FLAGS_ALL)
> > -			d_max_in = ACPI_STATE_D3_HOT;
> > -	}
> > -
> > -	acpi_method[2] = '0' + target_state;
> >  	/*
> > -	 * If the sleep state is S0, the lowest limit from ACPI is D3,
> > -	 * but if the device has _S0W, we will use the value from _S0W
> > -	 * as the lowest limit from ACPI.  Finally, we will constrain
> > -	 * the lowest limit with the specified one.
> > +	 * If the system state is S0, the lowest power state the device can be
> > +	 * in is D3cold, unless the device has _S0W and is supposed to signal
> > +	 * wakeup, in which case the return value of _S0W has to be used as the
> > +	 * lowest power state available to the device.
> >  	 */
> >  	d_min = ACPI_STATE_D0;
> >  	d_max = ACPI_STATE_D3_COLD;
> > @@ -449,12 +442,30 @@ static int acpi_dev_pm_get_state(struct
> >  	 * If present, _SxD methods return the minimum D-state (highest power
> >  	 * state) we can use for the corresponding S-states.  Otherwise, the
> >  	 * minimum D-state is D0 (ACPI 3.x).
> > -	 *
> > -	 * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
> > -	 * provided -- that's our fault recovery, we ignore retval.
> >  	 */
> >  	if (target_state > ACPI_STATE_S0) {
> > -		acpi_evaluate_integer(adev->handle, acpi_method, NULL, &d_min);
> > +		/*
> > +		 * We rely on acpi_evaluate_integer() not clobbering the integer
> > +		 * provided if AE_NOT_FOUND is returned.
> > +		 */
> > +		ret = d_min;
> > +		status = acpi_evaluate_integer(handle, method, NULL, &ret);
> > +		if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> > +		    || ret > ACPI_STATE_D3_COLD)
> > +			return -ENODATA;
> > +
> > +		/*
> > +		 * We need to handle legacy systems where D3hot and D3cold are
> > +		 * the same and 3 is returned in both cases, so fall back to
> > +		 * D3cold if D3hot is not a valid state.
> > +		 */
> > +		if (!adev->power.states[ret].flags.valid) {
> > +			if (ret == ACPI_STATE_D3_HOT)
> > +				ret = ACPI_STATE_D3_COLD;
> > +			else
> > +				return -ENODATA;
> > +		}
> > +		d_min = ret;
> >  		wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
> >  			&& adev->wakeup.sleep_state >= target_state;
> >  	} else if (dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP) !=
> > @@ -470,36 +481,29 @@ static int acpi_dev_pm_get_state(struct
> >  	 * can wake the system.  _S0W may be valid, too.
> >  	 */
> >  	if (wakeup) {
> > -		acpi_status status;
> > -
> > -		acpi_method[3] = 'W';
> > -		status = acpi_evaluate_integer(adev->handle, acpi_method, NULL,
> > -						&d_max);
> > -		if (ACPI_FAILURE(status)) {
> > -			if (target_state != ACPI_STATE_S0 ||
> > -			    status != AE_NOT_FOUND)
> > +		method[3] = 'W';
> > +		status = acpi_evaluate_integer(handle, method, NULL, &ret);
> > +		if (status == AE_NOT_FOUND) {
> > +			if (target_state > ACPI_STATE_S0)
> >  				d_max = d_min;
> > -		} else if (d_max < d_min) {
> > -			/* Warn the user of the broken DSDT */
> > -			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
> > -				acpi_method);
> > -			/* Sanitize it */
> > -			d_min = d_max;
> > +		} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
> > +			/* Fall back to D3cold if ret is not a valid state. */
> > +			if (!adev->power.states[ret].flags.valid)
> > +				ret = ACPI_STATE_D3_COLD;
> > +
> > +			d_max = ret > d_min ? ret : d_min;
> > +		} else {
> > +			return -ENODATA;
> >  		}
> >  	}
> >  
> > -	if (d_max_in < d_min)
> > -		return -EINVAL;
> >  	if (d_min_p)
> >  		*d_min_p = d_min;
> > -	/* constrain d_max with specified lowest limit (max number) */
> > -	if (d_max > d_max_in) {
> > -		for (d_max = d_max_in; d_max > d_min; d_max--) {
> > -			if (adev->power.states[d_max].flags.valid)
> > -				break;
> > -		}
> > -	}
> > -	return d_max;
> > +
> > +	if (d_max_p)
> > +		*d_max_p = d_max;
> > +
> > +	return 0;
> >  }
> >  
> >  /**
> > @@ -508,7 +512,8 @@ static int acpi_dev_pm_get_state(struct
> >   * @d_min_p: Location to store the upper limit of the allowed states range.
> >   * @d_max_in: Deepest low-power state to take into consideration.
> >   * Return value: Preferred power state of the device on success, -ENODEV
> > - * (if there's no 'struct acpi_device' for @dev) or -EINVAL on failure
> > + * if there's no 'struct acpi_device' for @dev, -EINVAL if @d_max_in is
> > + * incorrect, or -ENODATA on ACPI method failure.
> >   *
> >   * The caller must ensure that @dev is valid before using this function.
> >   */
> > @@ -516,14 +521,39 @@ int acpi_pm_device_sleep_state(struct de
> >  {
> >  	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
> >  	struct acpi_device *adev;
> > +	int ret, d_max;
> > +
> > +	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
> > +		return -EINVAL;
> > +
> > +	if (d_max_in > ACPI_STATE_D3_HOT) {
> > +		enum pm_qos_flags_status stat;
> > +
> > +		stat = dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF);
> > +		if (stat == PM_QOS_FLAGS_ALL)
> > +			d_max_in = ACPI_STATE_D3_HOT;
> > +	}
> >  
> >  	if (!handle || acpi_bus_get_device(handle, &adev)) {
> >  		dev_dbg(dev, "ACPI handle without context in %s!\n", __func__);
> >  		return -ENODEV;
> >  	}
> >  
> > -	return acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
> > -				     d_max_in, d_min_p);
> > +	ret = acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
> > +				    d_min_p, &d_max);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (d_max_in < *d_min_p)
> > +		return -EINVAL;
> 
> d_min_p can be NULL here.
> 
> Call trace:
> acpi_pm_device_sleep_state, where d_min_p is passed as NULL
> acpi_pci_choose_state
> pci_choose_state
> atl1e_suspend
> atl1e_shutdown
> pci_device_shutdown
> device_shutdown

Right, the patch below should fix that.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / PM: Fix possible NULL pointer deref in acpi_pm_device_sleep_state()

After commit fa1675b (ACPI / PM: Rework and clean up
acpi_dev_pm_get_state()) a NULL pointer dereference will take place
if NULL is passed to acpi_pm_device_sleep_state() as the second
argument.

Fix that by avoiding to use the pointer that may be NULL until
it's necessary to store a return value at the location pointed to
by it (if not NULL).

Reported-by: Aaron Lu <aaron.lu@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -541,7 +541,7 @@ int acpi_pm_device_sleep_state(struct de
 {
 	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
 	struct acpi_device *adev;
-	int ret, d_max;
+	int ret, d_min, d_max;
 
 	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
 		return -EINVAL;
@@ -560,19 +560,23 @@ int acpi_pm_device_sleep_state(struct de
 	}
 
 	ret = acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
-				    d_min_p, &d_max);
+				    &d_min, &d_max);
 	if (ret)
 		return ret;
 
-	if (d_max_in < *d_min_p)
+	if (d_max_in < d_min)
 		return -EINVAL;
 
 	if (d_max > d_max_in) {
-		for (d_max = d_max_in; d_max > *d_min_p; d_max--) {
+		for (d_max = d_max_in; d_max > d_min; d_max--) {
 			if (adev->power.states[d_max].flags.valid)
 				break;
 		}
 	}
+
+	if (d_min_p)
+		*d_min_p = d_min;
+
 	return d_max;
 }
 EXPORT_SYMBOL(acpi_pm_device_sleep_state);


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/4] ACPI / PM: Rework and clean up acpi_dev_pm_get_state()

[Aaron Lu]

On 06/27/2013 08:09 PM, Rafael J. Wysocki wrote:
> On Thursday, June 27, 2013 04:11:22 PM Aaron Lu wrote:
>> On 06/14/2013 08:32 PM, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> The acpi_dev_pm_get_state() function defined in device_pm.c is quite
>>> convoluted, which isn't really necessary, and it doesn't validate the
>>> values returned by the ACPI methods executed by it appropriately.
>>>
>>> To address these shortcomings modify it in the following way.
>>>
>>>  (1) Make its return value only mean whether or not it succeeded and
>>>      pass the device power states determined by it through pointers.
>>>
>>>  (2) Drop the d_max_in argument, used by only one of its callers,
>>>      from it, and move the code related to d_max_in into that caller,
>>>      acpi_pm_device_sleep_state().
>>>
>>>  (3) Make it always check the return value of acpi_evaluate_integer()
>>>      and handle failures as appropriate.  Moreover, make it check if
>>>      the values returned by the executed ACPI methods are not out of
>>>      range.
>>>
>>>  (4) Make it check if the values returned by the executed ACPI
>>>      methods represent valid power states of the given device and
>>>      handle situations in which that's not the case gracefully.
>>>
>>> Also update the kerneldoc comments of acpi_dev_pm_get_state() and
>>> acpi_pm_device_sleep_state() to reflect the code changes.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>  drivers/acpi/device_pm.c |  158 +++++++++++++++++++++++++++--------------------
>>>  1 file changed, 92 insertions(+), 66 deletions(-)
>>>
>>> Index: linux-pm/drivers/acpi/device_pm.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/acpi/device_pm.c
>>> +++ linux-pm/drivers/acpi/device_pm.c
>>> @@ -403,44 +403,37 @@ EXPORT_SYMBOL(acpi_bus_can_wakeup);
>>>   * @dev: Device whose preferred target power state to return.
>>>   * @adev: ACPI device node corresponding to @dev.
>>>   * @target_state: System state to match the resultant device state.
>>> - * @d_max_in: Deepest low-power state to take into consideration.
>>> - * @d_min_p: Location to store the upper limit of the allowed states range.
>>> - * Return value: Preferred power state of the device on success, -ENODEV
>>> - * (if there's no 'struct acpi_device' for @dev) or -EINVAL on failure
>>> + * @d_min_p: Location to store the highest power state available to the device.
>>> + * @d_max_p: Location to store the lowest power state available to the device.
>>>   *
>>> - * Find the lowest power (highest number) ACPI device power state that the
>>> - * device can be in while the system is in the state represented by
>>> - * @target_state.  If @d_min_p is set, the highest power (lowest number) device
>>> - * power state that @dev can be in for the given system sleep state is stored
>>> - * at the location pointed to by it.
>>> + * Find the lowest power (highest number) and highest power (lowest number) ACPI
>>> + * device power states that the device can be in while the system is in the
>>> + * state represented by @target_state.  Store the integer numbers representing
>>> + * those stats in the memory locations pointed to by @d_max_p and @d_min_p,
>>> + * respectively.
>>>   *
>>>   * Callers must ensure that @dev and @adev are valid pointers and that @adev
>>>   * actually corresponds to @dev before using this function.
>>> + *
>>> + * Returns 0 on success or -ENODATA when one of the ACPI methods fails or
>>> + * returns a value that doesn't make sense.  The memory locations pointed to by
>>> + * @d_max_p and @d_min_p are only modified on success.
>>>   */
>>>  static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
>>> -				 u32 target_state, int d_max_in, int *d_min_p)
>>> +				 u32 target_state, int *d_min_p, int *d_max_p)
>>>  {
>>> -	char acpi_method[] = "_SxD";
>>> -	unsigned long long d_min, d_max;
>>> +	char method[] = { '_', 'S', '0' + target_state, 'D', '\0' };
>>> +	acpi_handle handle = adev->handle;
>>> +	unsigned long long ret;
>>> +	int d_min, d_max;
>>>  	bool wakeup = false;
>>> +	acpi_status status;
>>>  
>>> -	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
>>> -		return -EINVAL;
>>> -
>>> -	if (d_max_in > ACPI_STATE_D3_HOT) {
>>> -		enum pm_qos_flags_status stat;
>>> -
>>> -		stat = dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF);
>>> -		if (stat == PM_QOS_FLAGS_ALL)
>>> -			d_max_in = ACPI_STATE_D3_HOT;
>>> -	}
>>> -
>>> -	acpi_method[2] = '0' + target_state;
>>>  	/*
>>> -	 * If the sleep state is S0, the lowest limit from ACPI is D3,
>>> -	 * but if the device has _S0W, we will use the value from _S0W
>>> -	 * as the lowest limit from ACPI.  Finally, we will constrain
>>> -	 * the lowest limit with the specified one.
>>> +	 * If the system state is S0, the lowest power state the device can be
>>> +	 * in is D3cold, unless the device has _S0W and is supposed to signal
>>> +	 * wakeup, in which case the return value of _S0W has to be used as the
>>> +	 * lowest power state available to the device.
>>>  	 */
>>>  	d_min = ACPI_STATE_D0;
>>>  	d_max = ACPI_STATE_D3_COLD;
>>> @@ -449,12 +442,30 @@ static int acpi_dev_pm_get_state(struct
>>>  	 * If present, _SxD methods return the minimum D-state (highest power
>>>  	 * state) we can use for the corresponding S-states.  Otherwise, the
>>>  	 * minimum D-state is D0 (ACPI 3.x).
>>> -	 *
>>> -	 * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
>>> -	 * provided -- that's our fault recovery, we ignore retval.
>>>  	 */
>>>  	if (target_state > ACPI_STATE_S0) {
>>> -		acpi_evaluate_integer(adev->handle, acpi_method, NULL, &d_min);
>>> +		/*
>>> +		 * We rely on acpi_evaluate_integer() not clobbering the integer
>>> +		 * provided if AE_NOT_FOUND is returned.
>>> +		 */
>>> +		ret = d_min;
>>> +		status = acpi_evaluate_integer(handle, method, NULL, &ret);
>>> +		if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
>>> +		    || ret > ACPI_STATE_D3_COLD)
>>> +			return -ENODATA;
>>> +
>>> +		/*
>>> +		 * We need to handle legacy systems where D3hot and D3cold are
>>> +		 * the same and 3 is returned in both cases, so fall back to
>>> +		 * D3cold if D3hot is not a valid state.
>>> +		 */
>>> +		if (!adev->power.states[ret].flags.valid) {
>>> +			if (ret == ACPI_STATE_D3_HOT)
>>> +				ret = ACPI_STATE_D3_COLD;
>>> +			else
>>> +				return -ENODATA;
>>> +		}
>>> +		d_min = ret;
>>>  		wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
>>>  			&& adev->wakeup.sleep_state >= target_state;
>>>  	} else if (dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP) !=
>>> @@ -470,36 +481,29 @@ static int acpi_dev_pm_get_state(struct
>>>  	 * can wake the system.  _S0W may be valid, too.
>>>  	 */
>>>  	if (wakeup) {
>>> -		acpi_status status;
>>> -
>>> -		acpi_method[3] = 'W';
>>> -		status = acpi_evaluate_integer(adev->handle, acpi_method, NULL,
>>> -						&d_max);
>>> -		if (ACPI_FAILURE(status)) {
>>> -			if (target_state != ACPI_STATE_S0 ||
>>> -			    status != AE_NOT_FOUND)
>>> +		method[3] = 'W';
>>> +		status = acpi_evaluate_integer(handle, method, NULL, &ret);
>>> +		if (status == AE_NOT_FOUND) {
>>> +			if (target_state > ACPI_STATE_S0)
>>>  				d_max = d_min;
>>> -		} else if (d_max < d_min) {
>>> -			/* Warn the user of the broken DSDT */
>>> -			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
>>> -				acpi_method);
>>> -			/* Sanitize it */
>>> -			d_min = d_max;
>>> +		} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
>>> +			/* Fall back to D3cold if ret is not a valid state. */
>>> +			if (!adev->power.states[ret].flags.valid)
>>> +				ret = ACPI_STATE_D3_COLD;
>>> +
>>> +			d_max = ret > d_min ? ret : d_min;
>>> +		} else {
>>> +			return -ENODATA;
>>>  		}
>>>  	}
>>>  
>>> -	if (d_max_in < d_min)
>>> -		return -EINVAL;
>>>  	if (d_min_p)
>>>  		*d_min_p = d_min;
>>> -	/* constrain d_max with specified lowest limit (max number) */
>>> -	if (d_max > d_max_in) {
>>> -		for (d_max = d_max_in; d_max > d_min; d_max--) {
>>> -			if (adev->power.states[d_max].flags.valid)
>>> -				break;
>>> -		}
>>> -	}
>>> -	return d_max;
>>> +
>>> +	if (d_max_p)
>>> +		*d_max_p = d_max;
>>> +
>>> +	return 0;
>>>  }
>>>  
>>>  /**
>>> @@ -508,7 +512,8 @@ static int acpi_dev_pm_get_state(struct
>>>   * @d_min_p: Location to store the upper limit of the allowed states range.
>>>   * @d_max_in: Deepest low-power state to take into consideration.
>>>   * Return value: Preferred power state of the device on success, -ENODEV
>>> - * (if there's no 'struct acpi_device' for @dev) or -EINVAL on failure
>>> + * if there's no 'struct acpi_device' for @dev, -EINVAL if @d_max_in is
>>> + * incorrect, or -ENODATA on ACPI method failure.
>>>   *
>>>   * The caller must ensure that @dev is valid before using this function.
>>>   */
>>> @@ -516,14 +521,39 @@ int acpi_pm_device_sleep_state(struct de
>>>  {
>>>  	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
>>>  	struct acpi_device *adev;
>>> +	int ret, d_max;
>>> +
>>> +	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
>>> +		return -EINVAL;
>>> +
>>> +	if (d_max_in > ACPI_STATE_D3_HOT) {
>>> +		enum pm_qos_flags_status stat;
>>> +
>>> +		stat = dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF);
>>> +		if (stat == PM_QOS_FLAGS_ALL)
>>> +			d_max_in = ACPI_STATE_D3_HOT;
>>> +	}
>>>  
>>>  	if (!handle || acpi_bus_get_device(handle, &adev)) {
>>>  		dev_dbg(dev, "ACPI handle without context in %s!\n", __func__);
>>>  		return -ENODEV;
>>>  	}
>>>  
>>> -	return acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
>>> -				     d_max_in, d_min_p);
>>> +	ret = acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
>>> +				    d_min_p, &d_max);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (d_max_in < *d_min_p)
>>> +		return -EINVAL;
>>
>> d_min_p can be NULL here.
>>
>> Call trace:
>> acpi_pm_device_sleep_state, where d_min_p is passed as NULL
>> acpi_pci_choose_state
>> pci_choose_state
>> atl1e_suspend
>> atl1e_shutdown
>> pci_device_shutdown
>> device_shutdown
> 
> Right, the patch below should fix that.

Confirmed, no more oops.
Feel free to add my Tested-by.

Thanks,
Aaron

> 
> Thanks,
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: ACPI / PM: Fix possible NULL pointer deref in acpi_pm_device_sleep_state()
> 
> After commit fa1675b (ACPI / PM: Rework and clean up
> acpi_dev_pm_get_state()) a NULL pointer dereference will take place
> if NULL is passed to acpi_pm_device_sleep_state() as the second
> argument.
> 
> Fix that by avoiding to use the pointer that may be NULL until
> it's necessary to store a return value at the location pointed to
> by it (if not NULL).
> 
> Reported-by: Aaron Lu <aaron.lu@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/device_pm.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -541,7 +541,7 @@ int acpi_pm_device_sleep_state(struct de
>  {
>  	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
>  	struct acpi_device *adev;
> -	int ret, d_max;
> +	int ret, d_min, d_max;
>  
>  	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3_COLD)
>  		return -EINVAL;
> @@ -560,19 +560,23 @@ int acpi_pm_device_sleep_state(struct de
>  	}
>  
>  	ret = acpi_dev_pm_get_state(dev, adev, acpi_target_system_state(),
> -				    d_min_p, &d_max);
> +				    &d_min, &d_max);
>  	if (ret)
>  		return ret;
>  
> -	if (d_max_in < *d_min_p)
> +	if (d_max_in < d_min)
>  		return -EINVAL;
>  
>  	if (d_max > d_max_in) {
> -		for (d_max = d_max_in; d_max > *d_min_p; d_max--) {
> +		for (d_max = d_max_in; d_max > d_min; d_max--) {
>  			if (adev->power.states[d_max].flags.valid)
>  				break;
>  		}
>  	}
> +
> +	if (d_min_p)
> +		*d_min_p = d_min;
> +
>  	return d_max;
>  }
>  EXPORT_SYMBOL(acpi_pm_device_sleep_state);
> 
>