[PATCH v3 0/7] Improve s0ix flows for systems i219LM

[PATCH v3 0/7] 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.

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.

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

[PATCH v3 1/7] e1000e: fix S0ix flow to allow S0i3.2 subset entry

[Mario Limonciello]

From: Vitaly Lifshits <vitaly.lifshits@intel.com>

Changed a configuration in the flows to align with
architecture requirements to achieve S0i3.2 substate.

Also fixed a typo in the previous commit 632fbd5eb5b0
("e1000e: fix S0ix flows for cable connected case").

Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index b30f00891c03..128ab6898070 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6475,13 +6475,13 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
 
 	/* Ungate PGCB clock */
 	mac_data = er32(FEXTNVM9);
-	mac_data |= BIT(28);
+	mac_data &= ~BIT(28);
 	ew32(FEXTNVM9, mac_data);
 
 	/* Enable K1 off to enable mPHY Power Gating */
 	mac_data = er32(FEXTNVM6);
 	mac_data |= BIT(31);
-	ew32(FEXTNVM12, mac_data);
+	ew32(FEXTNVM6, mac_data);
 
 	/* Enable mPHY power gating for any link and speed */
 	mac_data = er32(FEXTNVM8);
@@ -6525,11 +6525,11 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
 	/* Disable K1 off */
 	mac_data = er32(FEXTNVM6);
 	mac_data &= ~BIT(31);
-	ew32(FEXTNVM12, mac_data);
+	ew32(FEXTNVM6, mac_data);
 
 	/* Disable Ungate PGCB clock */
 	mac_data = er32(FEXTNVM9);
-	mac_data &= ~BIT(28);
+	mac_data |= BIT(28);
 	ew32(FEXTNVM9, mac_data);
 
 	/* Cancel not waking from dynamic
-- 
2.25.1

Re: [PATCH v3 1/7] e1000e: fix S0ix flow to allow S0i3.2 subset entry

[Mario Limonciello]

On 12/4/20 2:09 PM, Mario Limonciello wrote:
> From: Vitaly Lifshits <vitaly.lifshits@intel.com>
>
> Changed a configuration in the flows to align with
> architecture requirements to achieve S0i3.2 substate.
>
> Also fixed a typo in the previous commit 632fbd5eb5b0
> ("e1000e: fix S0ix flows for cable connected case").
>
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>   drivers/net/ethernet/intel/e1000e/netdev.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

I realize that the series is still under discussion, but I intentionally 
moved this
patch to the front of the series so it can be pulled in even if the 
others are still
discussed.

@David Miller:
This particular patch is more important than the rest.  It actually 
fixes issues
on the non-ME i219V as well.  Can this one be queued up and we can keep
discussing the rest?

> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index b30f00891c03..128ab6898070 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -6475,13 +6475,13 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
>   
>   	/* Ungate PGCB clock */
>   	mac_data = er32(FEXTNVM9);
> -	mac_data |= BIT(28);
> +	mac_data &= ~BIT(28);
>   	ew32(FEXTNVM9, mac_data);
>   
>   	/* Enable K1 off to enable mPHY Power Gating */
>   	mac_data = er32(FEXTNVM6);
>   	mac_data |= BIT(31);
> -	ew32(FEXTNVM12, mac_data);
> +	ew32(FEXTNVM6, mac_data);
>   
>   	/* Enable mPHY power gating for any link and speed */
>   	mac_data = er32(FEXTNVM8);
> @@ -6525,11 +6525,11 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
>   	/* Disable K1 off */
>   	mac_data = er32(FEXTNVM6);
>   	mac_data &= ~BIT(31);
> -	ew32(FEXTNVM12, mac_data);
> +	ew32(FEXTNVM6, mac_data);
>   
>   	/* Disable Ungate PGCB clock */
>   	mac_data = er32(FEXTNVM9);
> -	mac_data &= ~BIT(28);
> +	mac_data |= BIT(28);
>   	ew32(FEXTNVM9, mac_data);
>   
>   	/* Cancel not waking from dynamic
-- 
*Mario Limonciello*
*Dell*| CPG Software Engineering
*office*+1 512 723 0582

Re: [PATCH v3 1/7] e1000e: fix S0ix flow to allow S0i3.2 subset entry

[Jakub Kicinski]

On Tue, 8 Dec 2020 11:24:17 -0600 Mario Limonciello wrote:
> On 12/4/20 2:09 PM, Mario Limonciello wrote:
> > From: Vitaly Lifshits <vitaly.lifshits@intel.com>
> >
> > Changed a configuration in the flows to align with
> > architecture requirements to achieve S0i3.2 substate.
> >
> > Also fixed a typo in the previous commit 632fbd5eb5b0
> > ("e1000e: fix S0ix flows for cable connected case").
> >
> > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> > Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> >   drivers/net/ethernet/intel/e1000e/netdev.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)  
> 
> I realize that the series is still under discussion, but I intentionally 
> moved this
> patch to the front of the series so it can be pulled in even if the 
> others are still
> discussed.
> 
> @David Miller:
> This particular patch is more important than the rest.  It actually 
> fixes issues
> on the non-ME i219V as well.  Can this one be queued up and we can keep
> discussing the rest?

Not sure Dave will notice this discussion, best if you repost this patch
separately. If it's a fix that should be backported to stable make sure
you add a Fixes tag.

[PATCH v3 2/7] e1000e: Move all S0ix related code into its own source file

[Mario Limonciello]

Introduce a flag to indicate the device should be using the S0ix
flows and use this flag to run those functions.

Splitting the code to it's own file will make future heuristics
more self contained.

Tested-by: Yijun Shen <yijun.shen@dell.com>
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/net/ethernet/intel/e1000e/Makefile |   2 +-
 drivers/net/ethernet/intel/e1000e/e1000.h  |   4 +
 drivers/net/ethernet/intel/e1000e/netdev.c | 272 +-------------------
 drivers/net/ethernet/intel/e1000e/s0ix.c   | 280 +++++++++++++++++++++
 4 files changed, 290 insertions(+), 268 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/e1000e/s0ix.c

diff --git a/drivers/net/ethernet/intel/e1000e/Makefile b/drivers/net/ethernet/intel/e1000e/Makefile
index 44e58b6e7660..f2332c01f86c 100644
--- a/drivers/net/ethernet/intel/e1000e/Makefile
+++ b/drivers/net/ethernet/intel/e1000e/Makefile
@@ -9,5 +9,5 @@ obj-$(CONFIG_E1000E) += e1000e.o
 
 e1000e-objs := 82571.o ich8lan.o 80003es2lan.o \
 	       mac.o manage.o nvm.o phy.o \
-	       param.o ethtool.o netdev.o ptp.o
+	       param.o ethtool.o netdev.o s0ix.o ptp.o
 
diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index ba7a0f8f6937..b13f956285ae 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]))
@@ -462,6 +463,9 @@ enum latency_range {
 extern char e1000e_driver_name[];
 
 void e1000e_check_options(struct e1000_adapter *adapter);
+void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter);
+void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter);
+void e1000e_maybe_enable_s0ix(struct e1000_adapter *adapter);
 void e1000e_set_ethtool_ops(struct net_device *netdev);
 
 int e1000e_open(struct net_device *netdev);
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 128ab6898070..cd9839e86615 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
@@ -6368,228 +6329,6 @@ static void e1000e_flush_lpic(struct pci_dev *pdev)
 	pm_runtime_put_sync(netdev->dev.parent);
 }
 
-/* S0ix implementation */
-static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
-{
-	struct e1000_hw *hw = &adapter->hw;
-	u32 mac_data;
-	u16 phy_data;
-
-	/* Disable the periodic inband message,
-	 * don't request PCIe clock in K1 page770_17[10:9] = 10b
-	 */
-	e1e_rphy(hw, HV_PM_CTRL, &phy_data);
-	phy_data &= ~HV_PM_CTRL_K1_CLK_REQ;
-	phy_data |= BIT(10);
-	e1e_wphy(hw, HV_PM_CTRL, phy_data);
-
-	/* Make sure we don't exit K1 every time a new packet arrives
-	 * 772_29[5] = 1 CS_Mode_Stay_In_K1
-	 */
-	e1e_rphy(hw, I217_CGFREG, &phy_data);
-	phy_data |= BIT(5);
-	e1e_wphy(hw, I217_CGFREG, phy_data);
-
-	/* Change the MAC/PHY interface to SMBus
-	 * Force the SMBus in PHY page769_23[0] = 1
-	 * Force the SMBus in MAC CTRL_EXT[11] = 1
-	 */
-	e1e_rphy(hw, CV_SMB_CTRL, &phy_data);
-	phy_data |= CV_SMB_CTRL_FORCE_SMBUS;
-	e1e_wphy(hw, CV_SMB_CTRL, phy_data);
-	mac_data = er32(CTRL_EXT);
-	mac_data |= E1000_CTRL_EXT_FORCE_SMBUS;
-	ew32(CTRL_EXT, mac_data);
-
-	/* DFT control: PHY bit: page769_20[0] = 1
-	 * Gate PPW via EXTCNF_CTRL - set 0x0F00[7] = 1
-	 */
-	e1e_rphy(hw, I82579_DFT_CTRL, &phy_data);
-	phy_data |= BIT(0);
-	e1e_wphy(hw, I82579_DFT_CTRL, phy_data);
-
-	mac_data = er32(EXTCNF_CTRL);
-	mac_data |= E1000_EXTCNF_CTRL_GATE_PHY_CFG;
-	ew32(EXTCNF_CTRL, mac_data);
-
-	/* Check MAC Tx/Rx packet buffer pointers.
-	 * Reset MAC Tx/Rx packet buffer pointers to suppress any
-	 * pending traffic indication that would prevent power gating.
-	 */
-	mac_data = er32(TDFH);
-	if (mac_data)
-		ew32(TDFH, 0);
-	mac_data = er32(TDFT);
-	if (mac_data)
-		ew32(TDFT, 0);
-	mac_data = er32(TDFHS);
-	if (mac_data)
-		ew32(TDFHS, 0);
-	mac_data = er32(TDFTS);
-	if (mac_data)
-		ew32(TDFTS, 0);
-	mac_data = er32(TDFPC);
-	if (mac_data)
-		ew32(TDFPC, 0);
-	mac_data = er32(RDFH);
-	if (mac_data)
-		ew32(RDFH, 0);
-	mac_data = er32(RDFT);
-	if (mac_data)
-		ew32(RDFT, 0);
-	mac_data = er32(RDFHS);
-	if (mac_data)
-		ew32(RDFHS, 0);
-	mac_data = er32(RDFTS);
-	if (mac_data)
-		ew32(RDFTS, 0);
-	mac_data = er32(RDFPC);
-	if (mac_data)
-		ew32(RDFPC, 0);
-
-	/* Enable the Dynamic Power Gating in the MAC */
-	mac_data = er32(FEXTNVM7);
-	mac_data |= BIT(22);
-	ew32(FEXTNVM7, mac_data);
-
-	/* Disable the time synchronization clock */
-	mac_data = er32(FEXTNVM7);
-	mac_data |= BIT(31);
-	mac_data &= ~BIT(0);
-	ew32(FEXTNVM7, mac_data);
-
-	/* Dynamic Power Gating Enable */
-	mac_data = er32(CTRL_EXT);
-	mac_data |= BIT(3);
-	ew32(CTRL_EXT, mac_data);
-
-	/* Disable disconnected cable conditioning for Power Gating */
-	mac_data = er32(DPGFR);
-	mac_data |= BIT(2);
-	ew32(DPGFR, mac_data);
-
-	/* Don't wake from dynamic Power Gating with clock request */
-	mac_data = er32(FEXTNVM12);
-	mac_data |= BIT(12);
-	ew32(FEXTNVM12, mac_data);
-
-	/* Ungate PGCB clock */
-	mac_data = er32(FEXTNVM9);
-	mac_data &= ~BIT(28);
-	ew32(FEXTNVM9, mac_data);
-
-	/* Enable K1 off to enable mPHY Power Gating */
-	mac_data = er32(FEXTNVM6);
-	mac_data |= BIT(31);
-	ew32(FEXTNVM6, mac_data);
-
-	/* Enable mPHY power gating for any link and speed */
-	mac_data = er32(FEXTNVM8);
-	mac_data |= BIT(9);
-	ew32(FEXTNVM8, mac_data);
-
-	/* Enable the Dynamic Clock Gating in the DMA and MAC */
-	mac_data = er32(CTRL_EXT);
-	mac_data |= E1000_CTRL_EXT_DMA_DYN_CLK_EN;
-	ew32(CTRL_EXT, mac_data);
-
-	/* No MAC DPG gating SLP_S0 in modern standby
-	 * Switch the logic of the lanphypc to use PMC counter
-	 */
-	mac_data = er32(FEXTNVM5);
-	mac_data |= BIT(7);
-	ew32(FEXTNVM5, mac_data);
-}
-
-static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
-{
-	struct e1000_hw *hw = &adapter->hw;
-	u32 mac_data;
-	u16 phy_data;
-
-	/* Disable the Dynamic Power Gating in the MAC */
-	mac_data = er32(FEXTNVM7);
-	mac_data &= 0xFFBFFFFF;
-	ew32(FEXTNVM7, mac_data);
-
-	/* Enable the time synchronization clock */
-	mac_data = er32(FEXTNVM7);
-	mac_data |= BIT(0);
-	ew32(FEXTNVM7, mac_data);
-
-	/* Disable mPHY power gating for any link and speed */
-	mac_data = er32(FEXTNVM8);
-	mac_data &= ~BIT(9);
-	ew32(FEXTNVM8, mac_data);
-
-	/* Disable K1 off */
-	mac_data = er32(FEXTNVM6);
-	mac_data &= ~BIT(31);
-	ew32(FEXTNVM6, mac_data);
-
-	/* Disable Ungate PGCB clock */
-	mac_data = er32(FEXTNVM9);
-	mac_data |= BIT(28);
-	ew32(FEXTNVM9, mac_data);
-
-	/* Cancel not waking from dynamic
-	 * Power Gating with clock request
-	 */
-	mac_data = er32(FEXTNVM12);
-	mac_data &= ~BIT(12);
-	ew32(FEXTNVM12, mac_data);
-
-	/* Cancel disable disconnected cable conditioning
-	 * for Power Gating
-	 */
-	mac_data = er32(DPGFR);
-	mac_data &= ~BIT(2);
-	ew32(DPGFR, mac_data);
-
-	/* Disable Dynamic Power Gating */
-	mac_data = er32(CTRL_EXT);
-	mac_data &= 0xFFFFFFF7;
-	ew32(CTRL_EXT, mac_data);
-
-	/* Disable the Dynamic Clock Gating in the DMA and MAC */
-	mac_data = er32(CTRL_EXT);
-	mac_data &= 0xFFF7FFFF;
-	ew32(CTRL_EXT, mac_data);
-
-	/* Revert the lanphypc logic to use the internal Gbe counter
-	 * and not the PMC counter
-	 */
-	mac_data = er32(FEXTNVM5);
-	mac_data &= 0xFFFFFF7F;
-	ew32(FEXTNVM5, mac_data);
-
-	/* Enable the periodic inband message,
-	 * Request PCIe clock in K1 page770_17[10:9] =01b
-	 */
-	e1e_rphy(hw, HV_PM_CTRL, &phy_data);
-	phy_data &= 0xFBFF;
-	phy_data |= HV_PM_CTRL_K1_CLK_REQ;
-	e1e_wphy(hw, HV_PM_CTRL, phy_data);
-
-	/* Return back configuration
-	 * 772_29[5] = 0 CS_Mode_Stay_In_K1
-	 */
-	e1e_rphy(hw, I217_CGFREG, &phy_data);
-	phy_data &= 0xFFDF;
-	e1e_wphy(hw, I217_CGFREG, phy_data);
-
-	/* Change the MAC/PHY interface to Kumeran
-	 * Unforce the SMBus in PHY page769_23[0] = 0
-	 * Unforce the SMBus in MAC CTRL_EXT[11] = 0
-	 */
-	e1e_rphy(hw, CV_SMB_CTRL, &phy_data);
-	phy_data &= ~CV_SMB_CTRL_FORCE_SMBUS;
-	e1e_wphy(hw, CV_SMB_CTRL, phy_data);
-	mac_data = er32(CTRL_EXT);
-	mac_data &= ~E1000_CTRL_EXT_FORCE_SMBUS;
-	ew32(CTRL_EXT, mac_data);
-}
-
 static int e1000e_pm_freeze(struct device *dev)
 {
 	struct net_device *netdev = dev_get_drvdata(dev);
@@ -6962,7 +6701,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);
@@ -6974,8 +6712,7 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev)
 		e1000e_pm_thaw(dev);
 
 	/* Introduce S0ix implementation */
