Re: [Intel-wired-lan] [PATCH] e1000e: Add ADP_I219_LM17 to ME S0ix blacklist

[Neftin, Sasha]

On 1/18/2023 11:08, Jia Liu wrote:
> On Wed, Jan 18, 2023 at 1:20 PM Neftin, Sasha <sasha.neftin@xxxxxxxxx> wrote:
> > On 1/17/2023 21:34, Jacob Keller wrote:
> > > On 1/17/2023 2:26 AM, Jiajia Liu wrote:
> > > > I219 on HP EliteOne 840 All in One cannot work after s2idle resume
> > > > when the link speed is Gigabit, Wake-on-LAN is enabled and then set
> > > > the link down before suspend. No issue found when requesting driver
> > > > to configure S0ix. Add workround to let ADP_I219_LM17 use the dirver
> > > > configured S0ix.
> > > >
> > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216926
> > > > Signed-off-by: Jiajia Liu <liujia6264@xxxxxxxxx>
> > > > ---
> > > >
> > > > It's regarding the bug above, it looks it's causued by the ME S0ix.
> > > > And is there a method to make the ME S0ix path work?
> > No. This is a fragile approach. ME must get the message from us
> > (unconfigure the device from s0ix). Otherwise, ME will continue to
> > access LAN resources and the controller could get stuck.
> > I see two ways:
> > 1. you always can skip s0ix flow by priv_flag
> > 2. Especially in this case (HP platform) - please, contact HP (what is
> > the ME version on this system, and how was it released...). HP will open
> > a ticket with Intel. (then we can involve the ME team)
>
> HP released BIOS including ME firmware on their website HP.com at
> https://support.hp.com/my-en/drivers/selfservice/hp-eliteone-840-23.8-inch-g9-all-in-one-desktop-pc/2101132389.
> There is upgrade interface on the BIOS setup menu which can connect
> HP.com and upgrade to newer BIOS.
>
> The initial ME version was v16.0.15.1735 from BIOS 02.03.04.
> Then I upgraded to the latest one v16.1.25.1932v3 from BIOS 02.06.01
> released on Nov 28, 2022. Both of them can produce this issue.
>
> I have only one setup. Is it possible to try on your system which has the
> same I219-LM to see if it's platform specific or not?
Yes, s0ix flows works on our platforms.
> > > >
> > >
> > > No idea. It does seem better to disable S0ix if it doesn't work properly
> > > first though...
> > >
> > > >    drivers/net/ethernet/intel/e1000e/netdev.c | 25 ++++++++++++++++++++++
> > > >    1 file changed, 25 insertions(+)
> > > >
> > > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > > > index 04acd1a992fa..7ee759dbd09d 100644
> > > > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > > > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > > > @@ -6330,6 +6330,23 @@ static void e1000e_flush_lpic(struct pci_dev *pdev)
> > > >       pm_runtime_put_sync(netdev->dev.parent);
> > > >    }
> > > >
> > > > +static u16 me_s0ix_blacklist[] = {
> > > > +    E1000_DEV_ID_PCH_ADP_I219_LM17,
> > > > +    0
> > > > +};
> > > > +
> > > > +static bool e1000e_check_me_s0ix_blacklist(const struct e1000_adapter *adapter)
> > > > +{
> > > > +    u16 *list;
> > > > +
> > > > +    for (list = me_s0ix_blacklist; *list; list++) {
> > > > +            if (*list == adapter->pdev->device)
> > > > +                    return true;
> > > > +    }
> > > > +
> > > > +    return false;
> > > > +}
> > >
> > > The name of this function seems odd..? "check_me"? It also seems like we
> > > could just do a simple switch/case on the device ID or similar.
> > >
> > > Maybe: "e1000e_device_supports_s0ix"?
> > >
> > > > +
> > > >    /* S0ix implementation */
> > > >    static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
> > > >    {
> > > > @@ -6337,6 +6354,9 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
> > > >       u32 mac_data;
> > > >       u16 phy_data;
> > > >
> > > > +    if (e1000e_check_me_s0ix_blacklist(adapter))
> > > > +            goto req_driver;
> > > > +
> > > >       if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
> > > >           hw->mac.type >= e1000_pch_adp) {
> > > >               /* Request ME configure the device for S0ix */
> > >
> > >
> > > The related code also seems to already perform some set of mac checks
> > > here...
> > >
> > > > @@ -6346,6 +6366,7 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
> > > >               trace_e1000e_trace_mac_register(mac_data);
> > > >               ew32(H2ME, mac_data);
> > > >       } else {
> > > > +req_driver:>                /* Request driver configure the device to S0ix */
> > > >               /* Disable the periodic inband message,
> > > >                * don't request PCIe clock in K1 page770_17[10:9] = 10b
> > > > @@ -6488,6 +6509,9 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
> > > >       u16 phy_data;
> > > >       u32 i = 0;
> > > >
> > > > +    if (e1000e_check_me_s0ix_blacklist(adapter))
> > > > +            goto req_driver;
> > > > +
> > >
> > > Why not just combine this check into the statement below rather than
> > > adding a goto?
> > >
> > > >       if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
> > > >           hw->mac.type >= e1000_pch_adp) {
> > > >               /* Keep the GPT clock enabled for CSME */
> > > > @@ -6523,6 +6547,7 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
> > > >               else
> > > >                       e_dbg("DPG_EXIT_DONE cleared after %d msec\n", i * 10);
> > > >       } else {
> > > > +req_driver:
> > > >               /* Request driver unconfigure the device from S0ix */
> > > >
> > > >               /* Disable the Dynamic Power Gating in the MAC */
> > > _______________________________________________
> > > Intel-wired-lan mailing list
> > > Intel-wired-lan@xxxxxxxxxx
> > > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan