From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 189BAC1B087 for ; Mon, 7 Dec 2020 13:29:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D86FD233A0 for ; Mon, 7 Dec 2020 13:29:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726915AbgLGN3h (ORCPT ); Mon, 7 Dec 2020 08:29:37 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:32248 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726618AbgLGN3e (ORCPT ); Mon, 7 Dec 2020 08:29:34 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1607347687; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=zmWE4Di/SrsoSOSW6qXuYa1KIu/jIAKq7DifFwoMfwo=; b=jRXrn7CzyHYB4rYN7cyWuY8mL4Ehw0D7nnpZFNK5HSTO28hdMB+2ZnlKsxKAJZKhtHrq9x txmdFwaqIjP+Mga3M1A1Ly1ZVGo1YYKzfDtLWJUshuthrd2k4wMNB1Ei+LXfme1fXgsAoM eGjPwXA41RysXmG8fRHjoMOGJRxSF0M= Received: from mail-ej1-f70.google.com (mail-ej1-f70.google.com [209.85.218.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-87-XKq_uyNUN0KJkhEA7WGbpg-1; Mon, 07 Dec 2020 08:28:03 -0500 X-MC-Unique: XKq_uyNUN0KJkhEA7WGbpg-1 Received: by mail-ej1-f70.google.com with SMTP id dc13so3840515ejb.9 for ; Mon, 07 Dec 2020 05:28:03 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=zmWE4Di/SrsoSOSW6qXuYa1KIu/jIAKq7DifFwoMfwo=; b=fkMUVr4iXl/iix5Ve0XRgKNLrNVaz7VNdm05bqROm/eIQnowErEYWet1gtewHQ7lzb jg+2eJepIvxQeK7AAUeF0IvD2GYcHEOC/RiUGnUu46F0nUMPMxv1QWZZuoqqgiXJb6fI CyhJdlzfbkSvhWaAX3t6g/YlHjir4HlkK0VBF+zOSodftCLBQ6BBxhfhY+RNFpOtBH7g mr7sudG9UmH6MVJ5W04RxaP+N4fn+cszJd+o9u+OiIGpu5hrGwmVkA/d9f6TJq3KqhOT A8cTbPYxwKenuXL2EA8B6WtM+naXImRopLnhDKlFXxeZP8p7fyZQIVXAxnpQ0sUHICHr g85A== X-Gm-Message-State: AOAM533Po9jUaWJU49FCNtIkVvivMDDhmlzLd4nR1RiwvYlJa7pEctOX kxpmKn4de+r0jc6J10rL1HU5Juj8sezsnsZ0imBbsLvpSLyzXI35XrKtZ8SFA2moxFRX2dZHr5+ DwhVV6Jvr5WGBmmDj9WP/u+Oc X-Received: by 2002:a17:906:22c7:: with SMTP id q7mr18854677eja.486.1607347681961; Mon, 07 Dec 2020 05:28:01 -0800 (PST) X-Google-Smtp-Source: ABdhPJwEorJ0sLId/xL9w8EuO0JL3r6cc0kBtMY1HiSGC5kejytn+C5L/ToMHIPmo2of6ILKDTvCoA== X-Received: by 2002:a17:906:22c7:: with SMTP id q7mr18854658eja.486.1607347681764; Mon, 07 Dec 2020 05:28:01 -0800 (PST) Received: from x1.localdomain (2001-1c00-0c0c-fe00-d2ea-f29d-118b-24dc.cable.dynamic.v6.ziggo.nl. [2001:1c00:c0c:fe00:d2ea:f29d:118b:24dc]) by smtp.gmail.com with ESMTPSA id oq27sm3461543ejb.108.2020.12.07.05.28.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 07 Dec 2020 05:28:01 -0800 (PST) Subject: Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM To: Mario Limonciello , Jeff Kirsher , Tony Nguyen , intel-wired-lan@lists.osuosl.org Cc: linux-kernel@vger.kernel.org, Linux PM , Netdev , Alexander Duyck , Jakub Kicinski , Sasha Netfin , Aaron Brown , Stefan Assmann , David Miller , darcari@redhat.com, Yijun.Shen@dell.com, Perry.Yuan@dell.com, anthony.wong@canonical.com References: <20201204200920.133780-1-mario.limonciello@dell.com> From: Hans de Goede Message-ID: Date: Mon, 7 Dec 2020 14:28:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 MIME-Version: 1.0 In-Reply-To: <20201204200920.133780-1-mario.limonciello@dell.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 12/4/20 9:09 PM, Mario Limonciello wrote: > commit e086ba2fccda ("e1000e: disable s0ix entry and exit flows for ME systems") > disabled s0ix flows for systems that have various incarnations of the > i219-LM ethernet controller. This was done because of some regressions > caused by an earlier > commit 632fbd5eb5b0e ("e1000e: fix S0ix flows for cable connected case") > with i219-LM controller. > > Performing suspend to idle with these ethernet controllers requires a properly > configured system. To make enabling such systems easier, this patch > series allows determining if enabled and turning on using ethtool. > > The flows have also been confirmed to be configured correctly on Dell's Latitude > and Precision CML systems containing the i219-LM controller, when the kernel also > contains the fix for s0i3.2 entry previously submitted here and now part of this > series. > https://marc.info/?l=linux-netdev&m=160677194809564&w=2 > > Patches 4 through 7 will turn the behavior on by default for some of Dell's > CML and TGL systems. First of all thank you for working on this. I must say though that I don't like the approach taken here very much. This is not so much a criticism of this series as it is a criticism of the earlier decision to simply disable s0ix on all devices with the i219-LM + and active ME. AFAIK there was a perfectly acceptable patch to workaround those broken devices, which increased a timeout: https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20200323191639.48826-1-aaron.ma@canonical.com/ That patch was nacked because it increased the resume time *on broken devices*. So it seems to me that we have a simple choice here: 1. Longer resume time on devices with an improperly configured ME 2. Higher power-consumption on all non-buggy devices Your patches 4-7 try to workaround 2. but IMHO those are just bandaids for getting the initial priorities *very* wrong. Instead of penalizing non-buggy devices with a higher power-consumption, we should default to penalizing the buggy devices with a higher resume time. And if it is decided that the higher resume time is a worse problem then the higher power-consumption, then there should be a list of broken devices and s0ix can be disabled on those. The current allow-list approach is simply never going to work well leading to too high power-consumption on countless devices. This is going to be an endless game of whack-a-mole and as such really is a bad idea. A deny-list for broken devices is a much better approach, esp. since missing devices on that list will still work fine, they will just have a somewhat larger resume time. So what needs to happen IMHO is: 1. Merge your fix from patch 1 of this set 2. Merge "e1000e: bump up timeout to wait when ME un-configure ULP mode" 3. Drop the e1000e_check_me check. Then we also do not need the new "s0ix-enabled" ethertool flag because we do not need userspace to work-around us doing the wrong thing by default. Note a while ago I had access to one of the devices having suspend/resume issues caused by the S0ix support (a Lenovo Thinkpad X1 Carbon gen 7) and I can confirm that the "e1000e: bump up timeout to wait when ME un-configure ULP mode" patch fixes the suspend/resume problem without any noticeable negative side-effects. Regards, Hans > > Changes from v2 to v3: > - Correct some grammar and spelling issues caught by Bjorn H. > * s/s0ix/S0ix/ in all commit messages > * Fix a typo in commit message > * Fix capitalization of proper nouns > - Add more pre-release systems that pass > - Re-order the series to add systems only at the end of the series > - Add Fixes tag to a patch in series. > > Changes from v1 to v2: > - Directly incorporate Vitaly's dependency patch in the series > - Split out s0ix code into it's own file > - Adjust from DMI matching to PCI subsystem vendor ID/device matching > - Remove module parameter and sysfs, use ethtool flag instead. > - Export s0ix flag to ethtool private flags > - Include more people and lists directly in this submission chain. > > Mario Limonciello (6): > e1000e: Move all S0ix related code into its own source file > e1000e: Export S0ix flags to ethtool > e1000e: Add Dell's Comet Lake systems into S0ix heuristics > e1000e: Add more Dell CML systems into S0ix heuristics > e1000e: Add Dell TGL desktop systems into S0ix heuristics > e1000e: Add another Dell TGL notebook system into S0ix heuristics > > Vitaly Lifshits (1): > e1000e: fix S0ix flow to allow S0i3.2 subset entry > > drivers/net/ethernet/intel/e1000e/Makefile | 2 +- > drivers/net/ethernet/intel/e1000e/e1000.h | 4 + > drivers/net/ethernet/intel/e1000e/ethtool.c | 40 +++ > drivers/net/ethernet/intel/e1000e/netdev.c | 272 +---------------- > drivers/net/ethernet/intel/e1000e/s0ix.c | 311 ++++++++++++++++++++ > 5 files changed, 361 insertions(+), 268 deletions(-) > create mode 100644 drivers/net/ethernet/intel/e1000e/s0ix.c > > -- > 2.25.1 > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Mon, 7 Dec 2020 14:28:00 +0100 Subject: [Intel-wired-lan] [PATCH v3 0/7] Improve s0ix flows for systems i219LM In-Reply-To: <20201204200920.133780-1-mario.limonciello@dell.com> References: <20201204200920.133780-1-mario.limonciello@dell.com> Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: Hi, On 12/4/20 9:09 PM, Mario Limonciello wrote: > commit e086ba2fccda ("e1000e: disable s0ix entry and exit flows for ME systems") > disabled s0ix flows for systems that have various incarnations of the > i219-LM ethernet controller. This was done because of some regressions > caused by an earlier > commit 632fbd5eb5b0e ("e1000e: fix S0ix flows for cable connected case") > with i219-LM controller. > > Performing suspend to idle with these ethernet controllers requires a properly > configured system. To make enabling such systems easier, this patch > series allows determining if enabled and turning on using ethtool. > > The flows have also been confirmed to be configured correctly on Dell's Latitude > and Precision CML systems containing the i219-LM controller, when the kernel also > contains the fix for s0i3.2 entry previously submitted here and now part of this > series. > https://marc.info/?l=linux-netdev&m=160677194809564&w=2 > > Patches 4 through 7 will turn the behavior on by default for some of Dell's > CML and TGL systems. First of all thank you for working on this. I must say though that I don't like the approach taken here very much. This is not so much a criticism of this series as it is a criticism of the earlier decision to simply disable s0ix on all devices with the i219-LM + and active ME. AFAIK there was a perfectly acceptable patch to workaround those broken devices, which increased a timeout: https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20200323191639.48826-1-aaron.ma at canonical.com/ That patch was nacked because it increased the resume time *on broken devices*. So it seems to me that we have a simple choice here: 1. Longer resume time on devices with an improperly configured ME 2. Higher power-consumption on all non-buggy devices Your patches 4-7 try to workaround 2. but IMHO those are just bandaids for getting the initial priorities *very* wrong. Instead of penalizing non-buggy devices with a higher power-consumption, we should default to penalizing the buggy devices with a higher resume time. And if it is decided that the higher resume time is a worse problem then the higher power-consumption, then there should be a list of broken devices and s0ix can be disabled on those. The current allow-list approach is simply never going to work well leading to too high power-consumption on countless devices. This is going to be an endless game of whack-a-mole and as such really is a bad idea. A deny-list for broken devices is a much better approach, esp. since missing devices on that list will still work fine, they will just have a somewhat larger resume time. So what needs to happen IMHO is: 1. Merge your fix from patch 1 of this set 2. Merge "e1000e: bump up timeout to wait when ME un-configure ULP mode" 3. Drop the e1000e_check_me check. Then we also do not need the new "s0ix-enabled" ethertool flag because we do not need userspace to work-around us doing the wrong thing by default. Note a while ago I had access to one of the devices having suspend/resume issues caused by the S0ix support (a Lenovo Thinkpad X1 Carbon gen 7) and I can confirm that the "e1000e: bump up timeout to wait when ME un-configure ULP mode" patch fixes the suspend/resume problem without any noticeable negative side-effects. Regards, Hans > > Changes from v2 to v3: > - Correct some grammar and spelling issues caught by Bjorn H. > * s/s0ix/S0ix/ in all commit messages > * Fix a typo in commit message > * Fix capitalization of proper nouns > - Add more pre-release systems that pass > - Re-order the series to add systems only at the end of the series > - Add Fixes tag to a patch in series. > > Changes from v1 to v2: > - Directly incorporate Vitaly's dependency patch in the series > - Split out s0ix code into it's own file > - Adjust from DMI matching to PCI subsystem vendor ID/device matching > - Remove module parameter and sysfs, use ethtool flag instead. > - Export s0ix flag to ethtool private flags > - Include more people and lists directly in this submission chain. > > Mario Limonciello (6): > e1000e: Move all S0ix related code into its own source file > e1000e: Export S0ix flags to ethtool > e1000e: Add Dell's Comet Lake systems into S0ix heuristics > e1000e: Add more Dell CML systems into S0ix heuristics > e1000e: Add Dell TGL desktop systems into S0ix heuristics > e1000e: Add another Dell TGL notebook system into S0ix heuristics > > Vitaly Lifshits (1): > e1000e: fix S0ix flow to allow S0i3.2 subset entry > > drivers/net/ethernet/intel/e1000e/Makefile | 2 +- > drivers/net/ethernet/intel/e1000e/e1000.h | 4 + > drivers/net/ethernet/intel/e1000e/ethtool.c | 40 +++ > drivers/net/ethernet/intel/e1000e/netdev.c | 272 +---------------- > drivers/net/ethernet/intel/e1000e/s0ix.c | 311 ++++++++++++++++++++ > 5 files changed, 361 insertions(+), 268 deletions(-) > create mode 100644 drivers/net/ethernet/intel/e1000e/s0ix.c > > -- > 2.25.1 > >