-	if (hw->mac.type >= e1000_pch_cnp &&
-	    !e1000e_check_me(hw->adapter->pdev->device))
+	if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
 		e1000e_s0ix_entry_flow(adapter);
 
 	return rc;
@@ -6986,12 +6723,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 &&
-	    !e1000e_check_me(hw->adapter->pdev->device))
+	if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
 		e1000e_s0ix_exit_flow(adapter);
 
 	rc = __e1000_resume(pdev);
@@ -7655,6 +7390,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);
 
+	/* use heuristics to decide whether to enable s0ix flows */
+	e1000e_maybe_enable_s0ix(adapter);
+
 	strlcpy(netdev->name, "eth%d", sizeof(netdev->name));
 	err = register_netdev(netdev);
 	if (err)
diff --git a/drivers/net/ethernet/intel/e1000e/s0ix.c b/drivers/net/ethernet/intel/e1000e/s0ix.c
new file mode 100644
index 000000000000..c3013edbd9e4
--- /dev/null
+++ b/drivers/net/ethernet/intel/e1000e/s0ix.c
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 1999 - 2018 Intel Corporation. */
+
+#include <linux/netdevice.h>
+
+#include "e1000.h"
+
+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;
+}
+
+void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	u32 mac_data;
+	u16 phy_data;
+
+	/* Disable the periodic inband message,
+	 * don't request PCIe clock in K1 page770_17[10:9] = 10b
+	 */
+	e1e_rphy(hw, HV_PM_CTRL, &phy_data);
+	phy_data &= ~HV_PM_CTRL_K1_CLK_REQ;
+	phy_data |= BIT(10);
+	e1e_wphy(hw, HV_PM_CTRL, phy_data);
+
+	/* Make sure we don't exit K1 every time a new packet arrives
+	 * 772_29[5] = 1 CS_Mode_Stay_In_K1
+	 */
+	e1e_rphy(hw, I217_CGFREG, &phy_data);
+	phy_data |= BIT(5);
+	e1e_wphy(hw, I217_CGFREG, phy_data);
+
+	/* Change the MAC/PHY interface to SMBus
+	 * Force the SMBus in PHY page769_23[0] = 1
+	 * Force the SMBus in MAC CTRL_EXT[11] = 1
+	 */
+	e1e_rphy(hw, CV_SMB_CTRL, &phy_data);
+	phy_data |= CV_SMB_CTRL_FORCE_SMBUS;
+	e1e_wphy(hw, CV_SMB_CTRL, phy_data);
+	mac_data = er32(CTRL_EXT);
+	mac_data |= E1000_CTRL_EXT_FORCE_SMBUS;
+	ew32(CTRL_EXT, mac_data);
+
+	/* DFT control: PHY bit: page769_20[0] = 1
+	 * Gate PPW via EXTCNF_CTRL - set 0x0F00[7] = 1
+	 */
+	e1e_rphy(hw, I82579_DFT_CTRL, &phy_data);
+	phy_data |= BIT(0);
+	e1e_wphy(hw, I82579_DFT_CTRL, phy_data);
+
+	mac_data = er32(EXTCNF_CTRL);
+	mac_data |= E1000_EXTCNF_CTRL_GATE_PHY_CFG;
+	ew32(EXTCNF_CTRL, mac_data);
+
+	/* Check MAC Tx/Rx packet buffer pointers.
+	 * Reset MAC Tx/Rx packet buffer pointers to suppress any
+	 * pending traffic indication that would prevent power gating.
+	 */
+	mac_data = er32(TDFH);
+	if (mac_data)
+		ew32(TDFH, 0);
+	mac_data = er32(TDFT);
+	if (mac_data)
+		ew32(TDFT, 0);
+	mac_data = er32(TDFHS);
+	if (mac_data)
+		ew32(TDFHS, 0);
+	mac_data = er32(TDFTS);
+	if (mac_data)
+		ew32(TDFTS, 0);
+	mac_data = er32(TDFPC);
+	if (mac_data)
+		ew32(TDFPC, 0);
+	mac_data = er32(RDFH);
+	if (mac_data)
+		ew32(RDFH, 0);
+	mac_data = er32(RDFT);
+	if (mac_data)
+		ew32(RDFT, 0);
+	mac_data = er32(RDFHS);
+	if (mac_data)
+		ew32(RDFHS, 0);
+	mac_data = er32(RDFTS);
+	if (mac_data)
+		ew32(RDFTS, 0);
+	mac_data = er32(RDFPC);
+	if (mac_data)
+		ew32(RDFPC, 0);
+
+	/* Enable the Dynamic Power Gating in the MAC */
+	mac_data = er32(FEXTNVM7);
+	mac_data |= BIT(22);
+	ew32(FEXTNVM7, mac_data);
+
+	/* Disable the time synchronization clock */
+	mac_data = er32(FEXTNVM7);
+	mac_data |= BIT(31);
+	mac_data &= ~BIT(0);
+	ew32(FEXTNVM7, mac_data);
+
+	/* Dynamic Power Gating Enable */
+	mac_data = er32(CTRL_EXT);
+	mac_data |= BIT(3);
+	ew32(CTRL_EXT, mac_data);
+
+	/* Disable disconnected cable conditioning for Power Gating */
+	mac_data = er32(DPGFR);
+	mac_data |= BIT(2);
+	ew32(DPGFR, mac_data);
+
+	/* Don't wake from dynamic Power Gating with clock request */
+	mac_data = er32(FEXTNVM12);
+	mac_data |= BIT(12);
+	ew32(FEXTNVM12, mac_data);
+
+	/* Ungate PGCB clock */
+	mac_data = er32(FEXTNVM9);
+	mac_data &= ~BIT(28);
+	ew32(FEXTNVM9, mac_data);
+
+	/* Enable K1 off to enable mPHY Power Gating */
+	mac_data = er32(FEXTNVM6);
+	mac_data |= BIT(31);
+	ew32(FEXTNVM6, mac_data);
+
+	/* Enable mPHY power gating for any link and speed */
+	mac_data = er32(FEXTNVM8);
+	mac_data |= BIT(9);
+	ew32(FEXTNVM8, mac_data);
+
+	/* Enable the Dynamic Clock Gating in the DMA and MAC */
+	mac_data = er32(CTRL_EXT);
+	mac_data |= E1000_CTRL_EXT_DMA_DYN_CLK_EN;
+	ew32(CTRL_EXT, mac_data);
+
+	/* No MAC DPG gating SLP_S0 in modern standby
+	 * Switch the logic of the lanphypc to use PMC counter
+	 */
+	mac_data = er32(FEXTNVM5);
+	mac_data |= BIT(7);
+	ew32(FEXTNVM5, mac_data);
+}
+
+void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	u32 mac_data;
+	u16 phy_data;
+
+	/* Disable the Dynamic Power Gating in the MAC */
+	mac_data = er32(FEXTNVM7);
+	mac_data &= 0xFFBFFFFF;
+	ew32(FEXTNVM7, mac_data);
+
+	/* Enable the time synchronization clock */
+	mac_data = er32(FEXTNVM7);
+	mac_data |= BIT(0);
+	ew32(FEXTNVM7, mac_data);
+
+	/* Disable mPHY power gating for any link and speed */
+	mac_data = er32(FEXTNVM8);
+	mac_data &= ~BIT(9);
+	ew32(FEXTNVM8, mac_data);
+
+	/* Disable K1 off */
+	mac_data = er32(FEXTNVM6);
+	mac_data &= ~BIT(31);
+	ew32(FEXTNVM6, mac_data);
+
+	/* Disable Ungate PGCB clock */
+	mac_data = er32(FEXTNVM9);
+	mac_data |= BIT(28);
+	ew32(FEXTNVM9, mac_data);
+
+	/* Cancel not waking from dynamic
+	 * Power Gating with clock request
+	 */
+	mac_data = er32(FEXTNVM12);
+	mac_data &= ~BIT(12);
+	ew32(FEXTNVM12, mac_data);
+
+	/* Cancel disable disconnected cable conditioning
+	 * for Power Gating
+	 */
+	mac_data = er32(DPGFR);
+	mac_data &= ~BIT(2);
+	ew32(DPGFR, mac_data);
+
+	/* Disable Dynamic Power Gating */
+	mac_data = er32(CTRL_EXT);
+	mac_data &= 0xFFFFFFF7;
+	ew32(CTRL_EXT, mac_data);
+
+	/* Disable the Dynamic Clock Gating in the DMA and MAC */
+	mac_data = er32(CTRL_EXT);
+	mac_data &= 0xFFF7FFFF;
+	ew32(CTRL_EXT, mac_data);
+
+	/* Revert the lanphypc logic to use the internal Gbe counter
+	 * and not the PMC counter
+	 */
+	mac_data = er32(FEXTNVM5);
+	mac_data &= 0xFFFFFF7F;
+	ew32(FEXTNVM5, mac_data);
+
+	/* Enable the periodic inband message,
+	 * Request PCIe clock in K1 page770_17[10:9] =01b
+	 */
+	e1e_rphy(hw, HV_PM_CTRL, &phy_data);
+	phy_data &= 0xFBFF;
+	phy_data |= HV_PM_CTRL_K1_CLK_REQ;
+	e1e_wphy(hw, HV_PM_CTRL, phy_data);
+
+	/* Return back configuration
+	 * 772_29[5] = 0 CS_Mode_Stay_In_K1
+	 */
+	e1e_rphy(hw, I217_CGFREG, &phy_data);
+	phy_data &= 0xFFDF;
+	e1e_wphy(hw, I217_CGFREG, phy_data);
+
+	/* Change the MAC/PHY interface to Kumeran
+	 * Unforce the SMBus in PHY page769_23[0] = 0
+	 * Unforce the SMBus in MAC CTRL_EXT[11] = 0
+	 */
+	e1e_rphy(hw, CV_SMB_CTRL, &phy_data);
+	phy_data &= ~CV_SMB_CTRL_FORCE_SMBUS;
+	e1e_wphy(hw, CV_SMB_CTRL, phy_data);
+	mac_data = er32(CTRL_EXT);
+	mac_data &= ~E1000_CTRL_EXT_FORCE_SMBUS;
+	ew32(CTRL_EXT, mac_data);
+}
+
+void e1000e_maybe_enable_s0ix(struct e1000_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	struct pci_dev *pdev = adapter->pdev;
+
+	/* require cannon point or later */
+	if (hw->mac.type < e1000_pch_cnp)
+		return;
+	/* turn off on ME configurations */
+	if (e1000e_check_me(pdev->device))
+		return;
+	adapter->flags2 |= FLAG2_ENABLE_S0IX_FLOWS;
+}
-- 
2.25.1

