Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230601162537.1163270-1-kai.heng.feng@canonical.com> <269262acfcce8eb1b85ee1fe3424a5ef2991f481.camel@gmail.com>
In-Reply-To: <269262acfcce8eb1b85ee1fe3424a5ef2991f481.camel@gmail.com>
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
Date:   Fri, 2 Jun 2023 09:46:45 +0800
Message-ID: <CAAd53p7c6eEqxd3jecfgvpxuYO3nmmmovcqD=3PgbqSVCWFfxA@mail.gmail.com>
Subject: Re: [PATCH] e1000e: Use PME poll to circumvent unreliable ACPI wake
To:     Alexander H Duyck <alexander.duyck@gmail.com>
Cc:     jesse.brandeburg@intel.com, anthony.l.nguyen@intel.com,
        linux-pm@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 2, 2023 at 4:24=E2=80=AFAM Alexander H Duyck
<alexander.duyck@gmail.com> 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@canonical.com>
> > ---
> >  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/e=
thernet/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_resum=
e(struct device *dev)
> >       struct e1000_adapter *adapter =3D netdev_priv(netdev);
> >       int rc;
> >
> > +     pdev->pme_poll =3D true;
> > +
> >       rc =3D __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 =3D 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, cons=
t 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 !=3D 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