Date:   Fri, 3 Jun 2022 18:55:12 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Vaibhav Gupta <vaibhavgupta40@...il.com>
Subject: Re: [PATCH net] net: stmmac: Fix WoL for PCI-based setups

[+cc Rafael, Vaibhav for wakeup usage questions]

On Tue, Jul 31, 2018 at 03:08:20PM +0100, Jose Abreu wrote:
> WoL won't work in PCI-based setups because we are not saving the PCI EP
> state before entering suspend state and not allowing D3 wake.
> 
> Fix this by using a wrapper around stmmac_{suspend/resume} which
> correctly sets the PCI EP state.
> 
> Signed-off-by: Jose Abreu <joabreu@...opsys.com>
> Cc: David S. Miller <davem@...emloft.net>
> Cc: Joao Pinto <jpinto@...opsys.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@...com>
> Cc: Alexandre Torgue <alexandre.torgue@...com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 40 ++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> index 8d375e51a526..6a393b16a1fc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> @@ -257,7 +257,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
>  		return -ENOMEM;
>  
>  	/* Enable pci device */
> -	ret = pcim_enable_device(pdev);
> +	ret = pci_enable_device(pdev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n",
>  			__func__);
> @@ -300,9 +300,45 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
>  static void stmmac_pci_remove(struct pci_dev *pdev)
>  {
>  	stmmac_dvr_remove(&pdev->dev);
> +	pci_disable_device(pdev);
>  }
>  
> -static SIMPLE_DEV_PM_OPS(stmmac_pm_ops, stmmac_suspend, stmmac_resume);
> +static int stmmac_pci_suspend(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int ret;
> +
> +	ret = stmmac_suspend(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_save_state(pdev);
> +	if (ret)
> +		return ret;
> +
> +	pci_disable_device(pdev);
> +	pci_wake_from_d3(pdev, true);

I have a question about this.  This driver uses generic power
management (thank you for that!), and in that case the PCI core should
take care of all these PCI details for you, e.g.,

  suspend_devices_and_enter
    dpm_suspend_start(PMSG_SUSPEND)
      pci_pm_suspend                       # PCI bus .suspend() method
        stmmac_suspend                     # driver->pm->suspend
          netif_device_detach              <-- device-specific
          ...                              <-- device-specific
    suspend_enter
      dpm_suspend_noirq(PMSG_SUSPEND)
        pci_pm_suspend_noirq               # PCI bus .suspend_noirq() method
          pci_save_state                   <-- generic PCI
          pci_prepare_to_sleep             <-- generic PCI
            pci_enable_wake(true)
            pci_set_power_state
    ...
    dpm_resume_end(PMSG_RESUME)
      pci_pm_resume                        # PCI bus .resume() method
        pci_restore_standard_config
          pci_set_power_state(PCI_D0)      <-- generic PCI
          pci_restore_state                <-- generic PCI
        pci_pm_default_resume
          pci_enable_wake(false)
        stmmac_resume                      # driver->pm->resume
          ...                              <-- device-specific
          netif_device_attach              <-- device-specific

So why did WoL not work before this patch?  IIUC there are two
important pieces to the generic wakeup framework:

  device_set_wakeup_capable(true):  device physically CAN signal wakeup
  device_set_wakeup_enable(true):   device is ALLOWED to signal wakeup

The PCI core calls device_set_wakeup_capable(true) in pci_pm_init() if
the device advertises support for PME, so that part was probably fine.

But I don't think WoL would *work* unless somebody called
device_set_wakeup_enable(true), which the PCI core doesn't do.
stmmac_set_wol() does, but I think that's only exercised by ethtool.

There are several drivers, e.g., e1000, igb, ixgbe, that call
device_set_wakeup_enable() from their .probe() methods, and I suspect
that if you did the same, you wouldn't need this patch.

I'm not 100% sure that's the right approach though because of these
notes from devices.rst, which seem a little bit contradictory:

  These fields are initialized by bus or device driver code using
  device_set_wakeup_capable() and device_set_wakeup_enable(), defined
  in include/linux/pm_wakeup.h. [1]
  ...
  Device drivers, however, are not expected to call
  device_set_wakeup_enable() directly in any case. [2]

If drivers aren't expected to call device_set_wakeup_enable(), I guess
WoL would never be enabled except by user-space doing something?  If
that's the intent, why do all these drivers enable wakeup themselves?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/pm/devices.rst?id=v5.18#n141
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/pm/devices.rst?id=v5.18#n181

> +	return 0;
> +}
> +
> +static int stmmac_pci_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int ret;
> +
> +	pci_restore_state(pdev);
> +	pci_set_power_state(pdev, PCI_D0);
> +
> +	ret = pci_enable_device(pdev);
> +	if (ret)
> +		return ret;
> +
> +	pci_set_master(pdev);
> +
> +	return stmmac_resume(dev);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(stmmac_pm_ops, stmmac_pci_suspend, stmmac_pci_resume);
>  
>  /* synthetic ID, no official vendor */
>  #define PCI_VENDOR_ID_STMMAC 0x700
> -- 
> 2.7.4

Powered by blists - more mailing lists