[PATCH v4 0/4] Improve s0ix flows for systems i219LM

[PATCH v4 0/4] Improve s0ix flows for systems i219LM

[Mario Limonciello]

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.

Per discussion with Intel architecture team this direction should be changed and
allow S0ix flows to be used by default.  This patch series includes directional
changes for their conclusions in https://lkml.org/lkml/2020/12/13/15.

Changes from v3 to v4:
 - Drop patch 1 for proper s0i3.2 entry, it was separated and is now merged in kernel
 - Add patch to only run S0ix flows if shutdown succeeded which was suggested in
   thread
 - Adjust series for guidance from https://lkml.org/lkml/2020/12/13/15
   * Revert i219-LM disallow-list.
   * Drop all patches for systems tested by Dell in an allow list
   * Increase ULP timeout to 1000ms
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 (4):
  e1000e: Only run S0ix flows if shutdown succeeded
  e1000e: bump up timeout to wait when ME un-configure ULP mode
  Revert "e1000e: disable s0ix entry and exit flows for ME systems"
  e1000e: Export S0ix flags to ethtool

 drivers/net/ethernet/intel/e1000e/e1000.h   |  1 +
 drivers/net/ethernet/intel/e1000e/ethtool.c | 40 ++++++++++++++
 drivers/net/ethernet/intel/e1000e/ich8lan.c |  4 +-
 drivers/net/ethernet/intel/e1000e/netdev.c  | 59 ++++-----------------
 4 files changed, 53 insertions(+), 51 deletions(-)

--
2.25.1

[PATCH 1/4] e1000e: Only run S0ix flows if shutdown succeeded

[Mario Limonciello]

If the shutdown failed, the part will be thawed and running
S0ix flows will put it into an undefined state.

Reported-by: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 128ab6898070..6588f5d4a2be 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6970,13 +6970,14 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev)
 	e1000e_pm_freeze(dev);
 
 	rc = __e1000_shutdown(pdev, false);
-	if (rc)
+	if (rc) {
 		e1000e_pm_thaw(dev);
-
-	/* Introduce S0ix implementation */
-	if (hw->mac.type >= e1000_pch_cnp &&
-	    !e1000e_check_me(hw->adapter->pdev->device))
-		e1000e_s0ix_entry_flow(adapter);
+	} else {
+		/* Introduce S0ix implementation */
+		if (hw->mac.type >= e1000_pch_cnp &&
+		    !e1000e_check_me(hw->adapter->pdev->device))
+			e1000e_s0ix_entry_flow(adapter);
+	}
 
 	return rc;
 }
-- 
2.25.1

[PATCH 2/4] e1000e: bump up timeout to wait when ME un-configure ULP mode

[Mario Limonciello]

Per guidance from Intel ethernet architecture team, it may take
up to 1 second for unconfiguring ULP mode.

Suggested-by: Aaron Ma <aaron.ma@canonical.com>
Suggested-by: Sasha Netfin <sasha.neftin@intel.com>
Fixes: f15bb6dde738cc8fa0 ("e1000e: Add support for S0ix")
BugLink: https://bugs.launchpad.net/bugs/1865570
Link: https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20200323191639.48826-1-aaron.ma@canonical.com/
Link: https://lkml.org/lkml/2020/12/13/15
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 9aa6fad8ed47..4621d01e8a24 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -1248,9 +1248,9 @@ static s32 e1000_disable_ulp_lpt_lp(struct e1000_hw *hw, bool force)
 			ew32(H2ME, mac_reg);
 		}
 
-		/* Poll up to 300msec for ME to clear ULP_CFG_DONE. */
+		/* Poll up to 1 second for ME to clear ULP_CFG_DONE. */
 		while (er32(FWSM) & E1000_FWSM_ULP_CFG_DONE) {
-			if (i++ == 30) {
+			if (i++ == 100) {
 				ret_val = -E1000_ERR_PHY;
 				goto out;
 			}
-- 
2.25.1

[PATCH 3/4] Revert "e1000e: disable s0ix entry and exit flows for ME systems"

[Mario Limonciello]

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 changed caused power consumption regressions
on the following shipping Dell Comet Lake based laptops:
* Latitude 5310
* Latitude 5410
* Latitude 5410
* Latitude 5510
* Precision 3550
* Latitude 5411
* Latitude 5511
* Precision 3551
* Precision 7550
* Precision 7750

This commit was introduced because of some regressions on certain Thinkpad
laptops.  This comment was potentially caused by an earlier
commit 632fbd5eb5b0e ("e1000e: fix S0ix flows for cable connected case").
or it was possibly caused by a system not meeting platform architectural
requirements for low power consumption.  Other changes made in the driver
with extended timeouts are expected to make the driver more impervious to
platform firmware behavior.

Fixes: e086ba2fccda ("e1000e: disable s0ix entry and exit flows for ME systems")
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 45 +---------------------
 1 file changed, 2 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 6588f5d4a2be..b9800ba2006c 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -103,45 +103,6 @@ static const struct e1000_reg_info e1000_reg_info_tbl[] = {
 	{0, NULL}
 };
 
-struct e1000e_me_supported {
-	u16 device_id;		/* supported device ID */
-};
-
-static const struct e1000e_me_supported me_supported[] = {
-	{E1000_DEV_ID_PCH_LPT_I217_LM},
-	{E1000_DEV_ID_PCH_LPTLP_I218_LM},
-	{E1000_DEV_ID_PCH_I218_LM2},
-	{E1000_DEV_ID_PCH_I218_LM3},
-	{E1000_DEV_ID_PCH_SPT_I219_LM},
-	{E1000_DEV_ID_PCH_SPT_I219_LM2},
-	{E1000_DEV_ID_PCH_LBG_I219_LM3},
-	{E1000_DEV_ID_PCH_SPT_I219_LM4},
-	{E1000_DEV_ID_PCH_SPT_I219_LM5},
-	{E1000_DEV_ID_PCH_CNP_I219_LM6},
-	{E1000_DEV_ID_PCH_CNP_I219_LM7},
-	{E1000_DEV_ID_PCH_ICP_I219_LM8},
-	{E1000_DEV_ID_PCH_ICP_I219_LM9},
-	{E1000_DEV_ID_PCH_CMP_I219_LM10},
-	{E1000_DEV_ID_PCH_CMP_I219_LM11},
-	{E1000_DEV_ID_PCH_CMP_I219_LM12},
-	{E1000_DEV_ID_PCH_TGP_I219_LM13},
-	{E1000_DEV_ID_PCH_TGP_I219_LM14},
-	{E1000_DEV_ID_PCH_TGP_I219_LM15},
-	{0}
-};
-
-static bool e1000e_check_me(u16 device_id)
-{
-	struct e1000e_me_supported *id;
-
-	for (id = (struct e1000e_me_supported *)me_supported;
-	     id->device_id; id++)
-		if (device_id == id->device_id)
-			return true;
-
-	return false;
-}
-
 /**
  * __ew32_prepare - prepare to write to MAC CSR register on certain parts
  * @hw: pointer to the HW structure
@@ -6974,8 +6935,7 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev)
 		e1000e_pm_thaw(dev);
 	} else {
 		/* Introduce S0ix implementation */
-		if (hw->mac.type >= e1000_pch_cnp &&
-		    !e1000e_check_me(hw->adapter->pdev->device))
+		if (hw->mac.type >= e1000_pch_cnp)
 			e1000e_s0ix_entry_flow(adapter);
 	}
 
@@ -6991,8 +6951,7 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev)
 	int rc;
 
 	/* Introduce S0ix implementation */
-	if (hw->mac.type >= e1000_pch_cnp &&
-	    !e1000e_check_me(hw->adapter->pdev->device))
+	if (hw->mac.type >= e1000_pch_cnp)
 		e1000e_s0ix_exit_flow(adapter);
 
 	rc = __e1000_resume(pdev);
-- 
2.25.1

[PATCH 4/4] e1000e: Export S0ix flags to ethtool

[Mario Limonciello]

This flag can be used by an end user to disable S0ix flows on a
buggy system or by an OEM for development purposes.

If you need this flag to be persisted across reboots, it's suggested
to use a udev rule to call adjust it until the kernel could have your
configuration in a disallow list.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/net/ethernet/intel/e1000e/e1000.h   |  1 +
 drivers/net/ethernet/intel/e1000e/ethtool.c | 40 +++++++++++++++++++++
 drivers/net/ethernet/intel/e1000e/netdev.c  |  9 ++---
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index ba7a0f8f6937..5b2143f4b1f8 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -436,6 +436,7 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca);
 #define FLAG2_DFLT_CRC_STRIPPING          BIT(12)
 #define FLAG2_CHECK_RX_HWTSTAMP           BIT(13)
 #define FLAG2_CHECK_SYSTIM_OVERFLOW       BIT(14)
+#define FLAG2_ENABLE_S0IX_FLOWS           BIT(15)
 
 #define E1000_RX_DESC_PS(R, i)	    \
 	(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index 03215b0aee4b..eb683949ebfe 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -23,6 +23,13 @@ struct e1000_stats {
 	int stat_offset;
 };
 
+static const char e1000e_priv_flags_strings[][ETH_GSTRING_LEN] = {
+#define E1000E_PRIV_FLAGS_S0IX_ENABLED	BIT(0)
+	"s0ix-enabled",
+};
+
+#define E1000E_PRIV_FLAGS_STR_LEN ARRAY_SIZE(e1000e_priv_flags_strings)
+
 #define E1000_STAT(str, m) { \
 		.stat_string = str, \
 		.type = E1000_STATS, \
@@ -1776,6 +1783,8 @@ static int e1000e_get_sset_count(struct net_device __always_unused *netdev,
 		return E1000_TEST_LEN;
 	case ETH_SS_STATS:
 		return E1000_STATS_LEN;
+	case ETH_SS_PRIV_FLAGS:
+		return E1000E_PRIV_FLAGS_STR_LEN;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -2097,6 +2106,10 @@ static void e1000_get_strings(struct net_device __always_unused *netdev,
 			p += ETH_GSTRING_LEN;
 		}
 		break;
+	case ETH_SS_PRIV_FLAGS:
+		memcpy(data, e1000e_priv_flags_strings,
+		       E1000E_PRIV_FLAGS_STR_LEN * ETH_GSTRING_LEN);
+		break;
 	}
 }
 
@@ -2305,6 +2318,31 @@ static int e1000e_get_ts_info(struct net_device *netdev,
 	return 0;
 }
 
+static u32 e1000e_get_priv_flags(struct net_device *netdev)
+{
+	struct e1000_adapter *adapter = netdev_priv(netdev);
+	u32 priv_flags = 0;
+
+	if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
+		priv_flags |= E1000E_PRIV_FLAGS_S0IX_ENABLED;
+
+	return priv_flags;
+}
+
+static int e1000e_set_priv_flags(struct net_device *netdev, u32 priv_flags)
+{
+	struct e1000_adapter *adapter = netdev_priv(netdev);
+	unsigned int flags2 = adapter->flags2;
+
+	flags2 &= ~FLAG2_ENABLE_S0IX_FLOWS;
+	if (priv_flags & E1000E_PRIV_FLAGS_S0IX_ENABLED)
+		flags2 |= FLAG2_ENABLE_S0IX_FLOWS;
+	if (flags2 != adapter->flags2)
+		adapter->flags2 = flags2;
+
+	return 0;
+}
+
 static const struct ethtool_ops e1000_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS,
 	.get_drvinfo		= e1000_get_drvinfo,
@@ -2336,6 +2374,8 @@ static const struct ethtool_ops e1000_ethtool_ops = {
 	.set_eee		= e1000e_set_eee,
 	.get_link_ksettings	= e1000_get_link_ksettings,
 	.set_link_ksettings	= e1000_set_link_ksettings,
+	.get_priv_flags		= e1000e_get_priv_flags,
+	.set_priv_flags		= e1000e_set_priv_flags,
 };
 
 void e1000e_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index b9800ba2006c..e9b82c209c2d 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6923,7 +6923,6 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev)
 	struct net_device *netdev = pci_get_drvdata(to_pci_dev(dev));
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct pci_dev *pdev = to_pci_dev(dev);
-	struct e1000_hw *hw = &adapter->hw;
 	int rc;
 
 	e1000e_flush_lpic(pdev);
@@ -6935,7 +6934,7 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev)
 		e1000e_pm_thaw(dev);
 	} else {
 		/* Introduce S0ix implementation */
-		if (hw->mac.type >= e1000_pch_cnp)
+		if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
 			e1000e_s0ix_entry_flow(adapter);
 	}
 
@@ -6947,11 +6946,10 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev)
 	struct net_device *netdev = pci_get_drvdata(to_pci_dev(dev));
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct pci_dev *pdev = to_pci_dev(dev);
-	struct e1000_hw *hw = &adapter->hw;
 	int rc;
 
 	/* Introduce S0ix implementation */
-	if (hw->mac.type >= e1000_pch_cnp)
+	if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
 		e1000e_s0ix_exit_flow(adapter);
 
 	rc = __e1000_resume(pdev);
@@ -7615,6 +7613,9 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!(adapter->flags & FLAG_HAS_AMT))
 		e1000e_get_hw_control(adapter);
 
+	if (hw->mac.type >= e1000_pch_cnp)
+		adapter->flags2 |= FLAG2_ENABLE_S0IX_FLOWS;
+
 	strlcpy(netdev->name, "eth%d", sizeof(netdev->name));
 	err = register_netdev(netdev);
 	if (err)
-- 
2.25.1

Re: [PATCH v4 0/4] Improve s0ix flows for systems i219LM

[Alexander Duyck]

On Mon, Dec 14, 2020 at 7:35 AM Mario Limonciello
<mario.limonciello@dell.com> 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.
>
> Per discussion with Intel architecture team this direction should be changed and
> allow S0ix flows to be used by default.  This patch series includes directional
> changes for their conclusions in https://lkml.org/lkml/2020/12/13/15.
>
> Changes from v3 to v4:
>  - Drop patch 1 for proper s0i3.2 entry, it was separated and is now merged in kernel
>  - Add patch to only run S0ix flows if shutdown succeeded which was suggested in
>    thread
>  - Adjust series for guidance from https://lkml.org/lkml/2020/12/13/15
>    * Revert i219-LM disallow-list.
>    * Drop all patches for systems tested by Dell in an allow list
>    * Increase ULP timeout to 1000ms
> 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 (4):
>   e1000e: Only run S0ix flows if shutdown succeeded
>   e1000e: bump up timeout to wait when ME un-configure ULP mode
>   Revert "e1000e: disable s0ix entry and exit flows for ME systems"
>   e1000e: Export S0ix flags to ethtool
>
>  drivers/net/ethernet/intel/e1000e/e1000.h   |  1 +
>  drivers/net/ethernet/intel/e1000e/ethtool.c | 40 ++++++++++++++
>  drivers/net/ethernet/intel/e1000e/ich8lan.c |  4 +-
>  drivers/net/ethernet/intel/e1000e/netdev.c  | 59 ++++-----------------
>  4 files changed, 53 insertions(+), 51 deletions(-)
>

The changes look good to me.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Re: [PATCH v4 0/4] Improve s0ix flows for systems i219LM

[Hans de Goede]

Hi All,

Sasha (and the other intel-wired-lan folks), thank you for investigating this
further and for coming up with a better solution.

Mario, thank you for implementing the new scheme.

I've tested this patch set on a Lenovo X1C8 with vPRO and AMT enabled in the BIOS
(the previous issues were soon on a X1C7).

I have good and bad news:

The good news is that after reverting the
"e1000e: disable s0ix entry and exit flows for ME systems"
I can reproduce the original issue on the X1C8 (I no longer have
a X1C7 to test on).

The bad news is that increasing the timeout to 1 second does
not fix the issue. Suspend/resume is still broken after one
suspend/resume cycle, as described in the original bug-report:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1865570

More good news though, bumping the timeout to 250 poll iterations
(approx 2.5 seconds) as done in Aaron Ma's original patch for
this fixes this on the X1C8 just as it did on the X1C7
(it takes 2 seconds for ULP_CONFIG_DONE to clear).

I've ran some extra tests and the poll loop succeeds on its
first iteration when an ethernet-cable is connected. It seems
that Lenovo's variant of the ME firmware waits up to 2 seconds
for a link, causing the long wait for ULP_CONFIG_DONE to clear.

I think that for now the best fix would be to increase the timeout
to 2.5 seconds as done in  Aaron Ma's original patch. Combined
with a broken-firmware warning when we waited longer then 1 second,
to make it clear that there is a firmware issue here and that
the long wait / slow resume is not the fault of the driver.

###

I've added Mark Pearson from Lenovo to the Cc so that Lenovo
can investigate this issue further.

Mark, this thread is about an issue with enabling S0ix support for
e1000e (i219lm) controllers. This was enabled in the kernel a
while ago, but then got disabled again on vPro / AMT enabled
systems because on some systems (Lenovo X1C7 and now also X1C8)
this lead to suspend/resume issues.

When AMT is active then there is a handover handshake for the
OS to get access to the ethernet controller from the ME. The
Intel folks have checked and the Windows driver is using a timeout
of 1 second for this handshake, yet on Lenovo systems this is
taking 2 seconds. This likely has something to do with the
ME firmware on these Lenovo models, can you get the firmware
team at Lenovo to investigate this further ?

Regards,

Hans

p.s.

I also have a small review remark on patch 4/4 I will
reply to that patch separately.








On 12/14/20 4:34 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.
> 
> Per discussion with Intel architecture team this direction should be changed and
> allow S0ix flows to be used by default.  This patch series includes directional
> changes for their conclusions in https://lkml.org/lkml/2020/12/13/15.
> 
> Changes from v3 to v4:
>  - Drop patch 1 for proper s0i3.2 entry, it was separated and is now merged in kernel
>  - Add patch to only run S0ix flows if shutdown succeeded which was suggested in
>    thread
>  - Adjust series for guidance from https://lkml.org/lkml/2020/12/13/15
>    * Revert i219-LM disallow-list.
>    * Drop all patches for systems tested by Dell in an allow list
>    * Increase ULP timeout to 1000ms
> 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 (4):
>   e1000e: Only run S0ix flows if shutdown succeeded
>   e1000e: bump up timeout to wait when ME un-configure ULP mode
>   Revert "e1000e: disable s0ix entry and exit flows for ME systems"
>   e1000e: Export S0ix flags to ethtool
> 
>  drivers/net/ethernet/intel/e1000e/e1000.h   |  1 +
>  drivers/net/ethernet/intel/e1000e/ethtool.c | 40 ++++++++++++++
>  drivers/net/ethernet/intel/e1000e/ich8lan.c |  4 +-
>  drivers/net/ethernet/intel/e1000e/netdev.c  | 59 ++++-----------------
>  4 files changed, 53 insertions(+), 51 deletions(-)
> 
> --
> 2.25.1
> 

Re: [PATCH 4/4] e1000e: Export S0ix flags to ethtool

[Hans de Goede]

Hi,

On 12/14/20 4:34 PM, Mario Limonciello wrote:
> This flag can be used by an end user to disable S0ix flows on a
> buggy system or by an OEM for development purposes.
> 
> If you need this flag to be persisted across reboots, it's suggested
> to use a udev rule to call adjust it until the kernel could have your
> configuration in a disallow list.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
>  drivers/net/ethernet/intel/e1000e/e1000.h   |  1 +
>  drivers/net/ethernet/intel/e1000e/ethtool.c | 40 +++++++++++++++++++++
>  drivers/net/ethernet/intel/e1000e/netdev.c  |  9 ++---
>  3 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
> index ba7a0f8f6937..5b2143f4b1f8 100644
> --- a/drivers/net/ethernet/intel/e1000e/e1000.h
> +++ b/drivers/net/ethernet/intel/e1000e/e1000.h
> @@ -436,6 +436,7 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca);
>  #define FLAG2_DFLT_CRC_STRIPPING          BIT(12)
>  #define FLAG2_CHECK_RX_HWTSTAMP           BIT(13)
>  #define FLAG2_CHECK_SYSTIM_OVERFLOW       BIT(14)
> +#define FLAG2_ENABLE_S0IX_FLOWS           BIT(15)
>  
>  #define E1000_RX_DESC_PS(R, i)	    \
>  	(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
> index 03215b0aee4b..eb683949ebfe 100644
> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> @@ -23,6 +23,13 @@ struct e1000_stats {
>  	int stat_offset;
>  };
>  
> +static const char e1000e_priv_flags_strings[][ETH_GSTRING_LEN] = {
> +#define E1000E_PRIV_FLAGS_S0IX_ENABLED	BIT(0)
> +	"s0ix-enabled",
> +};
> +
> +#define E1000E_PRIV_FLAGS_STR_LEN ARRAY_SIZE(e1000e_priv_flags_strings)
> +
>  #define E1000_STAT(str, m) { \
>  		.stat_string = str, \
>  		.type = E1000_STATS, \
> @@ -1776,6 +1783,8 @@ static int e1000e_get_sset_count(struct net_device __always_unused *netdev,
>  		return E1000_TEST_LEN;
>  	case ETH_SS_STATS:
>  		return E1000_STATS_LEN;
> +	case ETH_SS_PRIV_FLAGS:
> +		return E1000E_PRIV_FLAGS_STR_LEN;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -2097,6 +2106,10 @@ static void e1000_get_strings(struct net_device __always_unused *netdev,
>  			p += ETH_GSTRING_LEN;
>  		}
>  		break;
> +	case ETH_SS_PRIV_FLAGS:
> +		memcpy(data, e1000e_priv_flags_strings,
> +		       E1000E_PRIV_FLAGS_STR_LEN * ETH_GSTRING_LEN);
> +		break;
>  	}
>  }
>  
> @@ -2305,6 +2318,31 @@ static int e1000e_get_ts_info(struct net_device *netdev,
>  	return 0;
>  }
>  
> +static u32 e1000e_get_priv_flags(struct net_device *netdev)
> +{
> +	struct e1000_adapter *adapter = netdev_priv(netdev);
> +	u32 priv_flags = 0;
> +
> +	if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
> +		priv_flags |= E1000E_PRIV_FLAGS_S0IX_ENABLED;
> +
> +	return priv_flags;
> +}
> +
> +static int e1000e_set_priv_flags(struct net_device *netdev, u32 priv_flags)
> +{
> +	struct e1000_adapter *adapter = netdev_priv(netdev);
> +	unsigned int flags2 = adapter->flags2;
> +
> +	flags2 &= ~FLAG2_ENABLE_S0IX_FLOWS;
> +	if (priv_flags & E1000E_PRIV_FLAGS_S0IX_ENABLED)
> +		flags2 |= FLAG2_ENABLE_S0IX_FLOWS;
> +	if (flags2 != adapter->flags2)
> +		adapter->flags2 = flags2;


This will allow ethtool to enable the s0ix code on hw which does not
support this. I believe that this needs a

	if (hw->mac.type >= e1000_pch_cnp)

Check to avoid this scenario. And probably return -EINVAL when
a user tries to enable this on hw where it is not supported.

Regards,

Hans




> +
> +	return 0;
> +}
> +
>  static const struct ethtool_ops e1000_ethtool_ops = {
>  	.supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS,
>  	.get_drvinfo		= e1000_get_drvinfo,
> @@ -2336,6 +2374,8 @@ static const struct ethtool_ops e1000_ethtool_ops = {
>  	.set_eee		= e1000e_set_eee,
>  	.get_link_ksettings	= e1000_get_link_ksettings,
>  	.set_link_ksettings	= e1000_set_link_ksettings,
> +	.get_priv_flags		= e1000e_get_priv_flags,
> +	.set_priv_flags		= e1000e_set_priv_flags,
>  };
>  
>  void e1000e_set_ethtool_ops(struct net_device *netdev)
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index b9800ba2006c..e9b82c209c2d 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -6923,7 +6923,6 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev)
>  	struct net_device *netdev = pci_get_drvdata(to_pci_dev(dev));
>  	struct e1000_adapter *adapter = netdev_priv(netdev);
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct e1000_hw *hw = &adapter->hw;
>  	int rc;
>  
>  	e1000e_flush_lpic(pdev);
> @@ -6935,7 +6934,7 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev)
>  		e1000e_pm_thaw(dev);
>  	} else {
>  		/* Introduce S0ix implementation */
> -		if (hw->mac.type >= e1000_pch_cnp)
> +		if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
>  			e1000e_s0ix_entry_flow(adapter);
>  	}
>  
> @@ -6947,11 +6946,10 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev)
>  	struct net_device *netdev = pci_get_drvdata(to_pci_dev(dev));
>  	struct e1000_adapter *adapter = netdev_priv(netdev);
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct e1000_hw *hw = &adapter->hw;
>  	int rc;
>  
>  	/* Introduce S0ix implementation */
> -	if (hw->mac.type >= e1000_pch_cnp)
> +	if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
>  		e1000e_s0ix_exit_flow(adapter);
>  
>  	rc = __e1000_resume(pdev);
> @@ -7615,6 +7613,9 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (!(adapter->flags & FLAG_HAS_AMT))
>  		e1000e_get_hw_control(adapter);
>  
> +	if (hw->mac.type >= e1000_pch_cnp)
> +		adapter->flags2 |= FLAG2_ENABLE_S0IX_FLOWS;
> +
>  	strlcpy(netdev->name, "eth%d", sizeof(netdev->name));
>  	err = register_netdev(netdev);
>  	if (err)
> 

Re: Fw: [External] Re: [PATCH v4 0/4] Improve s0ix flows for systems i219LM

[Mark Pearson]

Thanks Hans

On 14/12/2020 13:31, Mark Pearson wrote:
> 
> 
> ------------------------------------------------------------------------
> *From:* Hans de Goede <hdegoede@redhat.com>
> *Sent:* December 14, 2020 13:24
> *To:* Mario Limonciello <mario.limonciello@dell.com>; Jeff Kirsher
> <jeffrey.t.kirsher@intel.com>; Tony Nguyen <anthony.l.nguyen@intel.com>;
> intel-wired-lan@lists.osuosl.org <intel-wired-lan@lists.osuosl.org>;
> David Miller <davem@davemloft.net>; Aaron Ma <aaron.ma@canonical.com>;
> Mark Pearson <mpearson@lenovo.com>
> *Cc:* linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>;
> Netdev <netdev@vger.kernel.org>; Alexander Duyck
> <alexander.duyck@gmail.com>; Jakub Kicinski <kuba@kernel.org>; Sasha
> Netfin <sasha.neftin@intel.com>; Aaron Brown <aaron.f.brown@intel.com>;
> Stefan Assmann <sassmann@redhat.com>; darcari@redhat.com
> <darcari@redhat.com>; Yijun.Shen@dell.com <Yijun.Shen@dell.com>;
> Perry.Yuan@dell.com <Perry.Yuan@dell.com>; anthony.wong@canonical.com
> <anthony.wong@canonical.com>
> *Subject:* [External] Re: [PATCH v4 0/4] Improve s0ix flows for systems
> i219LM
>  
> Hi All,
> 
<snip>
> 
> ###
> 
> I've added Mark Pearson from Lenovo to the Cc so that Lenovo
> can investigate this issue further.
> 
> Mark, this thread is about an issue with enabling S0ix support for
> e1000e (i219lm) controllers. This was enabled in the kernel a
> while ago, but then got disabled again on vPro / AMT enabled
> systems because on some systems (Lenovo X1C7 and now also X1C8)
> this lead to suspend/resume issues.
> 
> When AMT is active then there is a handover handshake for the
> OS to get access to the ethernet controller from the ME. The
> Intel folks have checked and the Windows driver is using a timeout
> of 1 second for this handshake, yet on Lenovo systems this is
> taking 2 seconds. This likely has something to do with the
> ME firmware on these Lenovo models, can you get the firmware
> team at Lenovo to investigate this further ?
Absolutely - I'll ask them to look into this again.

We did try to make progress with this previously - but it got a bit
stuck and hence the need for these patches....but I believe things may
have changed a bit so it's worth trying again

Mark

RE: [PATCH v4 0/4] Improve s0ix flows for systems i219LM

[Limonciello, Mario]

> Hi All,
> 
> Sasha (and the other intel-wired-lan folks), thank you for investigating this
> further and for coming up with a better solution.
> 
> Mario, thank you for implementing the new scheme.
> 

Sure.

> I've tested this patch set on a Lenovo X1C8 with vPRO and AMT enabled in the
> BIOS
> (the previous issues were soon on a X1C7).
> 
> I have good and bad news:
> 
> The good news is that after reverting the
> "e1000e: disable s0ix entry and exit flows for ME systems"
> I can reproduce the original issue on the X1C8 (I no longer have
> a X1C7 to test on).
> 
> The bad news is that increasing the timeout to 1 second does
> not fix the issue. Suspend/resume is still broken after one
> suspend/resume cycle, as described in the original bug-report:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1865570
> 
> More good news though, bumping the timeout to 250 poll iterations
> (approx 2.5 seconds) as done in Aaron Ma's original patch for
> this fixes this on the X1C8 just as it did on the X1C7
> (it takes 2 seconds for ULP_CONFIG_DONE to clear).
> 
> I've ran some extra tests and the poll loop succeeds on its
> first iteration when an ethernet-cable is connected. It seems
> that Lenovo's variant of the ME firmware waits up to 2 seconds
> for a link, causing the long wait for ULP_CONFIG_DONE to clear.
> 
> I think that for now the best fix would be to increase the timeout
> to 2.5 seconds as done in  Aaron Ma's original patch. Combined
> with a broken-firmware warning when we waited longer then 1 second,
> to make it clear that there is a firmware issue here and that
> the long wait / slow resume is not the fault of the driver.
> 

OK.  I've submitted v5 with this suggestion.

> ###
> 
> I've added Mark Pearson from Lenovo to the Cc so that Lenovo
> can investigate this issue further.
> 
> Mark, this thread is about an issue with enabling S0ix support for
> e1000e (i219lm) controllers. This was enabled in the kernel a
> while ago, but then got disabled again on vPro / AMT enabled
> systems because on some systems (Lenovo X1C7 and now also X1C8)
> this lead to suspend/resume issues.
> 
> When AMT is active then there is a handover handshake for the
> OS to get access to the ethernet controller from the ME. The
> Intel folks have checked and the Windows driver is using a timeout
> of 1 second for this handshake, yet on Lenovo systems this is
> taking 2 seconds. This likely has something to do with the
> ME firmware on these Lenovo models, can you get the firmware
> team at Lenovo to investigate this further ?
> 

Please be very careful with nomenclature.  AMT active, or AMT capable?
The goal for this series is to support AMT capable systems with an i219LM
where AMT has not been provisioned by the end user or organization.
OEMs do not ship systems with AMD provisioned.

I don't know that this series will work properly with AMT active, and
we will need more guidance from Intel's team to enable that feature.
Please lets keep that discussion separate from this series.


> Regards,
> 
> Hans
> 
> p.s.
> 
> I also have a small review remark on patch 4/4 I will
> reply to that patch separately.
> 

Thanks.

> 
> 
> 
> 
> 
> 
> 
> On 12/14/20 4:34 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.
> >
> > Per discussion with Intel architecture team this direction should be changed
> and
> > allow S0ix flows to be used by default.  This patch series includes
> directional
> > changes for their conclusions in https://lkml.org/lkml/2020/12/13/15.
> >
> > Changes from v3 to v4:
> >  - Drop patch 1 for proper s0i3.2 entry, it was separated and is now merged
> in kernel
> >  - Add patch to only run S0ix flows if shutdown succeeded which was
> suggested in
> >    thread
> >  - Adjust series for guidance from https://lkml.org/lkml/2020/12/13/15
> >    * Revert i219-LM disallow-list.
> >    * Drop all patches for systems tested by Dell in an allow list
> >    * Increase ULP timeout to 1000ms
> > 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 (4):
> >   e1000e: Only run S0ix flows if shutdown succeeded
> >   e1000e: bump up timeout to wait when ME un-configure ULP mode
> >   Revert "e1000e: disable s0ix entry and exit flows for ME systems"
> >   e1000e: Export S0ix flags to ethtool
> >
> >  drivers/net/ethernet/intel/e1000e/e1000.h   |  1 +
> >  drivers/net/ethernet/intel/e1000e/ethtool.c | 40 ++++++++++++++
> >  drivers/net/ethernet/intel/e1000e/ich8lan.c |  4 +-
> >  drivers/net/ethernet/intel/e1000e/netdev.c  | 59 ++++-----------------
> >  4 files changed, 53 insertions(+), 51 deletions(-)
> >
> > --
> > 2.25.1
> >

Re: [PATCH v4 0/4] Improve s0ix flows for systems i219LM

[Hans de Goede]

Hi,

On 12/14/20 8:36 PM, Limonciello, Mario wrote:
>> Hi All,
>>
>> Sasha (and the other intel-wired-lan folks), thank you for investigating this
>> further and for coming up with a better solution.
>>
>> Mario, thank you for implementing the new scheme.
>>
> 
> Sure.
> 
>> I've tested this patch set on a Lenovo X1C8 with vPRO and AMT enabled in the
>> BIOS
>> (the previous issues were soon on a X1C7).
>>
>> I have good and bad news:
>>
>> The good news is that after reverting the
>> "e1000e: disable s0ix entry and exit flows for ME systems"
>> I can reproduce the original issue on the X1C8 (I no longer have
>> a X1C7 to test on).
>>
>> The bad news is that increasing the timeout to 1 second does
>> not fix the issue. Suspend/resume is still broken after one
>> suspend/resume cycle, as described in the original bug-report:
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1865570
>>
>> More good news though, bumping the timeout to 250 poll iterations
>> (approx 2.5 seconds) as done in Aaron Ma's original patch for
>> this fixes this on the X1C8 just as it did on the X1C7
>> (it takes 2 seconds for ULP_CONFIG_DONE to clear).
>>
>> I've ran some extra tests and the poll loop succeeds on its
>> first iteration when an ethernet-cable is connected. It seems
>> that Lenovo's variant of the ME firmware waits up to 2 seconds
>> for a link, causing the long wait for ULP_CONFIG_DONE to clear.
>>
>> I think that for now the best fix would be to increase the timeout
>> to 2.5 seconds as done in  Aaron Ma's original patch. Combined
>> with a broken-firmware warning when we waited longer then 1 second,
>> to make it clear that there is a firmware issue here and that
>> the long wait / slow resume is not the fault of the driver.
>>
> 
> OK.  I've submitted v5 with this suggestion.
> 
>> ###
>>
>> I've added Mark Pearson from Lenovo to the Cc so that Lenovo
>> can investigate this issue further.
>>
>> Mark, this thread is about an issue with enabling S0ix support for
>> e1000e (i219lm) controllers. This was enabled in the kernel a
>> while ago, but then got disabled again on vPro / AMT enabled
>> systems because on some systems (Lenovo X1C7 and now also X1C8)
>> this lead to suspend/resume issues.
>>
>> When AMT is active then there is a handover handshake for the
>> OS to get access to the ethernet controller from the ME. The
>> Intel folks have checked and the Windows driver is using a timeout
>> of 1 second for this handshake, yet on Lenovo systems this is
>> taking 2 seconds. This likely has something to do with the
>> ME firmware on these Lenovo models, can you get the firmware
>> team at Lenovo to investigate this further ?
>>
> 
> Please be very careful with nomenclature.  AMT active, or AMT capable?
> The goal for this series is to support AMT capable systems with an i219LM
> where AMT has not been provisioned by the end user or organization.
> OEMs do not ship systems with AMD provisioned.

