PCI: vmd: Allow VMD PM to use PCI core PM code

The pci_save_state call in vmd_suspend can be performed by
pci_pm_suspend_irq. This allows the call to pci_prepare_to_sleep into
ASPM flow.

The pci_restore_state call in vmd_resume was restoring state after
pci_pm_resume->pci_restore_standard_config had already restored state.
It's also been suspected that the config state should be restored before
re-requesting IRQs.

Remove the pci_{save,restore}_state calls in vmd_{suspend,resume} in
order to allow proper flow through PCI core power management ASPM code.

Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: You-Sheng Yang <vicamo.yang@canonical.com>
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 2 --
 1 file changed, 2 deletions(-)

On 2020-08-01 01:15, Jon Derrick wrote:
> The pci_save_state call in vmd_suspend can be performed by
> pci_pm_suspend_irq. This allows the call to pci_prepare_to_sleep into
> ASPM flow.
> 
> The pci_restore_state call in vmd_resume was restoring state after
> pci_pm_resume->pci_restore_standard_config had already restored state.
> It's also been suspected that the config state should be restored before
> re-requesting IRQs.
> 
> Remove the pci_{save,restore}_state calls in vmd_{suspend,resume} in
> order to allow proper flow through PCI core power management ASPM code.

I had a try on this patch but `lspci` still shows ASPM Disabled.
Anything prerequisite missing here?

You-Sheng Yang

[+cc Vaibhav, Rafael for suspend/resume question]

On Fri, Jul 31, 2020 at 01:15:44PM -0400, Jon Derrick wrote:
> The pci_save_state call in vmd_suspend can be performed by
> pci_pm_suspend_irq. This allows the call to pci_prepare_to_sleep into
> ASPM flow.

Add "()" after function names so they don't look like English words.

What is this "ASPM flow"?  The only ASPM-related code should be
configuration (enable/disable ASPM) (which happens at
enumeration-time, not suspend/resume time) and save/restore if we turn
the device off and we have to reconfigure it when we turn it on again.

> The pci_restore_state call in vmd_resume was restoring state after
> pci_pm_resume->pci_restore_standard_config had already restored state.
> It's also been suspected that the config state should be restored before
> re-requesting IRQs.
> 
> Remove the pci_{save,restore}_state calls in vmd_{suspend,resume} in
> order to allow proper flow through PCI core power management ASPM code.
> 
> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Cc: You-Sheng Yang <vicamo.yang@canonical.com>
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/controller/vmd.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 76d8acbee7d5..15c1d85d8780 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -719,7 +719,6 @@ static int vmd_suspend(struct device *dev)
>  	for (i = 0; i < vmd->msix_count; i++)
>  		devm_free_irq(dev, pci_irq_vector(pdev, i), &vmd->irqs[i]);
>  
> -	pci_save_state(pdev);

The VMD driver uses generic PM, not legacy PCI PM, so I think removing
the save/restore state from your suspend/resume functions is the right
thing to do.  You should only need to do VMD-specific things there.

I'm not even sure you need to free/request the IRQs in your
suspend/resume.  Maybe Rafael or Vaibhav know.

I just think the justification in the commit log is wrong.

>  	return 0;
>  }
>  
> @@ -737,7 +736,6 @@ static int vmd_resume(struct device *dev)
>  			return err;
>  	}
>  
> -	pci_restore_state(pdev);
>  	return 0;
>  }
>  #endif
> -- 
> 2.18.1
>

On Wed, 2020-08-05 at 15:54 +0800, You-Sheng Yang wrote:
> On 2020-08-01 01:15, Jon Derrick wrote:
> > The pci_save_state call in vmd_suspend can be performed by
> > pci_pm_suspend_irq. This allows the call to pci_prepare_to_sleep into
> > ASPM flow.
> > 
> > The pci_restore_state call in vmd_resume was restoring state after
> > pci_pm_resume->pci_restore_standard_config had already restored state.
> > It's also been suspected that the config state should be restored before
> > re-requesting IRQs.
> > 
> > Remove the pci_{save,restore}_state calls in vmd_{suspend,resume} in
> > order to allow proper flow through PCI core power management ASPM code.
> 
> I had a try on this patch but `lspci` still shows ASPM Disabled.
> Anything prerequisite missing here?
> 

Is enabling L0s/L1/etc on a device something that the driver should be
doing?

Does the state change with pcie_aspm.policy=powersave ?


> You-Sheng Yang
>

On Wed, Aug 05, 2020 at 03:30:00PM +0000, Derrick, Jonathan wrote:
> On Wed, 2020-08-05 at 15:54 +0800, You-Sheng Yang wrote:
> > On 2020-08-01 01:15, Jon Derrick wrote:
> > > The pci_save_state call in vmd_suspend can be performed by
> > > pci_pm_suspend_irq. This allows the call to pci_prepare_to_sleep into
> > > ASPM flow.
> > > 
> > > The pci_restore_state call in vmd_resume was restoring state after
> > > pci_pm_resume->pci_restore_standard_config had already restored state.
> > > It's also been suspected that the config state should be restored before
> > > re-requesting IRQs.
> > > 
> > > Remove the pci_{save,restore}_state calls in vmd_{suspend,resume} in
> > > order to allow proper flow through PCI core power management ASPM code.
> > 
> > I had a try on this patch but `lspci` still shows ASPM Disabled.
> > Anything prerequisite missing here?
> > 
> 
> Is enabling L0s/L1/etc on a device something that the driver should be
> doing?

No.  ASPM should be completely managed by the PCI core.  There are a
few drivers that *do* muck with ASPM, but they are broken and they
cause problems.

Drivers can use pci_disable_link_state() to completely disable ASPM
states, e.g., if they are known to be broken in hardware.  But they
should not update the Link Control register directly because there are
specific requirements that involve both ends of the link, not just the
endpoint.

Bjorn