linux-kernel - Re: [PATCH] e1000e: Add ADP_I219

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9f29ff29-62bb-c92b-6d69-ccc86938929e@intel.com>
Date:   Tue, 17 Jan 2023 11:34:48 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jiajia Liu <liujia6264@...il.com>, <jesse.brandeburg@...el.com>,
        <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>
CC:     <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] e1000e: Add ADP_I219_LM17 to ME S0ix blacklist



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@...il.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 */