Re: [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Thursday, 20 of March 2008, David Brownell wrote:
> Clean up pci_choose_state():
> 
>  - pci_choose_state() should only return PCI_D0, unless the system is
>    entering a suspend (or hibernate) system state.
> 
>  - Only use platform_pci_choose_state() when entering a suspend
>    state ... and avoid PCI_D1 and PCI_D2 when appropriate.
> 
>  - Corrrect kerneldoc.
> 
> Note that for now only ACPI provides platform_pci_choose_state(), so
> this could be a minor change in behavior on some non-PC systems:  it
> avoids D3 except in the final stage of hibernation.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/pci/pci.c |   47 ++++++++++++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> --- g26.orig/drivers/pci/pci.c	2008-02-24 00:18:16.000000000 -0800
> +++ g26/drivers/pci/pci.c	2008-02-24 00:41:18.000000000 -0800
> @@ -523,44 +523,49 @@ pci_set_power_state(struct pci_dev *dev,
>  }
>  
>  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
> - 
> +
>  /**
>   * pci_choose_state - Choose the power state of a PCI device
>   * @dev: PCI device to be suspended
> - * @state: target sleep state for the whole system. This is the value
> - *	that is passed to suspend() function.
> + * @mesg: value passed to suspend() function.
>   *
>   * Returns PCI power state suitable for given device and given system
> - * message.
> + * power state transition.
>   */
>  
> -pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
> +pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t mesg)
>  {
>  	pci_power_t ret;
>  
> +	/* PCI legacy PM? */
>  	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
>  		return PCI_D0;
>  
> -	if (platform_pci_choose_state) {
> -		ret = platform_pci_choose_state(dev, state);
> -		if (ret != PCI_POWER_ERROR)
> -			return ret;
> -	}
> -
> -	switch (state.event) {
> -	case PM_EVENT_ON:
> -		return PCI_D0;
> -	case PM_EVENT_FREEZE:
> -	case PM_EVENT_PRETHAW:
> -		/* REVISIT both freeze and pre-thaw "should" use D0 */
> +	switch (mesg.event) {
>  	case PM_EVENT_SUSPEND:
> +		/* NOTE:  platform_pci_choose_state() should only return
> +		 * states where wakeup won't work if
> +		 *   - !device_may_wakeup(&dev->dev), or
> +		 *   - dev can't wake from the target system state
> +		 */
> +		if (platform_pci_choose_state) {
> +			ret = platform_pci_choose_state(dev, mesg);
> +			if (ret == PCI_POWER_ERROR)
> +				ret = PCI_D3hot;
> +			else if ((ret == PCI_D1 || ret == PCI_D2)
> +					&& pci_no_d1d2(dev))
> +				ret = PCI_D3hot;
> +			break;
> +		}
> +		/* FALLTHROUGH ... D3hot works, but may be suboptimal */
>  	case PM_EVENT_HIBERNATE:
> -		return PCI_D3hot;
> +		ret = PCI_D3hot;

This is clearly wrong.  It should do the same as for suspend here (_S4D may be
defined and we should take it into account if it is).

> +		break;
>  	default:
> -		printk("Unrecognized suspend event %d\n", state.event);
> -		BUG();
> +		ret = PCI_D0;
> +		break;
>  	}
> -	return PCI_D0;
> +	return ret;
>  }
>  
>  EXPORT_SYMBOL(pci_choose_state);

I really don't think pci_choose_state() should take the state argument at all.

Thanks,
Rafael