Re: [PATCH v3 2/7] e1000e: Move all S0ix related code into its own source file

[Alexander Duyck]

On Fri, Dec 4, 2020 at 12:09 PM Mario Limonciello
<mario.limonciello@dell.com> wrote:
>
> Introduce a flag to indicate the device should be using the S0ix
> flows and use this flag to run those functions.
>
> Splitting the code to it's own file will make future heuristics
> more self contained.
>
> Tested-by: Yijun Shen <yijun.shen@dell.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>

One minor issue pointed out below.

> ---
>  drivers/net/ethernet/intel/e1000e/Makefile |   2 +-
>  drivers/net/ethernet/intel/e1000e/e1000.h  |   4 +
>  drivers/net/ethernet/intel/e1000e/netdev.c | 272 +-------------------
>  drivers/net/ethernet/intel/e1000e/s0ix.c   | 280 +++++++++++++++++++++
>  4 files changed, 290 insertions(+), 268 deletions(-)
>  create mode 100644 drivers/net/ethernet/intel/e1000e/s0ix.c
>
> diff --git a/drivers/net/ethernet/intel/e1000e/Makefile b/drivers/net/ethernet/intel/e1000e/Makefile
> index 44e58b6e7660..f2332c01f86c 100644
> --- a/drivers/net/ethernet/intel/e1000e/Makefile
> +++ b/drivers/net/ethernet/intel/e1000e/Makefile
> @@ -9,5 +9,5 @@ obj-$(CONFIG_E1000E) += e1000e.o
>
>  e1000e-objs := 82571.o ich8lan.o 80003es2lan.o \
>                mac.o manage.o nvm.o phy.o \
> -              param.o ethtool.o netdev.o ptp.o
> +              param.o ethtool.o netdev.o s0ix.o ptp.o
>
> diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
> index ba7a0f8f6937..b13f956285ae 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]))
> @@ -462,6 +463,9 @@ enum latency_range {
>  extern char e1000e_driver_name[];
>
>  void e1000e_check_options(struct e1000_adapter *adapter);
> +void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter);
> +void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter);
> +void e1000e_maybe_enable_s0ix(struct e1000_adapter *adapter);
>  void e1000e_set_ethtool_ops(struct net_device *netdev);
>
>  int e1000e_open(struct net_device *netdev);
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 128ab6898070..cd9839e86615 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c

<snip>

>  static int e1000e_pm_freeze(struct device *dev)
>  {
>         struct net_device *netdev = dev_get_drvdata(dev);
> @@ -6962,7 +6701,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);
> @@ -6974,8 +6712,7 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev)
>                 e1000e_pm_thaw(dev);
>
>         /* Introduce S0ix implementation */
> -       if (hw->mac.type >= e1000_pch_cnp &&
> -           !e1000e_check_me(hw->adapter->pdev->device))
> +       if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
>                 e1000e_s0ix_entry_flow(adapter);

So the placement of this code raises some issues. It isn't a problem
with your patch but a bug in the driver that needs to be addressed. I
am assuming you only need to perform this flow if you successfully
froze the part. However this is doing it in all cases, which is why
the e1000e_pm_thaw is being called before you call this code. This is
something that should probably be an "else if" rather than a seperate
if statement.

>
>         return rc;
> @@ -6986,12 +6723,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 &&
> -           !e1000e_check_me(hw->adapter->pdev->device))
> +       if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
>                 e1000e_s0ix_exit_flow(adapter);
>
>         rc = __e1000_resume(pdev);
> @@ -7655,6 +7390,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);
>
> +       /* use heuristics to decide whether to enable s0ix flows */
> +       e1000e_maybe_enable_s0ix(adapter);
> +
>         strlcpy(netdev->name, "eth%d", sizeof(netdev->name));
>         err = register_netdev(netdev);
>         if (err)

[PATCH v3 3/7] e1000e: Export S0ix flags to ethtool

[Mario Limonciello]

This flag can be used for debugging and development purposes
including determining if S0ix flows work properly for a system
not currently recognized by heuristics.

Tested-by: Yijun Shen <yijun.shen@dell.com>
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 40 +++++++++++++++++++++
 1 file changed, 40 insertions(+)

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)
-- 
2.25.1

[PATCH v3 4/7] e1000e: Add Dell's Comet Lake systems into S0ix heuristics

[Mario Limonciello]

Dell's shipping Comet Lake Latitude and Precision systems containing i219LM are
properly configured and should use the S0ix flows.

Disabling s0ix entry and exit flows caused a regression in power consumption
over suspend to idle on these systems.

Fixes: e086ba2fccda ("e1000e: disable s0ix entry and exit flows for ME systems")
Tested-by: Yijun Shen <yijun.shen@dell.com>
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/net/ethernet/intel/e1000e/s0ix.c | 33 +++++++++++++++++++++---
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/s0ix.c b/drivers/net/ethernet/intel/e1000e/s0ix.c
index c3013edbd9e4..74043e80c32f 100644
--- a/drivers/net/ethernet/intel/e1000e/s0ix.c
+++ b/drivers/net/ethernet/intel/e1000e/s0ix.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
-/* Copyright(c) 1999 - 2018 Intel Corporation. */
+/* Copyright(c) 1999 - 2018 Intel Corporation.
+ * Copyright(c) 2020 Dell Inc.
+ */
 
 #include <linux/netdevice.h>
 
@@ -44,6 +46,26 @@ static bool e1000e_check_me(u16 device_id)
 	return false;
 }
 
+static bool e1000e_check_subsystem_allowlist(struct pci_dev *dev)
+{
+	if (dev->subsystem_vendor == PCI_VENDOR_ID_DELL) {
+		switch (dev->subsystem_device) {
+		case 0x099f: /* Latitude 5310 */
+		case 0x09a0: /* Latitude 5410 */
+		case 0x09c9: /* Latitude 5410 */
+		case 0x09a1: /* Latitude 5510 */
+		case 0x09a2: /* Precision 3550 */
+		case 0x09c0: /* Latitude 5411 */
+		case 0x09c1: /* Latitude 5511 */
+		case 0x09c2: /* Precision 3551 */
+		case 0x09c3: /* Precision 7550 */
+		case 0x09c4: /* Precision 7750 */
+			return true;
+		}
+	}
+	return false;
+}
+
 void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
@@ -273,8 +295,11 @@ void e1000e_maybe_enable_s0ix(struct e1000_adapter *adapter)
 	/* require cannon point or later */
 	if (hw->mac.type < e1000_pch_cnp)
 		return;
-	/* turn off on ME configurations */
-	if (e1000e_check_me(pdev->device))
-		return;
+	/* check for allowlist of systems */
+	if (!e1000e_check_subsystem_allowlist(pdev)) {
+		/* turn off on ME configurations */
+		if (e1000e_check_me(pdev->device))
+			return;
+	}
 	adapter->flags2 |= FLAG2_ENABLE_S0IX_FLOWS;
 }
-- 
2.25.1

[PATCH v3 5/7] e1000e: Add more Dell CML systems into S0ix heuristics

[Mario Limonciello]

These Comet Lake systems are not yet released, but have been validated
on pre-release hardware.

This is being submitted separately from released hardware in case of
a regression between pre-release and release hardware so this commit
can be reverted alone.

Tested-by: Yijun Shen <yijun.shen@dell.com>
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/net/ethernet/intel/e1000e/s0ix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/s0ix.c b/drivers/net/ethernet/intel/e1000e/s0ix.c
index 74043e80c32f..0dd2e2702ebb 100644
--- a/drivers/net/ethernet/intel/e1000e/s0ix.c
+++ b/drivers/net/ethernet/intel/e1000e/s0ix.c
@@ -60,6 +60,9 @@ static bool e1000e_check_subsystem_allowlist(struct pci_dev *dev)
 		case 0x09c2: /* Precision 3551 */
 		case 0x09c3: /* Precision 7550 */
 		case 0x09c4: /* Precision 7750 */
+		case 0x0a40: /* Notebook 0x0a40 */
+		case 0x0a41: /* Notebook 0x0a41 */
+		case 0x0a42: /* Notebook 0x0a42 */
 			return true;
 		}
 	}
-- 
2.25.1

[PATCH v3 6/7] e1000e: Add Dell TGL desktop systems into S0ix heuristics

[Mario Limonciello]

These Tiger Lake systems are not yet released, but have been validated
on pre-release hardware.

This is being submitted separately from released hardware in case of
a regression between pre-release and release hardware so this commit
can be reverted alone.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/net/ethernet/intel/e1000e/s0ix.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/s0ix.c b/drivers/net/ethernet/intel/e1000e/s0ix.c
index 0dd2e2702ebb..cc04aeaa2292 100644
--- a/drivers/net/ethernet/intel/e1000e/s0ix.c
+++ b/drivers/net/ethernet/intel/e1000e/s0ix.c
@@ -63,6 +63,8 @@ static bool e1000e_check_subsystem_allowlist(struct pci_dev *dev)
 		case 0x0a40: /* Notebook 0x0a40 */
 		case 0x0a41: /* Notebook 0x0a41 */
 		case 0x0a42: /* Notebook 0x0a42 */
+		case 0x0a2e: /* Desktop  0x0a2e */
+		case 0x0a30: /* Desktop  0x0a30 */
 			return true;
 		}
 	}
-- 
2.25.1

[PATCH v3 7/7] e1000e: Add another Dell TGL notebook system into S0ix heuristics

[Mario Limonciello]

This Tiger Lake system is not yet released, but has been validated
on pre-release hardware.

This is being submitted separately from released hardware in case of
a regression between pre-release and release hardware so this commit
can be reverted alone.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/net/ethernet/intel/e1000e/s0ix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/e1000e/s0ix.c b/drivers/net/ethernet/intel/e1000e/s0ix.c
index cc04aeaa2292..3f2985fac67c 100644
--- a/drivers/net/ethernet/intel/e1000e/s0ix.c
+++ b/drivers/net/ethernet/intel/e1000e/s0ix.c
@@ -63,6 +63,7 @@ static bool e1000e_check_subsystem_allowlist(struct pci_dev *dev)
 		case 0x0a40: /* Notebook 0x0a40 */
 		case 0x0a41: /* Notebook 0x0a41 */
 		case 0x0a42: /* Notebook 0x0a42 */
+		case 0x0a22: /* Notebook 0x0a22 */
 		case 0x0a2e: /* Desktop  0x0a2e */
 		case 0x0a30: /* Desktop  0x0a30 */
 			return true;
-- 
2.25.1

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Alexander Duyck]

On Fri, Dec 4, 2020 at 12:09 PM 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.
>
> 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.

The patches look good to me. Just need to address the minor issue that
seems to have been present prior to the introduction of this patch
set.

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

RE: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Limonciello, Mario]

> -----Original Message-----
> From: Alexander Duyck <alexander.duyck@gmail.com>
> Sent: Friday, December 4, 2020 15:27
> To: Limonciello, Mario
> Cc: Jeff Kirsher; Tony Nguyen; intel-wired-lan; LKML; Linux PM; Netdev; Jakub
> Kicinski; Sasha Netfin; Aaron Brown; Stefan Assmann; David Miller; David
> Arcari; Shen, Yijun; Yuan, Perry; anthony.wong@canonical.com
> Subject: Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM
> 
> 
> [EXTERNAL EMAIL]
> 
> On Fri, Dec 4, 2020 at 12:09 PM 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.
> >
> > 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.
> 
> The patches look good to me. Just need to address the minor issue that
> seems to have been present prior to the introduction of this patch
> set.
> 
> Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Thanks for your review.  Just some operational questions - since this previously
existed do you want me to re-spin the series to a v4 for this, or should it be
a follow up after the series?

If I respin it, would you prefer that change to occur at the start or end
of the series?

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Alexander Duyck]

On Fri, Dec 4, 2020 at 2:28 PM Limonciello, Mario
<Mario.Limonciello@dell.com> wrote:
>
> > -----Original Message-----
> > From: Alexander Duyck <alexander.duyck@gmail.com>
> > Sent: Friday, December 4, 2020 15:27
> > To: Limonciello, Mario
> > Cc: Jeff Kirsher; Tony Nguyen; intel-wired-lan; LKML; Linux PM; Netdev; Jakub
> > Kicinski; Sasha Netfin; Aaron Brown; Stefan Assmann; David Miller; David
> > Arcari; Shen, Yijun; Yuan, Perry; anthony.wong@canonical.com
> > Subject: Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On Fri, Dec 4, 2020 at 12:09 PM 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.
> > >
> > > 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.
> >
> > The patches look good to me. Just need to address the minor issue that
> > seems to have been present prior to the introduction of this patch
> > set.
> >
> > Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
>
> Thanks for your review.  Just some operational questions - since this previously
> existed do you want me to re-spin the series to a v4 for this, or should it be
> a follow up after the series?
>
> If I respin it, would you prefer that change to occur at the start or end
> of the series?

I don't need a respin, but if you are going to fix it you should
probably put out the patch as something like a 8/7. If you respin it
should happen near the start of the series as it is a bug you are
addressing.

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Jakub Kicinski]

On Fri, 4 Dec 2020 14:38:03 -0800 Alexander Duyck wrote:
> > > The patches look good to me. Just need to address the minor issue that
> > > seems to have been present prior to the introduction of this patch
> > > set.
> > >
> > > Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>  
> >
> > Thanks for your review.  Just some operational questions - since this previously
> > existed do you want me to re-spin the series to a v4 for this, or should it be
> > a follow up after the series?
> >
> > If I respin it, would you prefer that change to occur at the start or end
> > of the series?  
> 
> I don't need a respin, but if you are going to fix it you should
> probably put out the patch as something like a 8/7. If you respin it
> should happen near the start of the series as it is a bug you are
> addressing.

Don't we need that patch to be before this series so it can be
back ported easily? Or is it not really a bug?

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Alexander Duyck]

On Sat, Dec 5, 2020 at 3:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 4 Dec 2020 14:38:03 -0800 Alexander Duyck wrote:
> > > > The patches look good to me. Just need to address the minor issue that
> > > > seems to have been present prior to the introduction of this patch
> > > > set.
> > > >
> > > > Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
> > >
> > > Thanks for your review.  Just some operational questions - since this previously
> > > existed do you want me to re-spin the series to a v4 for this, or should it be
> > > a follow up after the series?
> > >
> > > If I respin it, would you prefer that change to occur at the start or end
> > > of the series?
> >
> > I don't need a respin, but if you are going to fix it you should
> > probably put out the patch as something like a 8/7. If you respin it
> > should happen near the start of the series as it is a bug you are
> > addressing.
>
> Don't we need that patch to be before this series so it can be
> back ported easily? Or is it not really a bug?

You're right. For backports it would make it easier to have the patch
be before the changes. As far as being a bug, it is one, but it isn't
an urgent bug as it is basically some bad exception handling so the
likelihood of seeing it should be quite low.

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Hans de Goede]

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

RE: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Limonciello, Mario]

> 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.

I was not happy with that decision either as it did cause regressions
on all of the "named" Comet Lake laptops that were in the market at
the time.  The "unnamed" ones are not yet released, and I don't feel
it's fair to call it a regression on "unreleased" hardware.

> 
> 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.

They were done based upon the discussion in that thread you linked and others.
If the owners of this driver feel it's possible/scalable to follow your proposal
I'm happy to resubmit a new v4 series with these sets of patches:

1) Fixup for the exception corner case referenced in this thread
2) Patch 1 from this series that fixes cable connected case
3) Increase the timeout (from your referenced link)
4) Revert the ME disallow list

> 
> 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.

I'm perfectly happy either way, my primary goal is that Dell's notebooks and
desktops that meet the architectural and firmware guidelines for appropriate
low power consumption over s0ix are not penalized.

> 
> 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.

I envisioned that it would evolve over time.  For example if by the time Dell
finished shipping new CML models it was deemed that all the CML hardware was done
properly it could instead by an allow list of Dell + Comet Point.
If all of Tiger Lake are done properly 'maybe' by the time the ML ships maybe it
could be an allow list of Dell + CML or newer.

But even if the heuristic changed - this particular configuration needs to be tested
on every single new model.  All of the notebooks that have a Tested-By clause were
checked by Dell and Dell's partners.

> 
> 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.

I don't have configuration deemed buggy.  Since you were commenting in that other
thread with the patch from Aaaron It sounds like you do. Can you perhaps check if
that proposal actually works?

> 
> 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.

If we collectively agree to keep either an allow list "or" disallow list at
all I think you need a way check whether enabling this feature works.

If we are making an assertion it will always work properly all the time, I agree
that there is no need for an ethtool flag.

> 
> 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.
> 

Can you or someone else with this model please check with a current kernel
w/ reverting ME check and adding the patch from Vitaly (included as patch 1
in my series)?

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

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Neftin, Sasha]

On 12/7/2020 17:41, Limonciello, Mario wrote:
>> 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.
> 
> I was not happy with that decision either as it did cause regressions
> on all of the "named" Comet Lake laptops that were in the market at
> the time.  The "unnamed" ones are not yet released, and I don't feel
> it's fair to call it a regression on "unreleased" hardware.
> 
>>
>> 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*.
>>
Officially CSME/ME not POR for Linux and we haven't interfrace to the 
ME. Nobody can tell how long (and why) ME will hold PHY access semaphore 
ant just increasing the resuming time (ULP configure) won't be solve the 
problem. This is not reliable approach.
I would agree users can add ME system on their responsibilities.
>> 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.
> 
> They were done based upon the discussion in that thread you linked and others.
> If the owners of this driver feel it's possible/scalable to follow your proposal
> I'm happy to resubmit a new v4 series with these sets of patches:
> 
> 1) Fixup for the exception corner case referenced in this thread
> 2) Patch 1 from this series that fixes cable connected case
> 3) Increase the timeout (from your referenced link)
> 4) Revert the ME disallow list
> 
>>
>> 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.
> 
> I'm perfectly happy either way, my primary goal is that Dell's notebooks and
> desktops that meet the architectural and firmware guidelines for appropriate
> low power consumption over s0ix are not penalized.
> 
>>
>> 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.
> 
> I envisioned that it would evolve over time.  For example if by the time Dell
> finished shipping new CML models it was deemed that all the CML hardware was done
> properly it could instead by an allow list of Dell + Comet Point.
> If all of Tiger Lake are done properly 'maybe' by the time the ML ships maybe it
> could be an allow list of Dell + CML or newer.
> 
> But even if the heuristic changed - this particular configuration needs to be tested
> on every single new model.  All of the notebooks that have a Tested-By clause were
> checked by Dell and Dell's partners.
> 
>>
>> 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.
> 
> I don't have configuration deemed buggy.  Since you were commenting in that other
> thread with the patch from Aaaron It sounds like you do. Can you perhaps check if
> that proposal actually works?
> 
>>
>> 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.
> 
> If we collectively agree to keep either an allow list "or" disallow list at
> all I think you need a way check whether enabling this feature works.
> 
> If we are making an assertion it will always work properly all the time, I agree
> that there is no need for an ethtool flag.
> 
>>
>> 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.
>>
> 
> Can you or someone else with this model please check with a current kernel
> w/ reverting ME check and adding the patch from Vitaly (included as patch 1
> in my series)?
> 
>> 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
>>>
>>>
> 
Thanks,
Sasha

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Hans de Goede]

Hi,

