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: <3d63835d-cc1f-a6f3-6407-8b7c155a6d65@intel.com>
Date:   Sun, 22 Jan 2023 11:30:44 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.6.1
Subject: Re: [Intel-wired-lan] [PATCH] e1000e: Add ADP_I219_LM17 to ME S0ix
 blacklist
To:     Jia Liu <liujia6264@gmail.com>
CC:     Jacob Keller <jacob.e.keller@intel.com>,
        <jesse.brandeburg@intel.com>, <anthony.l.nguyen@intel.com>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <netdev@vger.kernel.org>,
        <intel-wired-lan@lists.osuosl.org>, <linux-kernel@vger.kernel.org>,
        "Ruinskiy, Dima" <dima.ruinskiy@intel.com>,
        "Lifshits, Vitaly" <vitaly.lifshits@intel.com>,
        "Neftin, Sasha" <sasha.neftin@intel.com>
References: <20230117102645.24920-1-liujia6264@gmail.com>
 <9f29ff29-62bb-c92b-6d69-ccc86938929e@intel.com>
 <5d96deeb-a59d-366d-dbb2-d88623cdfa2d@intel.com>
 <CA+eZsiZ81+AL1-mLb4mONZnMqO=uUPFcw=QWFhEY36_jg9MpiQ@mail.gmail.com>
Content-Language: en-US
From:   "Neftin, Sasha" <sasha.neftin@intel.com>
In-Reply-To: <CA+eZsiZ81+AL1-mLb4mONZnMqO=uUPFcw=QWFhEY36_jg9MpiQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NFhkTWlZNU01M3VhelAxQmZRZk1aZDlTbGtPWllxR1NJUDdLUGE2b1R1VjZ1?=
 =?utf-8?B?Q3piZnF6ZG9YcUo4TVVDNXlqa1hHanJrQkFxNzZ2VjZCeEQxaVB5ZWZVL08w?=
 =?utf-8?B?R0s3K1VVdjhrbHMvTTVVZmFkTVlPTnZYZWFLbjk2amRuVTdqNmh4T3JWQWcy?=
 =?utf-8?B?VDNJZjJCYnlqT2R3d3Y5djE5UmF1eGpsY1EzNVIyeDdNUHRZcW83bndmZ3hD?=
 =?utf-8?B?NnAwbjN0c0FMazh1d3A5dFhtMUU3RW5QMEhKdHpLMXdsWTlyaVJ0RitkdFJz?=
 =?utf-8?B?Vjk2VGZXT2ZBUFQ1Q1dkeFdaRytTa0dPOGRFSXA5OTMxdWZQb0lHem54WXRO?=
 =?utf-8?B?SnFEZXJVcGFVTWlDOEVKaE44bUw2Q01zS0VSZ3UwbDFMUVdEL2FwVUJlN2pz?=
 =?utf-8?B?M3Erc3FURHMzS0ZrRGtRL3RzNkN1SWJNekZZVHRETjdjZ3Azam96c1FSV3RX?=
 =?utf-8?B?N3Y2cEdYMjduQVRnV3ZKSldHcE9POHlMK1VtTGt4ZTZnVm5uV1NaekU5ZFNi?=
 =?utf-8?B?WmtVVEVZbW1EZFBrdzBxa3UxT0x4UWhCTkRndG92M0ZKeXdpRkFjdGwxM1E2?=
 =?utf-8?B?MzhWZlp2TVA1WmNLdmM3d0cwcjBZQUluTFJGVnJzUGNQNUdRZ04xZ3pYWnkx?=
 =?utf-8?B?MTNRNCtSWHUxSkFNbk8wOHU2QmkwR1BrQVhjTmZhcll6andQU0pqeVlqcGlC?=
 =?utf-8?B?bHdhWUI2L2dTaUR6TlltVGJnQnVldTVmQmtadlBTblI2OHJ6QkI3ZlZVYUg1?=
 =?utf-8?B?QWFybW9SQ3hVdWJ5YmUycmlqMC8rOEw2QmpKbjdjSmdlTFY5TE9Vb1BVMWZp?=
 =?utf-8?B?U3JPOHU2Y3RJNDdlYUlCcG51QXFHdXBBYVFDSHgyZmhPVE0vZEY5NHc2cUFn?=
 =?utf-8?B?WEFkaDVlejlnQUJIRHMrYzBFWXJ4TnVuNUdqazM5U2xqRnZXUW5aWTlTbkZx?=
 =?utf-8?B?Q0lMdExnTXN5dzhwTVhOR2pOa1BDVHhjdFhwU2FyQzhZZm1oME8xSjlJS3ln?=
 =?utf-8?B?MitWZCtPMTVvMStMQ0tSVXlJclB0RFhNSWVCdzhLdGpUNCtKZGFOb25YQTl5?=
 =?utf-8?B?VExjY3hHNVFBZE9GMzFicHVTaG9kZTNMZXFkdk1ydjdacFJ5bmxBNG5LYStj?=
 =?utf-8?B?YWw5cVlRK2svR2hyQzBwOHdDK00wb1FLSjBUYWllNzlBODNLWEtEcmVkQ0l1?=
 =?utf-8?B?SExzbEUwRFJvZ2hNUXVoeGEvT014d2xsU3MrSFJ4TzVhNDc1VUVzWE9CYS9m?=
 =?utf-8?B?SHl0OGVpaHUyZmJNcW9LSDlQcmpJdllNTmd1b3MzS25oa2I0c0QxSGxwaGtv?=
 =?utf-8?B?a0ZPd3krUURQSEdhZU8yQ0VZcDkvWS8rYWkrU2hONWV5NG1qYXp3TTVEY1E2?=
 =?utf-8?B?cm05eTVPQ3JxdjB1Vyt0cXRES3JsVEdhWllBdkJGcjEyMjNNT1RNOEFmVjlM?=
 =?utf-8?B?Ty9tNFZ1NEluekhaMWFqeGZoZk82NldhVklCR084N21jMm5BYzlwSXFCNE1F?=
 =?utf-8?B?ZnBQZ1hHbFZRMnBKb2EwUFl5M0dGTkdlL3hDZ0NWVDhxYVlOQVJ3dmxCSkgz?=
 =?utf-8?B?am93UFgzV2YzZVlSUUwyaXB2Uk43dUNTUmYwZDVtKzZsenpod3A1V29SeUlM?=
 =?utf-8?B?ZEtRUUZndER2SzNadjVpb29HZzdxZDE0cFN4NXc1S1Z3NjNSaXl1NGsyalRl?=
 =?utf-8?B?cWVpdkV0VCtNRlpnaTJYVU01cU5xREVHaC9DTGEwbENhNEdQbWExMDBJUy8w?=
 =?utf-8?B?UitqcTNPbVVUT1JZWTZpZ0F1QnRxM0NtbGlxRGgySkJMVDJtNHVPNzR4MjFj?=
 =?utf-8?B?WmVhV01IbjBwSkdTUWdZeUdxcHZLMlpLOGhGc1JUYk5nSHYyOFRyRHp6c1U2?=
 =?utf-8?B?Z2xmaGt6YmFrVnVuN0ZIcDYwV01wNGMwcFJHdm04SUg3UU5BVFFSQ0pGbXN4?=
 =?utf-8?B?VkM2WUJpMy95L1cwU3VHMDFoVk9FR1gvdjVSN2JzS3g4RFRlNHBEbkFFV1d2?=
 =?utf-8?B?ZTNXc2diRzhlSDBHMlo1T0RwRjRTR0RmLzFzemJLRGcyeXJIK0l2SnpWcTI2?=
 =?utf-8?B?TU1ZTENVRFNvZG15cmcveGl1U24yTUJUMWZoeG9KcmhGV1hBYUtZUXRPRUQy?=
 =?utf-8?B?OHR3Vlhjcjl2bUp5ZlRpTzZTZ2lQZjFHMjdqNk9uWnpTN2FWV2dUdEsyUWdW?=
 =?utf-8?B?OGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5c2398e9-eb26-460e-8c63-08dafc5b5b4e
X-MS-Exchange-CrossTenant-AuthSource: MW4PR11MB6738.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jan 2023 09:30:53.4818
 (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: bJMJQKFX4xaR4cJZ6ayT82b7Tadv33KagRRPlNkIqC1TDR0m8+cGdpsIr81o0PmHHd/LZNKgxYT/DglORpxCeA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB5793
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,URIBL_BLOCKED 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/18/2023 11:08, Jia Liu wrote:
> On Wed, Jan 18, 2023 at 1:20 PM Neftin, Sasha <sasha.neftin@intel.com> wrote:
>>
>> 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)
> 
> HP released BIOS including ME firmware on their website HP.com at
> https://support.hp.com/my-en/drivers/selfservice/hp-eliteone-840-23.8-inch-g9-all-in-one-desktop-pc/2101132389.
> There is upgrade interface on the BIOS setup menu which can connect
> HP.com and upgrade to newer BIOS.
> 
> The initial ME version was v16.0.15.1735 from BIOS 02.03.04.
> Then I upgraded to the latest one v16.1.25.1932v3 from BIOS 02.06.01
> released on Nov 28, 2022. Both of them can produce this issue.
> 
> I have only one setup. Is it possible to try on your system which has the
> same I219-LM to see if it's platform specific or not?
Yes, s0ix flows works on our platforms.
> 
>>>>
>>>
>>> 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
>>