Re: [PATCH] e1000e: Use PME poll to circumvent unreliable ACPI wake

[Kai-Heng Feng]

On Fri, Jun 2, 2023 at 4:24 AM Alexander H Duyck
<alexander.duyck@xxxxxxxxx> wrote:
>
> On Fri, 2023-06-02 at 00:25 +0800, Kai-Heng Feng wrote:
> > On some I219 devices, ethernet cable plugging detection only works once
> > from PCI D3 state. Subsequent cable plugging does set PME bit correctly,
> > but device still doesn't get woken up.
>
> Do we have a root cause on why things don't get woken up? This seems
> like an issue where something isn't getting reset after the first
> wakeup and so future ones are blocked.

No we don't know the root cause.
I guess the D3 wake isn't really tested under Windows because I219
doesn't use runtime D3 on Windows.

>
> > Since I219 connects to the root complex directly, it relies on platform
> > firmware (ACPI) to wake it up. In this case, the GPE from _PRW only
> > works for first cable plugging but fails to notify the driver for
> > subsequent plugging events.
> >
> > The issue was originally found on CNP, but the same issue can be found
> > on ADL too. So workaround the issue by continuing use PME poll after
> > first ACPI wake. As PME poll is always used, the runtime suspend
> > restriction for CNP can also be removed.
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > ---
> >  drivers/net/ethernet/intel/e1000e/netdev.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > index bd7ef59b1f2e..f0e48f2bc3a2 100644
> > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > @@ -7021,6 +7021,8 @@ static __maybe_unused int e1000e_pm_runtime_resume(struct device *dev)
> >       struct e1000_adapter *adapter = netdev_priv(netdev);
> >       int rc;
> >
> > +     pdev->pme_poll = true;
> > +
> >       rc = __e1000_resume(pdev);
> >       if (rc)
> >               return rc;
>
> Doesn't this enable this too broadly. I know there are a number of
> devices that run under the e1000e and I would imagine that we don't
> want them all running with "pme_poll = true" do we?

Whack a mole isn't scaling, either.
The generation between CNP and ADL are probably affected too.

>
> It seems like at a minimum we should only be setting this for specific
> platofrms or devices instead of on all of them.
>
> Also this seems like something we should be setting on the suspend side
> since it seems to be clared in the wakeup calls.

pme_poll gets cleared on wakeup, and once it's cleared the device will
be removed from pci_pme_list.

To prevent that, reset pme_poll to true immediately on runtime resume.

>
> Lastly I am not sure the first one is necessarily succeding. You might
> want to check the status of pme_poll before you run your first test.
> From what I can tell it looks like the initial state is true in
> pci_pm_init. If so it might be getting cleared after the first wakeup
> which is what causes your issues.

That's by design. pme_poll gets cleared when the hardware is capable
to signal wakeup via PME# or ACPI GPE. For detected hardwares, the
pme_poll will never be cleared.
So this becomes tricky for the issue, since the ACPI GPE works for
just one time, but never again.

>
> > @@ -7682,7 +7684,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >
> >       dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_SMART_PREPARE);
> >
> > -     if (pci_dev_run_wake(pdev) && hw->mac.type != e1000_pch_cnp)
> > +     if (pci_dev_run_wake(pdev))
> >               pm_runtime_put_noidle(&pdev->dev);
> >
> >       return 0;
>
> I assume this is the original workaround that was put in to address
> this issue. Perhaps you should add a Fixes tag to this to identify
> which workaround this patch is meant to be replacing.

Another possibility is to remove runtime power management completely.
I wonder why Windows keep the device at D0 all the time? Can Linux
align with Windows?

Kai-Heng