Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9f29ff29-62bb-c92b-6d69-ccc86938929e@intel.com>
Date:   Tue, 17 Jan 2023 11:34:48 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH] e1000e: Add ADP_I219_LM17 to ME S0ix blacklist
Content-Language: en-US
To:     Jiajia Liu <liujia6264@gmail.com>, <jesse.brandeburg@intel.com>,
        <anthony.l.nguyen@intel.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>
CC:     <intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20230117102645.24920-1-liujia6264@gmail.com>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20230117102645.24920-1-liujia6264@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aU81Q1dsenBoQWx5T25uMjdXZ2lnVGs0Z3FkZThQMWFqSzBHdlpiU294VGVl?=
 =?utf-8?B?L1JmbDlWK3pCclhFeWVkV1lqbUlNb3FvTmRlZlhKNGNrR2tpZjlPaTQrQ0RM?=
 =?utf-8?B?bUFZRE9VZXQvbDhjZHJJL0RSVjVwUnh2Y2l6TGlHbnJrbkpDZTNjWndnSlVW?=
 =?utf-8?B?djIxT0Z0ay9uM1drNXRSZWZ0ZFlXSXZ0OGFBMklZK0k3T1NJNUJHbXRtT2Fo?=
 =?utf-8?B?QnNwWE5XWGpIcS85bW93dWRiY0l2OHFKVFlqYS9BSVBTeDlIQU95WmxKTEhY?=
 =?utf-8?B?MC9rSG9zck9XdG9JTFAxRVIzRGtzSFRiNXo1Z0RoSUFDNnpMUUxQcXV5QUdY?=
 =?utf-8?B?c1M3VmdqbDFiZVpIUFdkeVE5UDJwTkxtdGlYY052Q2t5dHFHRTVJWmpRdTRp?=
 =?utf-8?B?TEJYREg5c2d4TzJJdnRXL1hHUEJpUG5VSGRMd1VxSkprSmJuUlFDNC8vVTVS?=
 =?utf-8?B?QW01OWxiQ3NJZFFUSmRvdFg4QjIwTS9tOWIxa1grbjJRSlNBZTZQYndRbkhM?=
 =?utf-8?B?bmRvZVo4VHlHWHY4MVR0OStvNFQ4QlVTNUUvZGNPODRQWllQT2RsaVc3VGxv?=
 =?utf-8?B?R1IrMGtMaHprZjlmWnN4NkZTT1NHZ3JSRHp6NWgydXcva2hnMUVFWTNrL29X?=
 =?utf-8?B?bW9iRlZWT2xyVW01bEdiM0pSZ3dMNUdUL3J1bDkxbzJ6RVI0VEgxQStOQXhu?=
 =?utf-8?B?b05OOUpiN3VxVFFhaXByeDF6OUt1aFFJWkdRMHk0NDBaeit4Mjc0TDJBbnNz?=
 =?utf-8?B?RklveWoyRVNsUitSN1RRMm5KaERsZjNQWUtiVGJ4bW9VVlpwdWZaWDRpZDJp?=
 =?utf-8?B?ZUtldUU0STNMMFVqV3Z3bHJGVHBQVDVUSFZCYTViWkQvL2tHVW93TlVJdjZ6?=
 =?utf-8?B?VVNqbHZOTnFoRFVIQm8yazBSa01CQS9LaW5uN081aE5nWjBjRjBWbGtSZks1?=
 =?utf-8?B?WGJWVWNDdXpIUVBQTGg2dTNQYmtGZFRvTEw5bUREZnNyMVREMjZvbXRHNDZW?=
 =?utf-8?B?Qk50SnYxVkNWZFFtbVg0ZWZpcHlKQnJncmZpTjF6Tm1Id3JFQ3F5N0RGaEdW?=
 =?utf-8?B?aGNlV0VtTzdqS3RsamY3YTA2S21IM0wxMXBBcDNUZW55dWJzelA5VE1DT0xy?=
 =?utf-8?B?V0NiVmNKR2tpY2lSeFpQK1VkMTR6cDJFME1YSmtlUVlodTk0MEgwOER1WDBJ?=
 =?utf-8?B?QUEwNnQ1VWNtc25GMGN6c3kwbUFnY1VYWnR5Skw4cjR6UTZ5NTNJWUlDVVFS?=
 =?utf-8?B?RDNyT0NyelRYckR0enNwNWh5b0xDeks5eHhjTFFwQVdGM2k1dkxLSlZqVnlu?=
 =?utf-8?B?bHVrWFIwUmxmN25nenhjMEVUVUF2WGlqNHVjMzlndE9nSi95dmlodFhjQm9t?=
 =?utf-8?B?QWpzRjlERlNtRWVRYmFYdUE5Nkg2cGtwSXkwZDNQSkczY0NZcE0rNmE1dm54?=
 =?utf-8?B?alZRdCtSak9jckpORno3TEJ5M0gyakYrVk5uYi9NUGNhVTBjL2ZjYVpEVFR0?=
 =?utf-8?B?cVJwc2JwakpPL3lhWUpid1JkamViYmVRZnlrendRa0R1a2tCZDE0OEp4WkZD?=
 =?utf-8?B?WlkrakhtZGFnMUgzZE1BaGVVM3JiRjlJWGdXSkJ1WGZKRTVSVVJPWW4vZEZh?=
 =?utf-8?B?eUppT1l3V29qNkVOVk5BY1FyVkhRMHdadXlOQlNYNVl3OXhVclRCeEs0Z1Fv?=
 =?utf-8?B?WU9LS09jRXYxcDcvcmhGekpaWWpWT0pkL2dTRFR4MTZGWUovTTEzRmQvWTBE?=
 =?utf-8?B?YjF4aFZrVDlZcUFhQUt5cy8vMWE5YXg3ZFFhRkowTlh5cWR5MnZiY3E3c1hk?=
 =?utf-8?B?cWUrbWs0UjBRRE1SdDBtbDNob0dqVDJLb3dvVFJVeDNJaE5meHpTT2R6K1RC?=
 =?utf-8?B?b2M3UnlrU1NrZkxjRU5PMlRGTmVub3dWakFBRXhrdkRnbCtpc09wbnFhYmU5?=
 =?utf-8?B?c1lJQXN4SW9PWTVwV0NGTENMazFIRCt1emVGSzh6VjlNa2IxUmZpUWpFY2Jp?=
 =?utf-8?B?MnZTNmF3MUxBV2JCSFNlVXRMNzQ4Y2hLL3RPL3oxQUpNK1ZaTmkzdXZRSjVN?=
 =?utf-8?B?NExXNnNYdnB0Zkk5T0NId3lmb0xoR0tJWHVCVFF0OEJielVEY1pIamZFWWNr?=
 =?utf-8?B?UHFQL2N0MFhsVE5leVNESi91WS9Jd0I4RU85VUtTeE1FWlFITnlKVnlMY1c0?=
 =?utf-8?B?c0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c15e16fd-5933-4685-5688-08daf8c1e706
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jan 2023 19:34:51.6237
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /++/hWwwtsG5h8AMuKf/oogFmKjtZ0hSte20FcBxghZHOKMHWH47QU9NTXPhKvm6+0teSQI8plnoU9+SSBqbSBn1gzKEoaeKAbEKg/XMM34=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6127
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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 */