Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 17 Jan 2023 11:34:48 -0800 | Subject | Re: [PATCH] e1000e: Add ADP_I219_LM17 to ME S0ix blacklist | From | Jacob Keller <> |
| |
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 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 */
| ![\](/images/icornerr.gif) |