lkml.org 
[lkml]   [2023]   [Jan]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [Intel-wired-lan] [PATCH] e1000e: Add ADP_I219_LM17 to ME S0ix blacklist
    From
    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

    \
     
     \ /
      Last update: 2023-03-26 23:45    [W:3.500 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site