Messages in this thread | | | Date | Wed, 18 Jan 2023 07:20:03 +0200 | Subject | Re: [Intel-wired-lan] [PATCH] e1000e: Add ADP_I219_LM17 to ME S0ix blacklist | From | "Neftin, Sasha" <> |
| |
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@gmail.com> >> --- >> >> 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) >> > > 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@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
| |