Message-ID: <51CBF3AA.2030508@intel.com>
Date:	Thu, 27 Jun 2013 16:11:22 +0800
From:	Aaron Lu <aaron.lu@...el.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
CC:	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux PM list <linux-pm@...r.kernel.org>,
	Lan Tianyu <tianyu.lan@...el.com>
Subject: Re: [PATCH 3/4] ACPI / PM: Rework and clean up acpi_dev_pm_get_state()

On 06/14/2013 08:32 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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);
>  }
>  
>  /**
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists