* Re: [PATCH net] net: stmmac: Fix WoL for PCI-based setups
2018-07-31 14:08 [PATCH net] net: stmmac: Fix WoL for PCI-based setups Jose Abreu
2018-08-01 16:36 ` David Miller
@ 2022-06-03 23:55 ` Bjorn Helgaas
1 sibling, 0 replies; 3+ messages in thread
From: Bjorn Helgaas @ 2022-06-03 23:55 UTC (permalink / raw)
To: Jose Abreu
Cc: netdev, David S. Miller, Joao Pinto, Giuseppe Cavallaro,
Alexandre Torgue, Rafael J. Wysocki, Vaibhav Gupta
[+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@synopsys.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.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
^ permalink raw reply [flat|nested] 3+ messages in thread