Ah, sorry about that. What I meant with "active" is set to "Enabled"
in the BIOS.

Also FWIW I just tried disabling AMT in the BIOS (using the "Disabled"
option, not the "Permanently Disabled" option) on the Lenovo X1 Carbon
8th gen, but that does not make a difference.

It still takes 2 seconds for ULP_CONFIG_DONE to clear even with AMT
set to "Disabled" in the BIOS :|

Regards,

Hans

Re: Fw: [External] Re: [PATCH v4 0/4] Improve s0ix flows for systems i219LM

[Neftin, Sasha]

On 12/14/2020 20:40, Mark Pearson wrote:
> Thanks Hans
> 
> On 14/12/2020 13:31, Mark Pearson wrote:
>>
>>
>> ------------------------------------------------------------------------
>> *From:* Hans de Goede <hdegoede@redhat.com>
>> *Sent:* December 14, 2020 13:24
>> *To:* Mario Limonciello <mario.limonciello@dell.com>; Jeff Kirsher
>> <jeffrey.t.kirsher@intel.com>; Tony Nguyen <anthony.l.nguyen@intel.com>;
>> intel-wired-lan@lists.osuosl.org <intel-wired-lan@lists.osuosl.org>;
>> David Miller <davem@davemloft.net>; Aaron Ma <aaron.ma@canonical.com>;
>> Mark Pearson <mpearson@lenovo.com>
>> *Cc:* linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>;
>> Netdev <netdev@vger.kernel.org>; Alexander Duyck
>> <alexander.duyck@gmail.com>; Jakub Kicinski <kuba@kernel.org>; Sasha
>> Netfin <sasha.neftin@intel.com>; Aaron Brown <aaron.f.brown@intel.com>;
>> Stefan Assmann <sassmann@redhat.com>; darcari@redhat.com
>> <darcari@redhat.com>; Yijun.Shen@dell.com <Yijun.Shen@dell.com>;
>> Perry.Yuan@dell.com <Perry.Yuan@dell.com>; anthony.wong@canonical.com
>> <anthony.wong@canonical.com>
>> *Subject:* [External] Re: [PATCH v4 0/4] Improve s0ix flows for systems
>> i219LM
>>   
>> Hi All,
>>
> <snip>
>>
>> ###
>>
>> I've added Mark Pearson from Lenovo to the Cc so that Lenovo
>> can investigate this issue further.
>>
>> Mark, this thread is about an issue with enabling S0ix support for
>> e1000e (i219lm) controllers. This was enabled in the kernel a
>> while ago, but then got disabled again on vPro / AMT enabled
>> systems because on some systems (Lenovo X1C7 and now also X1C8)
>> this lead to suspend/resume issues.
>>
>> When AMT is active then there is a handover handshake for the
>> OS to get access to the ethernet controller from the ME. The
>> Intel folks have checked and the Windows driver is using a timeout
>> of 1 second for this handshake, yet on Lenovo systems this is
>> taking 2 seconds. This likely has something to do with the
>> ME firmware on these Lenovo models, can you get the firmware
>> team at Lenovo to investigate this further ?
> Absolutely - I'll ask them to look into this again.
> 
we need to explain why on Windows systems required 1s and on Linux 
systems up to 2.5s - otherwise it is not reliable approach - you will 
encounter others buggy system.
(ME not POR on the Linux systems - is only one possible answer)
> We did try to make progress with this previously - but it got a bit
> stuck and hence the need for these patches....but I believe things may
> have changed a bit so it's worth trying again
> 
> Mark
> 
Sasha

RE: Fw: [External] Re: [PATCH v4 0/4] Improve s0ix flows for systems i219LM

[Limonciello, Mario]

> > Absolutely - I'll ask them to look into this again.
> >
> we need to explain why on Windows systems required 1s and on Linux
> systems up to 2.5s - otherwise it is not reliable approach - you will
> encounter others buggy system.
> (ME not POR on the Linux systems - is only one possible answer)

Sasha: In your opinion does this information need to block the series?
or can we follow up with more changes later on as more information becomes
available?

For now v5 of the series extends the timeout but at least makes a mention
that there appears to be a firmware bug when more than 1 second is taken.

Re: Fw: [External] Re: [PATCH v4 0/4] Improve s0ix flows for systems i219LM

[Neftin, Sasha]

On 12/15/2020 19:20, Limonciello, Mario wrote:
> 
>>> Absolutely - I'll ask them to look into this again.
>>>
>> we need to explain why on Windows systems required 1s and on Linux
>> systems up to 2.5s - otherwise it is not reliable approach - you will
>> encounter others buggy system.
>> (ME not POR on the Linux systems - is only one possible answer)
> 
> Sasha: In your opinion does this information need to block the series?
> or can we follow up with more changes later on as more information becomes
> available?
> 
I do not think this should block the patches series.
> For now v5 of the series extends the timeout but at least makes a mention
> that there appears to be a firmware bug when more than 1 second is taken.
>