Re: [PATCH] PCI: PM: Avoid resuming devices in D3hot during system suspend

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, May 31, 2019 at 11:49:30AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The current code resumes devices in D3hot during system suspend if
> the target power state for them is D3cold, but that is not necessary
> in general.  It only is necessary to do that if the platform firmware
> requires the device to be resumed, but that should be covered by
> the platform_pci_need_resume() check anyway, so rework
> pci_dev_keep_suspended() to avoid returning 'false' for devices
> in D3hot which need not be resumed due to platform firmware
> requirements.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/pci.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -2474,10 +2474,19 @@ bool pci_dev_keep_suspended(struct pci_d
>  {
>  	struct device *dev = &pci_dev->dev;
>  	bool wakeup = device_may_wakeup(dev);
> +	pci_power_t target_state;
>  
> -	if (!pm_runtime_suspended(dev)
> -	    || pci_target_state(pci_dev, wakeup) != pci_dev->current_state
> -	    || platform_pci_need_resume(pci_dev))
> +	if (!pm_runtime_suspended(dev) || platform_pci_need_resume(pci_dev))
> +		return false;
> +
> +	target_state = pci_target_state(pci_dev, wakeup);

Nit, add a blank line here.

> +	/*
> +	 * If the earlier platform check has not triggered, D3cold is just power
> +	 * removal on top of D3hot, so no need to resume the device in that
> +	 * case.
> +	 */
> +	if (target_state != pci_dev->current_state &&
> +	    target_state != PCI_D3cold && pci_dev->current_state != PCI_D3hot)
>  		return false;

This is more a comment on the existing code than on this particular
patch, but I find this whole function hard to understand, and I think
one reason is that there are a lot of negative conditions, both in
this function and in its callers.  This "target_state != ... &&
target_state != ...  && current_state != ..." is one example.  Another
is the function name itself.  It might be easier to read as something
like this:

  bool pci_dev_need_resume(...)
  {
    if (!pm_runtime_suspended(...))
      return true;

    if (platform_pci_need_resume(...))
      return true;

    if (target_state != current_state)
      return true;

    ...

Another reason I think it's hard to read is that
"pci_dev_keep_suspended" suggests that this is a pure boolean function
without side-effects, but in fact it also fiddles with the PME state
in some cases.  I don't have any ideas for that part.

Bjorn