On 12/8/20 6:08 AM, Neftin, Sasha wrote:
> On 12/7/2020 17:41, Limonciello, Mario wrote:
>>> 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.
>>
>> I was not happy with that decision either as it did cause regressions
>> on all of the "named" Comet Lake laptops that were in the market at
>> the time.  The "unnamed" ones are not yet released, and I don't feel
>> it's fair to call it a regression on "unreleased" hardware.
>>
>>>
>>> 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*.
>>>
> Officially CSME/ME not POR for Linux and we haven't interfrace to the ME. Nobody can tell how long (and why) ME will hold PHY access semaphore ant just increasing the resuming time (ULP configure) won't be solve the problem. This is not reliable approach.
> I would agree users can add ME system on their responsibilities.

It is not clear to me what you are trying to say here.

Are you saying that you insist on keeping the e1000e_check_me check and
thus needlessly penalizing 100s of laptops models with higher
power-consumption unless these 100s of laptops are added manually
to an allow list for this?

I'm sorry but that is simply unacceptable, the maintenance burden
of that is just way too high.

Testing on the models where the timeout issue was first hit has
shown that increasing the timeout does actually fix it on those
models. Sure in theory the ME on some buggy model could hold the
semaphore even longer, but then the right thing would be to
have a deny-list for s0ix where we can add those buggy models
(none of which we have encountered sofar). Just like we have
denylist for buggy hw in other places in the kernel.

Maintaining an ever growing allow list for the *theoretical*
case of encountering a model where things do not work with
the increased timeout is not a workable and this not an
acceptable solution.

The initial addition of the e1000e_check_me check instead
of just going with the confirmed fix of bumping the timeout
was already highly controversial and should IMHO never have
been done.

Combining this with an ever-growing allow-list on which every
new laptop model needs to be added separately + a new
"s0ix-enabled" ethertool flag, which existence is basically
an admission that the allow-list approach is flawed goes
from controversial to just plain not acceptable.

Regards,

Hans



>>> 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.
>>
>> They were done based upon the discussion in that thread you linked and others.
>> If the owners of this driver feel it's possible/scalable to follow your proposal
>> I'm happy to resubmit a new v4 series with these sets of patches:
>>
>> 1) Fixup for the exception corner case referenced in this thread
>> 2) Patch 1 from this series that fixes cable connected case
>> 3) Increase the timeout (from your referenced link)
>> 4) Revert the ME disallow list
>>
>>>
>>> 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.
>>
>> I'm perfectly happy either way, my primary goal is that Dell's notebooks and
>> desktops that meet the architectural and firmware guidelines for appropriate
>> low power consumption over s0ix are not penalized.
>>
>>>
>>> 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.
>>
>> I envisioned that it would evolve over time.  For example if by the time Dell
>> finished shipping new CML models it was deemed that all the CML hardware was done
>> properly it could instead by an allow list of Dell + Comet Point.
>> If all of Tiger Lake are done properly 'maybe' by the time the ML ships maybe it
>> could be an allow list of Dell + CML or newer.
>>
>> But even if the heuristic changed - this particular configuration needs to be tested
>> on every single new model.  All of the notebooks that have a Tested-By clause were
>> checked by Dell and Dell's partners.
>>
>>>
>>> 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.
>>
>> I don't have configuration deemed buggy.  Since you were commenting in that other
>> thread with the patch from Aaaron It sounds like you do. Can you perhaps check if
>> that proposal actually works?
>>
>>>
>>> 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.
>>
>> If we collectively agree to keep either an allow list "or" disallow list at
>> all I think you need a way check whether enabling this feature works.
>>
>> If we are making an assertion it will always work properly all the time, I agree
>> that there is no need for an ethtool flag.
>>
>>>
>>> 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.
>>>
>>
>> Can you or someone else with this model please check with a current kernel
>> w/ reverting ME check and adding the patch from Vitaly (included as patch 1
>> in my series)?
>>
>>> 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
>>>>
>>>>
>>
> Thanks,
> Sasha
> 

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Alexander Duyck]

On Tue, Dec 8, 2020 at 1:30 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/8/20 6:08 AM, Neftin, Sasha wrote:
> > On 12/7/2020 17:41, Limonciello, Mario wrote:
> >>> 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.
> >>
> >> I was not happy with that decision either as it did cause regressions
> >> on all of the "named" Comet Lake laptops that were in the market at
> >> the time.  The "unnamed" ones are not yet released, and I don't feel
> >> it's fair to call it a regression on "unreleased" hardware.
> >>
> >>>
> >>> 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*.
> >>>
> > Officially CSME/ME not POR for Linux and we haven't interface to the ME. Nobody can tell how long (and why) ME will hold PHY access semaphore ant just increasing the resuming time (ULP configure) won't be solve the problem. This is not reliable approach.
> > I would agree users can add ME system on their responsibilities.
>
> It is not clear to me what you are trying to say here.

Based on the earlier thread you had referenced and his comment here it
sounds like while adding time will work for most cases, it doesn't
solve it for all cases. The problem is as a vendor you are usually
stuck looking for a solution that will work for all cases which can
lead to things like having to drop features because they can be
problematic for a few cases.

> Are you saying that you insist on keeping the e1000e_check_me check and
> thus needlessly penalizing 100s of laptops models with higher
> power-consumption unless these 100s of laptops are added manually
> to an allow list for this?
>
> I'm sorry but that is simply unacceptable, the maintenance burden
> of that is just way too high.

Think about this the other way though. If it is enabled and there are
cases where adding a delay doesn't resolve it then it still doesn't
really solve the issue does it?

> Testing on the models where the timeout issue was first hit has
> shown that increasing the timeout does actually fix it on those
> models. Sure in theory the ME on some buggy model could hold the
> semaphore even longer, but then the right thing would be to
> have a deny-list for s0ix where we can add those buggy models
> (none of which we have encountered sofar). Just like we have
> denylist for buggy hw in other places in the kernel.

This would actually have a higher maintenance burden then just
disabling the feature. Having to individually test for and deny-list
every one-off system with this bad configuration would be a pretty
significant burden. That also implies somebody would have access to
such systems and that is not normally the case. Even Intel doesn't
have all possible systems that would include this NIC.

> Maintaining an ever growing allow list for the *theoretical*
> case of encountering a model where things do not work with
> the increased timeout is not a workable and this not an
> acceptable solution.

I'm not a fan of the allow-list either, but it is preferable to a
deny-list where you have to first trigger the bug before you realize
it is there. Ideally there should be another solution in which the ME
could somehow set a flag somewhere in the hardware to indicate that it
is alive and the driver could read that order to determine if the ME
is actually alive and can skip this workaround. Then this could all be
avoided and it can be safely assumed the system is working correctly.

> The initial addition of the e1000e_check_me check instead
> of just going with the confirmed fix of bumping the timeout
> was already highly controversial and should IMHO never have
> been done.

How big was the sample size for the "confirmed" fix? How many
different vendors were there within the mix? The problem is while it
may have worked for the case you encountered you cannot say with
certainty that it worked in all cases unless you had samples of all
the different hardware out there.

> Combining this with an ever-growing allow-list on which every
> new laptop model needs to be added separately + a new
> "s0ix-enabled" ethertool flag, which existence is basically
> an admission that the allow-list approach is flawed goes
> from controversial to just plain not acceptable.

I don't view this as problematic, however this is some overhead to it.
One thing I don't know is if anyone has looked at is if the issue only
applies to a few specific system vendors. Currently the allow-list is
based on the subdevice ID. One thing we could look at doing is
enabling it based on the subvendor ID in which case we could
allow-list in large swaths of hardware with certain trusted vendors.
The only issue is that it pulls in any future parts as well so it puts
the onus on that manufacturer to avoid misconfiguring things in the
future.

RE: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Limonciello, Mario]

> 
> Based on the earlier thread you had referenced and his comment here it
> sounds like while adding time will work for most cases, it doesn't
> solve it for all cases. The problem is as a vendor you are usually
> stuck looking for a solution that will work for all cases which can
> lead to things like having to drop features because they can be
> problematic for a few cases.
> 
> > Are you saying that you insist on keeping the e1000e_check_me check and
> > thus needlessly penalizing 100s of laptops models with higher
> > power-consumption unless these 100s of laptops are added manually
> > to an allow list for this?
> >
> > I'm sorry but that is simply unacceptable, the maintenance burden
> > of that is just way too high.
> 
> Think about this the other way though. If it is enabled and there are
> cases where adding a delay doesn't resolve it then it still doesn't
> really solve the issue does it?
> 
> > Testing on the models where the timeout issue was first hit has
> > shown that increasing the timeout does actually fix it on those
> > models. Sure in theory the ME on some buggy model could hold the
> > semaphore even longer, but then the right thing would be to
> > have a deny-list for s0ix where we can add those buggy models
> > (none of which we have encountered sofar). Just like we have
> > denylist for buggy hw in other places in the kernel.
> 
> This would actually have a higher maintenance burden then just
> disabling the feature. Having to individually test for and deny-list
> every one-off system with this bad configuration would be a pretty
> significant burden. That also implies somebody would have access to
> such systems and that is not normally the case. Even Intel doesn't
> have all possible systems that would include this NIC.
> 
> > Maintaining an ever growing allow list for the *theoretical*
> > case of encountering a model where things do not work with
> > the increased timeout is not a workable and this not an
> > acceptable solution.
> 
> I'm not a fan of the allow-list either, but it is preferable to a
> deny-list where you have to first trigger the bug before you realize
> it is there. Ideally there should be another solution in which the ME
> could somehow set a flag somewhere in the hardware to indicate that it
> is alive and the driver could read that order to determine if the ME
> is actually alive and can skip this workaround. Then this could all be
> avoided and it can be safely assumed the system is working correctly.
> 
> > The initial addition of the e1000e_check_me check instead
> > of just going with the confirmed fix of bumping the timeout
> > was already highly controversial and should IMHO never have
> > been done.
> 
> How big was the sample size for the "confirmed" fix? How many
> different vendors were there within the mix? The problem is while it
> may have worked for the case you encountered you cannot say with
> certainty that it worked in all cases unless you had samples of all
> the different hardware out there.

+1
IIRC it was just Lenovo that was checked and just a few systems.

> 
> > Combining this with an ever-growing allow-list on which every
> > new laptop model needs to be added separately + a new
> > "s0ix-enabled" ethertool flag, which existence is basically
> > an admission that the allow-list approach is flawed goes
> > from controversial to just plain not acceptable.
> 
> I don't view this as problematic, however this is some overhead to it.
> One thing I don't know is if anyone has looked at is if the issue only
> applies to a few specific system vendors. Currently the allow-list is
> based on the subdevice ID. One thing we could look at doing is
> enabling it based on the subvendor ID in which case we could
> allow-list in large swaths of hardware with certain trusted vendors.

Although it would decrease the overhead my preference is that we don't lump
all of an OEM's hardware together until it's actually been checked.  It's going
to be very 
Even in a single OEM there are a variety of
BIOS vendors in the mix, different ODMs working assisting on designs, and lots
of moving pieces of firmware during development.  You'll notice I intentionally
have only included a subset of Dell's TGL designs in the later patches and
separated them out for easy reverts in the series because they're far enough
in development to be considered a stable baseline and have been validated.

I'm a fan of collapsing the lists and heuristics after a generation of systems
is done if they can all be checked, but beyond that it becomes very difficult
to pull out one single system that has a failure.

> The only issue is that it pulls in any future parts as well so it puts
> the onus on that manufacturer to avoid misconfiguring things in the
> future.

As Sasha said it's not a POR configuration right now.  So until it's become
POR configuration it should be case by case basis enabled where it works.

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Hans de Goede]

Hi,

On 12/8/20 5:14 PM, Alexander Duyck wrote:
> On Tue, Dec 8, 2020 at 1:30 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 12/8/20 6:08 AM, Neftin, Sasha wrote:
>>> On 12/7/2020 17:41, Limonciello, Mario wrote:
>>>>> 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.
>>>>
>>>> I was not happy with that decision either as it did cause regressions
>>>> on all of the "named" Comet Lake laptops that were in the market at
>>>> the time.  The "unnamed" ones are not yet released, and I don't feel
>>>> it's fair to call it a regression on "unreleased" hardware.
>>>>
>>>>>
>>>>> 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*.
>>>>>
>>> Officially CSME/ME not POR for Linux and we haven't interface to the ME. Nobody can tell how long (and why) ME will hold PHY access semaphore ant just increasing the resuming time (ULP configure) won't be solve the problem. This is not reliable approach.
>>> I would agree users can add ME system on their responsibilities.
>>
>> It is not clear to me what you are trying to say here.
> 
> Based on the earlier thread you had referenced and his comment here it
> sounds like while adding time will work for most cases, it doesn't
> solve it for all cases.

AFAIK there are 0 documented cases where the suspend/resume issue
continues to be a problem after the timeout has been increased.

If you know of actual documented cases (rather then this just being
a theoretical problem), then please provide links to those cases.

> The problem is as a vendor you are usually
> stuck looking for a solution that will work for all cases which can
> lead to things like having to drop features because they can be
> problematic for a few cases.

I disagree, there will/might always be some broken corner case
laptop-model / hw-design out there on which a feature breaks. Simply
disabling all features which might cause problems in "a few cases"
would mean that we pretty much have to disable over half the features
in the kernel.

Take for example SATA NCQ (command queing) this is know to not work
on some devices, up to the point of where with some buggy firmwares
it may cause full systems hangs and/or data-corruption. So this is
a much bigger problem then the "system won't suspend" issue we
are talking about here. Still the ATA subsys maintainers have enabled
this by default because it is an important feature to have and they
are using a deny-list to avoid enabling this on known broken hardware;
and yes every know and then we need to add a new model to the deny-list.

And the same for SATA ALPM support (a power-management feature like s0ix)
that is enabled by default too, combined with a deny-list.
I'm very familiar with the ALPM case since I pushed of it being
enabled by default and I've done most of the maintenance work
of the deny-list since it was enabled by default.

The kernel is full of this pattern, we don't disable an important
feature (and power-management is important) just because of this
causing issues in "a few cases". And again you say "a few cases"
but I know of 0 documented cases where this issue is still a problem
after bumping the timeout.

>> Are you saying that you insist on keeping the e1000e_check_me check and
>> thus needlessly penalizing 100s of laptops models with higher
>> power-consumption unless these 100s of laptops are added manually
>> to an allow list for this?
>>
>> I'm sorry but that is simply unacceptable, the maintenance burden
>> of that is just way too high.
> 
> Think about this the other way though. If it is enabled and there are
> cases where adding a delay doesn't resolve it then it still doesn't
> really solve the issue does it?

Again AFAIK that is a theoretical "If it ..." and even if it is not
theoretical, then we can add a deny-list. Maintaining a deny list for
"a few cases" being broken is a lot easier then maintaining an allow
list for allother hardware out there.

Let me put it this way, the allow-list will be orders of magnitude
longer then the deny lists. Which list would you rather manually
keep up2date?


>> Testing on the models where the timeout issue was first hit has
>> shown that increasing the timeout does actually fix it on those
>> models. Sure in theory the ME on some buggy model could hold the
>> semaphore even longer, but then the right thing would be to
>> have a deny-list for s0ix where we can add those buggy models
>> (none of which we have encountered sofar). Just like we have
>> denylist for buggy hw in other places in the kernel.
> 
> This would actually have a higher maintenance burden then just
> disabling the feature. Having to individually test for and deny-list
> every one-off system with this bad configuration would be a pretty
> significant burden. That also implies somebody would have access to
> such systems and that is not normally the case. Even Intel doesn't
> have all possible systems that would include this NIC.
> 
>> Maintaining an ever growing allow list for the *theoretical*
>> case of encountering a model where things do not work with
>> the increased timeout is not a workable and this not an
>> acceptable solution.
> 
> I'm not a fan of the allow-list either, but it is preferable to a
> deny-list where you have to first trigger the bug before you realize
> it is there.

IIRC, if the bug is there the system does not suspend, and the e1000e
driver logs an error that it is the culprit. So this is very easy to spot /
detect by end users when they hit it.

Again the kernel is full of deny lists to disable some features
on broken hardware, with sometimes hitting the buggy/broken hw
scenario having much worse consequences. Yet this is how this is
done everywhere.

The e1000e driver really is not all that special that it should
get an exception to how this is normally done.

> Ideally there should be another solution in which the ME
> could somehow set a flag somewhere in the hardware to indicate that it
> is alive and the driver could read that order to determine if the ME
> is actually alive and can skip this workaround. Then this could all be
> avoided and it can be safely assumed the system is working correctly.
> 
>> The initial addition of the e1000e_check_me check instead
>> of just going with the confirmed fix of bumping the timeout
>> was already highly controversial and should IMHO never have
>> been done.
> 
> How big was the sample size for the "confirmed" fix? How many
> different vendors were there within the mix? The problem is while it
> may have worked for the case you encountered you cannot say with
> certainty that it worked in all cases unless you had samples of all
> the different hardware out there.
> 
>> Combining this with an ever-growing allow-list on which every
>> new laptop model needs to be added separately + a new
>> "s0ix-enabled" ethertool flag, which existence is basically
>> an admission that the allow-list approach is flawed goes
>> from controversial to just plain not acceptable.
> 
> I don't view this as problematic, however this is some overhead to it.
> One thing I don't know is if anyone has looked at is if the issue only
> applies to a few specific system vendors. Currently the allow-list is
> based on the subdevice ID. One thing we could look at doing is
> enabling it based on the subvendor ID in which case we could
> allow-list in large swaths of hardware with certain trusted vendors.
> The only issue is that it pulls in any future parts as well so it puts
> the onus on that manufacturer to avoid misconfiguring things in the
> future.

If we go this route, we will likely get Dell, Lenovo (which had
the issue without the increased timeout) and maybe HP on the
allow-list, probably with a finer grained deny-list on top to
opt out on some models from these vendors where things turn
out to be buggy after all.

This:

1. Still requires a deny-list on top (at least this is very likely)
2. Leaves users of all but the 3 big vendors in the cold which
really is not a nice way to deal with this.

Regards,

Hans

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Alexander Duyck]

On Wed, Dec 9, 2020 at 6:44 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/8/20 5:14 PM, Alexander Duyck wrote:
> > On Tue, Dec 8, 2020 at 1:30 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 12/8/20 6:08 AM, Neftin, Sasha wrote:
> >>> On 12/7/2020 17:41, Limonciello, Mario wrote:
> >>>>> 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.
> >>>>
> >>>> I was not happy with that decision either as it did cause regressions
> >>>> on all of the "named" Comet Lake laptops that were in the market at
> >>>> the time.  The "unnamed" ones are not yet released, and I don't feel
> >>>> it's fair to call it a regression on "unreleased" hardware.
> >>>>
> >>>>>
> >>>>> 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*.
> >>>>>
> >>> Officially CSME/ME not POR for Linux and we haven't interface to the ME. Nobody can tell how long (and why) ME will hold PHY access semaphore ant just increasing the resuming time (ULP configure) won't be solve the problem. This is not reliable approach.
> >>> I would agree users can add ME system on their responsibilities.
> >>
> >> It is not clear to me what you are trying to say here.
> >
> > Based on the earlier thread you had referenced and his comment here it
> > sounds like while adding time will work for most cases, it doesn't
> > solve it for all cases.
>
> AFAIK there are 0 documented cases where the suspend/resume issue
> continues to be a problem after the timeout has been increased.
>
> If you know of actual documented cases (rather then this just being
> a theoretical problem), then please provide links to those cases.

If there are such notes I wouldn't have access to them. Do we know if
any sort of errata document has been posted for this issue by Intel?
That would be where an explanation of the problems and the reasoning
behind the workaround would be defined. Without that I am just
speculating based off of what has been said here and in the other
thread.

> > The problem is as a vendor you are usually
> > stuck looking for a solution that will work for all cases which can
> > lead to things like having to drop features because they can be
> > problematic for a few cases.
>
> I disagree, there will/might always be some broken corner case
> laptop-model / hw-design out there on which a feature breaks. Simply
> disabling all features which might cause problems in "a few cases"
> would mean that we pretty much have to disable over half the features
> in the kernel.
>
> Take for example SATA NCQ (command queing) this is know to not work
> on some devices, up to the point of where with some buggy firmwares
> it may cause full systems hangs and/or data-corruption. So this is
> a much bigger problem then the "system won't suspend" issue we
> are talking about here. Still the ATA subsys maintainers have enabled
> this by default because it is an important feature to have and they
> are using a deny-list to avoid enabling this on known broken hardware;
> and yes every know and then we need to add a new model to the deny-list.
>
> And the same for SATA ALPM support (a power-management feature like s0ix)
> that is enabled by default too, combined with a deny-list.
> I'm very familiar with the ALPM case since I pushed of it being
> enabled by default and I've done most of the maintenance work
> of the deny-list since it was enabled by default.
>
> The kernel is full of this pattern, we don't disable an important
> feature (and power-management is important) just because of this
> causing issues in "a few cases". And again you say "a few cases"
> but I know of 0 documented cases where this issue is still a problem
> after bumping the timeout.

It all comes down to who owns the maintenance in those cases. That is
the heart of the issue.

Last I knew Intel was maintaining the e1000e driver. So the decision
to support this or not is up to them unless Dave or Jakub want to
override. Basically the maintenance cost has to be assumed by whoever
decides what route to go. I guess Intel for now is opting to require
an allow-list rather than a deny-list for that reason. That way
whoever adds a new device is on the hook to verify it works, rather
than them having to fix things after something breaks.

> >> Are you saying that you insist on keeping the e1000e_check_me check and
> >> thus needlessly penalizing 100s of laptops models with higher
> >> power-consumption unless these 100s of laptops are added manually
> >> to an allow list for this?
> >>
> >> I'm sorry but that is simply unacceptable, the maintenance burden
> >> of that is just way too high.
> >
> > Think about this the other way though. If it is enabled and there are
> > cases where adding a delay doesn't resolve it then it still doesn't
> > really solve the issue does it?
>
> Again AFAIK that is a theoretical "If it ..." and even if it is not
> theoretical, then we can add a deny-list. Maintaining a deny list for
> "a few cases" being broken is a lot easier then maintaining an allow
> list for allother hardware out there.
>
> Let me put it this way, the allow-list will be orders of magnitude
> longer then the deny lists. Which list would you rather manually
> keep up2date?

It all depends on the support model. An allow-list puts the onus on
the vendors to validate their parts before they have access to the
feature as we are seeing now from Dell. A deny-list would put the onus
on the community and Intel as we would have to find and document the
cases where this doesn't work. Ultimately it all comes down to who has
to do the work.

> >> Testing on the models where the timeout issue was first hit has
> >> shown that increasing the timeout does actually fix it on those
> >> models. Sure in theory the ME on some buggy model could hold the
> >> semaphore even longer, but then the right thing would be to
> >> have a deny-list for s0ix where we can add those buggy models
> >> (none of which we have encountered sofar). Just like we have
> >> denylist for buggy hw in other places in the kernel.
> >
> > This would actually have a higher maintenance burden then just
> > disabling the feature. Having to individually test for and deny-list
> > every one-off system with this bad configuration would be a pretty
> > significant burden. That also implies somebody would have access to
> > such systems and that is not normally the case. Even Intel doesn't
> > have all possible systems that would include this NIC.
> >
> >> Maintaining an ever growing allow list for the *theoretical*
> >> case of encountering a model where things do not work with
> >> the increased timeout is not a workable and this not an
> >> acceptable solution.
> >
> > I'm not a fan of the allow-list either, but it is preferable to a
> > deny-list where you have to first trigger the bug before you realize
> > it is there.
>
> IIRC, if the bug is there the system does not suspend, and the e1000e
> driver logs an error that it is the culprit. So this is very easy to spot /
> detect by end users when they hit it.
>
> Again the kernel is full of deny lists to disable some features
> on broken hardware, with sometimes hitting the buggy/broken hw
> scenario having much worse consequences. Yet this is how this is
> done everywhere.
>
> The e1000e driver really is not all that special that it should
> get an exception to how this is normally done.

Actually allow-lists are not all that uncommon when it comes to the
network tree. The fact is there are a number of PHYs and the like that
are supported only by allow-list if I recall on the Intel parts.
Basically the model depends on the issue. If you want to be able to
test and verify something before you add support for it normally an
allow-list is the way to go.

> > Ideally there should be another solution in which the ME
> > could somehow set a flag somewhere in the hardware to indicate that it
> > is alive and the driver could read that order to determine if the ME
> > is actually alive and can skip this workaround. Then this could all be
> > avoided and it can be safely assumed the system is working correctly.
> >
> >> The initial addition of the e1000e_check_me check instead
> >> of just going with the confirmed fix of bumping the timeout
> >> was already highly controversial and should IMHO never have
> >> been done.
> >
> > How big was the sample size for the "confirmed" fix? How many
> > different vendors were there within the mix? The problem is while it
> > may have worked for the case you encountered you cannot say with
> > certainty that it worked in all cases unless you had samples of all
> > the different hardware out there.
> >
> >> Combining this with an ever-growing allow-list on which every
> >> new laptop model needs to be added separately + a new
> >> "s0ix-enabled" ethertool flag, which existence is basically
> >> an admission that the allow-list approach is flawed goes
> >> from controversial to just plain not acceptable.
> >
> > I don't view this as problematic, however this is some overhead to it.
> > One thing I don't know is if anyone has looked at is if the issue only
> > applies to a few specific system vendors. Currently the allow-list is
> > based on the subdevice ID. One thing we could look at doing is
> > enabling it based on the subvendor ID in which case we could
> > allow-list in large swaths of hardware with certain trusted vendors.
> > The only issue is that it pulls in any future parts as well so it puts
> > the onus on that manufacturer to avoid misconfiguring things in the
> > future.
>
> If we go this route, we will likely get Dell, Lenovo (which had
> the issue without the increased timeout) and maybe HP on the
> allow-list, probably with a finer grained deny-list on top to
> opt out on some models from these vendors where things turn
> out to be buggy after all.
>
> This:
>
> 1. Still requires a deny-list on top (at least this is very likely)
> 2. Leaves users of all but the 3 big vendors in the cold which
> really is not a nice way to deal with this.

Well the beauty about the kernel is that you are always welcome to
submit a patch and we can debate it. I know in the case of the Intel
10G NIC there was a patch that added a module parameter for overriding
the PHY allow-list so that the NIC would try to enable whatever PHY
was connected to it. Perhaps you could submit a similar patch that
would allow your timer approach and add a warning indicating that if
you see PHY hangs the s0ix issue may be responsible.

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Neftin, Sasha]

On 12/10/2020 04:24, Alexander Duyck wrote:
> On Wed, Dec 9, 2020 at 6:44 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 12/8/20 5:14 PM, Alexander Duyck wrote:
>>> On Tue, Dec 8, 2020 at 1:30 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 12/8/20 6:08 AM, Neftin, Sasha wrote:
>>>>> On 12/7/2020 17:41, Limonciello, Mario wrote:
>>>>>>> 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.
>>>>>>
>>>>>> I was not happy with that decision either as it did cause regressions
>>>>>> on all of the "named" Comet Lake laptops that were in the market at
>>>>>> the time.  The "unnamed" ones are not yet released, and I don't feel
>>>>>> it's fair to call it a regression on "unreleased" hardware.
>>>>>>
>>>>>>>
>>>>>>> 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*.
>>>>>>>
>>>>> Officially CSME/ME not POR for Linux and we haven't interface to the ME. Nobody can tell how long (and why) ME will hold PHY access semaphore ant just increasing the resuming time (ULP configure) won't be solve the problem. This is not reliable approach.
>>>>> I would agree users can add ME system on their responsibilities.
>>>>
>>>> It is not clear to me what you are trying to say here.
>>>
>>> Based on the earlier thread you had referenced and his comment here it
>>> sounds like while adding time will work for most cases, it doesn't
>>> solve it for all cases.
>>
>> AFAIK there are 0 documented cases where the suspend/resume issue
>> continues to be a problem after the timeout has been increased.
>>
>> If you know of actual documented cases (rather then this just being
>> a theoretical problem), then please provide links to those cases.
> 
> If there are such notes I wouldn't have access to them. Do we know if
> any sort of errata document has been posted for this issue by Intel?
> That would be where an explanation of the problems and the reasoning
> behind the workaround would be defined. Without that I am just
> speculating based off of what has been said here and in the other
> thread.
> 
>>> The problem is as a vendor you are usually
>>> stuck looking for a solution that will work for all cases which can
>>> lead to things like having to drop features because they can be
>>> problematic for a few cases.
>>
>> I disagree, there will/might always be some broken corner case
>> laptop-model / hw-design out there on which a feature breaks. Simply
>> disabling all features which might cause problems in "a few cases"
>> would mean that we pretty much have to disable over half the features
>> in the kernel.
>>
>> Take for example SATA NCQ (command queing) this is know to not work
>> on some devices, up to the point of where with some buggy firmwares
>> it may cause full systems hangs and/or data-corruption. So this is
>> a much bigger problem then the "system won't suspend" issue we
>> are talking about here. Still the ATA subsys maintainers have enabled
>> this by default because it is an important feature to have and they
>> are using a deny-list to avoid enabling this on known broken hardware;
>> and yes every know and then we need to add a new model to the deny-list.
>>
>> And the same for SATA ALPM support (a power-management feature like s0ix)
>> that is enabled by default too, combined with a deny-list.
>> I'm very familiar with the ALPM case since I pushed of it being
>> enabled by default and I've done most of the maintenance work
>> of the deny-list since it was enabled by default.
>>
>> The kernel is full of this pattern, we don't disable an important
>> feature (and power-management is important) just because of this
>> causing issues in "a few cases". And again you say "a few cases"
>> but I know of 0 documented cases where this issue is still a problem
>> after bumping the timeout.
> 
> It all comes down to who owns the maintenance in those cases. That is
> the heart of the issue.
> 
> Last I knew Intel was maintaining the e1000e driver. So the decision
> to support this or not is up to them unless Dave or Jakub want to
> override. Basically the maintenance cost has to be assumed by whoever
> decides what route to go. I guess Intel for now is opting to require
> an allow-list rather than a deny-list for that reason. That way
> whoever adds a new device is on the hook to verify it works, rather
> than them having to fix things after something breaks.
> 
>>>> Are you saying that you insist on keeping the e1000e_check_me check and
>>>> thus needlessly penalizing 100s of laptops models with higher
>>>> power-consumption unless these 100s of laptops are added manually
>>>> to an allow list for this?
>>>>
>>>> I'm sorry but that is simply unacceptable, the maintenance burden
>>>> of that is just way too high.
>>>
>>> Think about this the other way though. If it is enabled and there are
>>> cases where adding a delay doesn't resolve it then it still doesn't
>>> really solve the issue does it?
>>
>> Again AFAIK that is a theoretical "If it ..." and even if it is not
>> theoretical, then we can add a deny-list. Maintaining a deny list for
>> "a few cases" being broken is a lot easier then maintaining an allow
>> list for allother hardware out there.
>>
>> Let me put it this way, the allow-list will be orders of magnitude
>> longer then the deny lists. Which list would you rather manually
>> keep up2date?
> 
> It all depends on the support model. An allow-list puts the onus on
> the vendors to validate their parts before they have access to the
> feature as we are seeing now from Dell. A deny-list would put the onus
> on the community and Intel as we would have to find and document the
> cases where this doesn't work. Ultimately it all comes down to who has
> to do the work.
> 
>>>> Testing on the models where the timeout issue was first hit has
>>>> shown that increasing the timeout does actually fix it on those
>>>> models. Sure in theory the ME on some buggy model could hold the
>>>> semaphore even longer, but then the right thing would be to
>>>> have a deny-list for s0ix where we can add those buggy models
>>>> (none of which we have encountered sofar). Just like we have
>>>> denylist for buggy hw in other places in the kernel.
>>>
>>> This would actually have a higher maintenance burden then just
>>> disabling the feature. Having to individually test for and deny-list
>>> every one-off system with this bad configuration would be a pretty
>>> significant burden. That also implies somebody would have access to
>>> such systems and that is not normally the case. Even Intel doesn't
>>> have all possible systems that would include this NIC.
>>>
>>>> Maintaining an ever growing allow list for the *theoretical*
>>>> case of encountering a model where things do not work with
>>>> the increased timeout is not a workable and this not an
>>>> acceptable solution.
>>>
>>> I'm not a fan of the allow-list either, but it is preferable to a
>>> deny-list where you have to first trigger the bug before you realize
>>> it is there.
>>
>> IIRC, if the bug is there the system does not suspend, and the e1000e
>> driver logs an error that it is the culprit. So this is very easy to spot /
>> detect by end users when they hit it.
>>
>> Again the kernel is full of deny lists to disable some features
>> on broken hardware, with sometimes hitting the buggy/broken hw
>> scenario having much worse consequences. Yet this is how this is
>> done everywhere.
>>
>> The e1000e driver really is not all that special that it should
>> get an exception to how this is normally done.
> 
> Actually allow-lists are not all that uncommon when it comes to the
> network tree. The fact is there are a number of PHYs and the like that
> are supported only by allow-list if I recall on the Intel parts.
> Basically the model depends on the issue. If you want to be able to
> test and verify something before you add support for it normally an
> allow-list is the way to go.
> 
>>> Ideally there should be another solution in which the ME
>>> could somehow set a flag somewhere in the hardware to indicate that it
>>> is alive and the driver could read that order to determine if the ME
>>> is actually alive and can skip this workaround. Then this could all be
>>> avoided and it can be safely assumed the system is working correctly.
>>>
>>>> The initial addition of the e1000e_check_me check instead
>>>> of just going with the confirmed fix of bumping the timeout
>>>> was already highly controversial and should IMHO never have
>>>> been done.
>>>
>>> How big was the sample size for the "confirmed" fix? How many
>>> different vendors were there within the mix? The problem is while it
>>> may have worked for the case you encountered you cannot say with
>>> certainty that it worked in all cases unless you had samples of all
>>> the different hardware out there.
>>>
>>>> Combining this with an ever-growing allow-list on which every
>>>> new laptop model needs to be added separately + a new
>>>> "s0ix-enabled" ethertool flag, which existence is basically
>>>> an admission that the allow-list approach is flawed goes
>>>> from controversial to just plain not acceptable.
>>>
>>> I don't view this as problematic, however this is some overhead to it.
>>> One thing I don't know is if anyone has looked at is if the issue only
>>> applies to a few specific system vendors. Currently the allow-list is
>>> based on the subdevice ID. One thing we could look at doing is
>>> enabling it based on the subvendor ID in which case we could
>>> allow-list in large swaths of hardware with certain trusted vendors.
>>> The only issue is that it pulls in any future parts as well so it puts
>>> the onus on that manufacturer to avoid misconfiguring things in the
>>> future.
>>
>> If we go this route, we will likely get Dell, Lenovo (which had
>> the issue without the increased timeout) and maybe HP on the
>> allow-list, probably with a finer grained deny-list on top to
>> opt out on some models from these vendors where things turn
>> out to be buggy after all.
>>
>> This:
>>
>> 1. Still requires a deny-list on top (at least this is very likely)
>> 2. Leaves users of all but the 3 big vendors in the cold which
>> really is not a nice way to deal with this.
> 
> Well the beauty about the kernel is that you are always welcome to
> submit a patch and we can debate it. I know in the case of the Intel
> 10G NIC there was a patch that added a module parameter for overriding
> the PHY allow-list so that the NIC would try to enable whatever PHY
> was connected to it. Perhaps you could submit a similar patch that
> would allow your timer approach and add a warning indicating that if
> you see PHY hangs the s0ix issue may be responsible.
> 
Again, the ME/AMT system not POR on Linux. We can't guarantee smooth 
working Linux platforms with ME.
We will consult with our Architecture regards this approach 
(ULP_CONFIG_DONE time out and privileged flag for S0ix) - I will reply 
soon.
Thanks,
Sasha

Re: [PATCH v3 0/7] Improve s0ix flows for systems i219LM

[Neftin, Sasha]

On 12/10/2020 07:28, Neftin, Sasha wrote:
> On 12/10/2020 04:24, Alexander Duyck wrote:
>> On Wed, Dec 9, 2020 at 6:44 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 12/8/20 5:14 PM, Alexander Duyck wrote:
>>>> On Tue, Dec 8, 2020 at 1:30 AM Hans de Goede <hdegoede@redhat.com> 
>>>> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 12/8/20 6:08 AM, Neftin, Sasha wrote:
>>>>>> On 12/7/2020 17:41, Limonciello, Mario wrote:
>>>>>>>> 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.
>>>>>>>
>>>>>>> I was not happy with that decision either as it did cause 
>>>>>>> regressions
>>>>>>> on all of the "named" Comet Lake laptops that were in the market at
>>>>>>> the time.  The "unnamed" ones are not yet released, and I don't feel
>>>>>>> it's fair to call it a regression on "unreleased" hardware.
>>>>>>>
>>>>>>>>
>>>>>>>> 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*.
>>>>>>>>
>>>>>> Officially CSME/ME not POR for Linux and we haven't interface to 
>>>>>> the ME. Nobody can tell how long (and why) ME will hold PHY access 
>>>>>> semaphore ant just increasing the resuming time (ULP configure) 
>>>>>> won't be solve the problem. This is not reliable approach.
>>>>>> I would agree users can add ME system on their responsibilities.
>>>>>
>>>>> It is not clear to me what you are trying to say here.
>>>>
>>>> Based on the earlier thread you had referenced and his comment here it
>>>> sounds like while adding time will work for most cases, it doesn't
>>>> solve it for all cases.
>>>
>>> AFAIK there are 0 documented cases where the suspend/resume issue
>>> continues to be a problem after the timeout has been increased.
>>>
>>> If you know of actual documented cases (rather then this just being
>>> a theoretical problem), then please provide links to those cases.
>>
>> If there are such notes I wouldn't have access to them. Do we know if
>> any sort of errata document has been posted for this issue by Intel?
>> That would be where an explanation of the problems and the reasoning
>> behind the workaround would be defined. Without that I am just
>> speculating based off of what has been said here and in the other
>> thread.
>>
>>>> The problem is as a vendor you are usually
>>>> stuck looking for a solution that will work for all cases which can
>>>> lead to things like having to drop features because they can be
>>>> problematic for a few cases.
>>>
>>> I disagree, there will/might always be some broken corner case
>>> laptop-model / hw-design out there on which a feature breaks. Simply
>>> disabling all features which might cause problems in "a few cases"
>>> would mean that we pretty much have to disable over half the features
>>> in the kernel.
>>>
>>> Take for example SATA NCQ (command queing) this is know to not work
>>> on some devices, up to the point of where with some buggy firmwares
>>> it may cause full systems hangs and/or data-corruption. So this is
>>> a much bigger problem then the "system won't suspend" issue we
>>> are talking about here. Still the ATA subsys maintainers have enabled
>>> this by default because it is an important feature to have and they
>>> are using a deny-list to avoid enabling this on known broken hardware;
>>> and yes every know and then we need to add a new model to the deny-list.
>>>
>>> And the same for SATA ALPM support (a power-management feature like 
>>> s0ix)
>>> that is enabled by default too, combined with a deny-list.
>>> I'm very familiar with the ALPM case since I pushed of it being
>>> enabled by default and I've done most of the maintenance work
>>> of the deny-list since it was enabled by default.
>>>
>>> The kernel is full of this pattern, we don't disable an important
>>> feature (and power-management is important) just because of this
>>> causing issues in "a few cases". And again you say "a few cases"
>>> but I know of 0 documented cases where this issue is still a problem
>>> after bumping the timeout.
>>
>> It all comes down to who owns the maintenance in those cases. That is
>> the heart of the issue.
>>
>> Last I knew Intel was maintaining the e1000e driver. So the decision
>> to support this or not is up to them unless Dave or Jakub want to
>> override. Basically the maintenance cost has to be assumed by whoever
>> decides what route to go. I guess Intel for now is opting to require
>> an allow-list rather than a deny-list for that reason. That way
>> whoever adds a new device is on the hook to verify it works, rather
>> than them having to fix things after something breaks.
>>
>>>>> Are you saying that you insist on keeping the e1000e_check_me check 
>>>>> and
>>>>> thus needlessly penalizing 100s of laptops models with higher
>>>>> power-consumption unless these 100s of laptops are added manually
>>>>> to an allow list for this?
>>>>>
>>>>> I'm sorry but that is simply unacceptable, the maintenance burden
>>>>> of that is just way too high.
>>>>
>>>> Think about this the other way though. If it is enabled and there are
>>>> cases where adding a delay doesn't resolve it then it still doesn't
>>>> really solve the issue does it?
>>>
>>> Again AFAIK that is a theoretical "If it ..." and even if it is not
>>> theoretical, then we can add a deny-list. Maintaining a deny list for
>>> "a few cases" being broken is a lot easier then maintaining an allow
>>> list for allother hardware out there.
>>>
>>> Let me put it this way, the allow-list will be orders of magnitude
>>> longer then the deny lists. Which list would you rather manually
>>> keep up2date?
>>
>> It all depends on the support model. An allow-list puts the onus on
>> the vendors to validate their parts before they have access to the
>> feature as we are seeing now from Dell. A deny-list would put the onus
>> on the community and Intel as we would have to find and document the
>> cases where this doesn't work. Ultimately it all comes down to who has
>> to do the work.
>>
>>>>> Testing on the models where the timeout issue was first hit has
>>>>> shown that increasing the timeout does actually fix it on those
>>>>> models. Sure in theory the ME on some buggy model could hold the
>>>>> semaphore even longer, but then the right thing would be to
>>>>> have a deny-list for s0ix where we can add those buggy models
>>>>> (none of which we have encountered sofar). Just like we have
>>>>> denylist for buggy hw in other places in the kernel.
>>>>
>>>> This would actually have a higher maintenance burden then just
>>>> disabling the feature. Having to individually test for and deny-list
>>>> every one-off system with this bad configuration would be a pretty
>>>> significant burden. That also implies somebody would have access to
>>>> such systems and that is not normally the case. Even Intel doesn't
>>>> have all possible systems that would include this NIC.
>>>>
>>>>> Maintaining an ever growing allow list for the *theoretical*
>>>>> case of encountering a model where things do not work with
>>>>> the increased timeout is not a workable and this not an
>>>>> acceptable solution.
>>>>
>>>> I'm not a fan of the allow-list either, but it is preferable to a
>>>> deny-list where you have to first trigger the bug before you realize
>>>> it is there.
>>>
>>> IIRC, if the bug is there the system does not suspend, and the e1000e
>>> driver logs an error that it is the culprit. So this is very easy to 
>>> spot /
>>> detect by end users when they hit it.
>>>
>>> Again the kernel is full of deny lists to disable some features
>>> on broken hardware, with sometimes hitting the buggy/broken hw
>>> scenario having much worse consequences. Yet this is how this is
>>> done everywhere.
>>>
>>> The e1000e driver really is not all that special that it should
>>> get an exception to how this is normally done.
>>
>> Actually allow-lists are not all that uncommon when it comes to the
>> network tree. The fact is there are a number of PHYs and the like that
>> are supported only by allow-list if I recall on the Intel parts.
>> Basically the model depends on the issue. If you want to be able to
>> test and verify something before you add support for it normally an
>> allow-list is the way to go.
>>
>>>> Ideally there should be another solution in which the ME
>>>> could somehow set a flag somewhere in the hardware to indicate that it
>>>> is alive and the driver could read that order to determine if the ME
>>>> is actually alive and can skip this workaround. Then this could all be
>>>> avoided and it can be safely assumed the system is working correctly.
>>>>
>>>>> The initial addition of the e1000e_check_me check instead
>>>>> of just going with the confirmed fix of bumping the timeout
>>>>> was already highly controversial and should IMHO never have
>>>>> been done.
>>>>
>>>> How big was the sample size for the "confirmed" fix? How many
>>>> different vendors were there within the mix? The problem is while it
>>>> may have worked for the case you encountered you cannot say with
>>>> certainty that it worked in all cases unless you had samples of all
>>>> the different hardware out there.
>>>>
>>>>> Combining this with an ever-growing allow-list on which every
>>>>> new laptop model needs to be added separately + a new
>>>>> "s0ix-enabled" ethertool flag, which existence is basically
>>>>> an admission that the allow-list approach is flawed goes
>>>>> from controversial to just plain not acceptable.
>>>>
>>>> I don't view this as problematic, however this is some overhead to it.
>>>> One thing I don't know is if anyone has looked at is if the issue only
>>>> applies to a few specific system vendors. Currently the allow-list is
>>>> based on the subdevice ID. One thing we could look at doing is
>>>> enabling it based on the subvendor ID in which case we could
>>>> allow-list in large swaths of hardware with certain trusted vendors.
>>>> The only issue is that it pulls in any future parts as well so it puts
>>>> the onus on that manufacturer to avoid misconfiguring things in the
>>>> future.
>>>
>>> If we go this route, we will likely get Dell, Lenovo (which had
>>> the issue without the increased timeout) and maybe HP on the
>>> allow-list, probably with a finer grained deny-list on top to
>>> opt out on some models from these vendors where things turn
>>> out to be buggy after all.
>>>
>>> This:
>>>
>>> 1. Still requires a deny-list on top (at least this is very likely)
>>> 2. Leaves users of all but the 3 big vendors in the cold which
>>> really is not a nice way to deal with this.
>>
>> Well the beauty about the kernel is that you are always welcome to
>> submit a patch and we can debate it. I know in the case of the Intel
>> 10G NIC there was a patch that added a module parameter for overriding
>> the PHY allow-list so that the NIC would try to enable whatever PHY
>> was connected to it. Perhaps you could submit a similar patch that
>> would allow your timer approach and add a warning indicating that if
>> you see PHY hangs the s0ix issue may be responsible.
>>
> Again, the ME/AMT system not POR on Linux. We can't guarantee smooth 
> working Linux platforms with ME.
> We will consult with our Architecture regards this approach 
> (ULP_CONFIG_DONE time out and privileged flag for S0ix) - I will reply 
> soon.
After internal discussion, our recommendations as follow:

1. The relevant timeout should be increased from 300ms to 1000ms 
regardless of anything else. We haven't interface with CSME. Despite 
this, we compared it to the Windows driver and decided to recommend an 
increased delay up to 1000ms. 
(https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20200323191639.48826-1-aaron.ma@canonical.com/ 
- this is expected solve part of the problem)

2.Since S0ix is mainstream now and users expect the related power
consumption savings, the flow should be enabled by default
We don't want any lists for controlling how the feature should be
enabled/disabled.
There should be a controllable flag for enabling/disabling the S0ix 
flow, since this is the flow that introduced the inter-op problems with ME.

3.The flag must be persistent (keeping its value across reboots/Sx 
entries) and controllable at runtime, either by the OEM (during initial 
setup) or by a privileged user (admin).

> Thanks,
> Sasha
Thanks,
Sasha