diff mbox series

[net-next,2/4] e1000e: Add Dell's Comet Lake systems into s0ix heuristics

Message ID 20201130212907.320677-3-anthony.l.nguyen@intel.com
State Superseded
Headers show
Series 1GbE Intel Wired LAN Driver Updates 2020-11-30 | expand

Commit Message

Tony Nguyen Nov. 30, 2020, 9:29 p.m. UTC 
From: Mario Limonciello <mario.limonciello@dell.com>

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

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
Tested-by: Yijun Shen <Yijun.shen@dell.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/Kconfig        |  1 +
 drivers/net/ethernet/intel/e1000e/param.c | 80 ++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 1 deletion(-)

Comments

Alexander H Duyck Nov. 30, 2020, 10:31 p.m. UTC | #1 
On Mon, Nov 30, 2020 at 1:32 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>
> From: Mario Limonciello <mario.limonciello@dell.com>
>
> Dell's Comet Lake Latitude and Precision systems containing i219LM are
> properly configured and should use the s0ix flows.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> Tested-by: Yijun Shen <Yijun.shen@dell.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/Kconfig        |  1 +
>  drivers/net/ethernet/intel/e1000e/param.c | 80 ++++++++++++++++++++++-
>  2 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
> index 5aa86318ed3e..280af47d74d2 100644
> --- a/drivers/net/ethernet/intel/Kconfig
> +++ b/drivers/net/ethernet/intel/Kconfig
> @@ -58,6 +58,7 @@ config E1000
>  config E1000E
>         tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
>         depends on PCI && (!SPARC32 || BROKEN)
> +       depends on DMI
>         select CRC32
>         imply PTP_1588_CLOCK
>         help

Is DMI the only way we can identify systems that want to enable S0ix
states? I'm just wondering if we could identify these parts using a 4
tuple device ID or if the DMI ID is the only way we can do this?

> diff --git a/drivers/net/ethernet/intel/e1000e/param.c b/drivers/net/ethernet/intel/e1000e/param.c
> index 56316b797521..d05f55201541 100644
> --- a/drivers/net/ethernet/intel/e1000e/param.c
> +++ b/drivers/net/ethernet/intel/e1000e/param.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright(c) 1999 - 2018 Intel Corporation. */
>
> +#include <linux/dmi.h>
>  #include <linux/netdevice.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> @@ -201,6 +202,80 @@ static const struct e1000e_me_supported me_supported[] = {
>         {0}
>  };
>
> +static const struct dmi_system_id s0ix_supported_systems[] = {
> +       {
> +               /* Dell Latitude 5310 */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_SKU, "099F"),
> +               },
> +       },
> +       {
> +               /* Dell Latitude 5410 */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_SKU, "09A0"),
> +               },
> +       },
> +       {
> +               /* Dell Latitude 5410 */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C9"),
> +               },
> +       },
> +       {
> +               /* Dell Latitude 5510 */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_SKU, "09A1"),
> +               },
> +       },
> +       {
> +               /* Dell Precision 3550 */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_SKU, "09A2"),
> +               },
> +       },
> +       {
> +               /* Dell Latitude 5411 */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C0"),
> +               },
> +       },
> +       {
> +               /* Dell Latitude 5511 */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C1"),
> +               },
> +       },
> +       {
> +               /* Dell Precision 3551 */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C2"),
> +               },
> +       },
> +       {
> +               /* Dell Precision 7550 */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C3"),
> +               },
> +       },
> +       {
> +               /* Dell Precision 7750 */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C4"),
> +               },
> +       },
> +       { }
> +};
> +

So which "product" are we verifying here? Are these SKU values for the
platform or for the NIC? Just wondering if this is something we could
retrieve via PCI as I mentioned or if this is something that can only
be retrieved via DMI.

>  static bool e1000e_check_me(u16 device_id)
>  {
>         struct e1000e_me_supported *id;
> @@ -599,8 +674,11 @@ void e1000e_check_options(struct e1000_adapter *adapter)
>                 }
>
>                 if (enabled == S0IX_HEURISTICS) {
> +                       /* check for allowlist of systems */
> +                       if (dmi_check_system(s0ix_supported_systems))
> +                               enabled = S0IX_FORCE_ON;
>                         /* default to off for ME configurations */
> -                       if (e1000e_check_me(hw->adapter->pdev->device))
> +                       else if (e1000e_check_me(hw->adapter->pdev->device))
>                                 enabled = S0IX_FORCE_OFF;
>                 }
>

Is there really a need to set it to SOIX_FORCE_ON when the if
statement below this section will essentially treat it as though it is
set that way anyway? Seems like we only really need to just do a
(!dmi_check_system() && e1000e_check_me()) in the code block that is
setting SOIX_FORCE_OFF rather than bothering to even set enabled to
SOIX_FORCE_ON.
Limonciello, Mario Nov. 30, 2020, 10:49 p.m. UTC | #2 
> On Mon, Nov 30, 2020 at 1:32 PM Tony Nguyen <anthony.l.nguyen@intel.com>
> wrote:
> >
> > From: Mario Limonciello <mario.limonciello@dell.com>
> >
> > Dell's Comet Lake Latitude and Precision systems containing i219LM are
> > properly configured and should use the s0ix flows.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > Tested-by: Yijun Shen <Yijun.shen@dell.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> >  drivers/net/ethernet/intel/Kconfig        |  1 +
> >  drivers/net/ethernet/intel/e1000e/param.c | 80 ++++++++++++++++++++++-
> >  2 files changed, 80 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/Kconfig
> b/drivers/net/ethernet/intel/Kconfig
> > index 5aa86318ed3e..280af47d74d2 100644
> > --- a/drivers/net/ethernet/intel/Kconfig
> > +++ b/drivers/net/ethernet/intel/Kconfig
> > @@ -58,6 +58,7 @@ config E1000
> >  config E1000E
> >         tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
> >         depends on PCI && (!SPARC32 || BROKEN)
> > +       depends on DMI
> >         select CRC32
> >         imply PTP_1588_CLOCK
> >         help
> 
> Is DMI the only way we can identify systems that want to enable S0ix
> states? I'm just wondering if we could identify these parts using a 4
> tuple device ID or if the DMI ID is the only way we can do this?

I'll have to check if the PCI susbsystem vendor ID is set on this device.
That's the only other thing that comes to mind for me.

> 
> > diff --git a/drivers/net/ethernet/intel/e1000e/param.c
> b/drivers/net/ethernet/intel/e1000e/param.c
> > index 56316b797521..d05f55201541 100644
> > --- a/drivers/net/ethernet/intel/e1000e/param.c
> > +++ b/drivers/net/ethernet/intel/e1000e/param.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /* Copyright(c) 1999 - 2018 Intel Corporation. */
> >
> > +#include <linux/dmi.h>
> >  #include <linux/netdevice.h>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> > @@ -201,6 +202,80 @@ static const struct e1000e_me_supported me_supported[]
> = {
> >         {0}
> >  };
> >
> > +static const struct dmi_system_id s0ix_supported_systems[] = {
> > +       {
> > +               /* Dell Latitude 5310 */
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +                       DMI_MATCH(DMI_PRODUCT_SKU, "099F"),
> > +               },
> > +       },
> > +       {
> > +               /* Dell Latitude 5410 */
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +                       DMI_MATCH(DMI_PRODUCT_SKU, "09A0"),
> > +               },
> > +       },
> > +       {
> > +               /* Dell Latitude 5410 */
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C9"),
> > +               },
> > +       },
> > +       {
> > +               /* Dell Latitude 5510 */
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +                       DMI_MATCH(DMI_PRODUCT_SKU, "09A1"),
> > +               },
> > +       },
> > +       {
> > +               /* Dell Precision 3550 */
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +                       DMI_MATCH(DMI_PRODUCT_SKU, "09A2"),
> > +               },
> > +       },
> > +       {
> > +               /* Dell Latitude 5411 */
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C0"),
> > +               },
> > +       },
> > +       {
> > +               /* Dell Latitude 5511 */
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C1"),
> > +               },
> > +       },
> > +       {
> > +               /* Dell Precision 3551 */
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C2"),
> > +               },
> > +       },
> > +       {
> > +               /* Dell Precision 7550 */
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C3"),
> > +               },
> > +       },
> > +       {
> > +               /* Dell Precision 7750 */
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +                       DMI_MATCH(DMI_PRODUCT_SKU, "09C4"),
> > +               },
> > +       },
> > +       { }
> > +};
> > +
> 
> So which "product" are we verifying here? Are these SKU values for the
> platform or for the NIC? Just wondering if this is something we could
> retrieve via PCI as I mentioned or if this is something that can only
> be retrieved via DMI.

These are for the platform.  The platform needs to be properly configured
for doing s0ix flows.

> 
> >  static bool e1000e_check_me(u16 device_id)
> >  {
> >         struct e1000e_me_supported *id;
> > @@ -599,8 +674,11 @@ void e1000e_check_options(struct e1000_adapter
> *adapter)
> >                 }
> >
> >                 if (enabled == S0IX_HEURISTICS) {
> > +                       /* check for allowlist of systems */
> > +                       if (dmi_check_system(s0ix_supported_systems))
> > +                               enabled = S0IX_FORCE_ON;
> >                         /* default to off for ME configurations */
> > -                       if (e1000e_check_me(hw->adapter->pdev->device))
> > +                       else if (e1000e_check_me(hw->adapter->pdev->device))
> >                                 enabled = S0IX_FORCE_OFF;
> >                 }
> >
> 
> Is there really a need to set it to SOIX_FORCE_ON when the if
> statement below this section will essentially treat it as though it is
> set that way anyway? Seems like we only really need to just do a
> (!dmi_check_system() && e1000e_check_me()) in the code block that is
> setting SOIX_FORCE_OFF rather than bothering to even set enabled to
> SOIX_FORCE_ON.

Yes there are possible logic optimizations, but I wanted to make it very
explicit what was happening so that when new heuristics get added later it
makes sense.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 5aa86318ed3e..280af47d74d2 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -58,6 +58,7 @@  config E1000
 config E1000E
 	tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
 	depends on PCI && (!SPARC32 || BROKEN)
+	depends on DMI
 	select CRC32
 	imply PTP_1588_CLOCK
 	help
diff --git a/drivers/net/ethernet/intel/e1000e/param.c b/drivers/net/ethernet/intel/e1000e/param.c
index 56316b797521..d05f55201541 100644
--- a/drivers/net/ethernet/intel/e1000e/param.c
+++ b/drivers/net/ethernet/intel/e1000e/param.c
@@ -1,6 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright(c) 1999 - 2018 Intel Corporation. */
 
+#include <linux/dmi.h>
 #include <linux/netdevice.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -201,6 +202,80 @@  static const struct e1000e_me_supported me_supported[] = {
 	{0}
 };
 
+static const struct dmi_system_id s0ix_supported_systems[] = {
+	{
+		/* Dell Latitude 5310 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_SKU, "099F"),
+		},
+	},
+	{
+		/* Dell Latitude 5410 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_SKU, "09A0"),
+		},
+	},
+	{
+		/* Dell Latitude 5410 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_SKU, "09C9"),
+		},
+	},
+	{
+		/* Dell Latitude 5510 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_SKU, "09A1"),
+		},
+	},
+	{
+		/* Dell Precision 3550 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_SKU, "09A2"),
+		},
+	},
+	{
+		/* Dell Latitude 5411 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_SKU, "09C0"),
+		},
+	},
+	{
+		/* Dell Latitude 5511 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_SKU, "09C1"),
+		},
+	},
+	{
+		/* Dell Precision 3551 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_SKU, "09C2"),
+		},
+	},
+	{
+		/* Dell Precision 7550 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_SKU, "09C3"),
+		},
+	},
+	{
+		/* Dell Precision 7750 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_SKU, "09C4"),
+		},
+	},
+	{ }
+};
+
 static bool e1000e_check_me(u16 device_id)
 {
 	struct e1000e_me_supported *id;
@@ -599,8 +674,11 @@  void e1000e_check_options(struct e1000_adapter *adapter)
 		}
 
 		if (enabled == S0IX_HEURISTICS) {
+			/* check for allowlist of systems */
+			if (dmi_check_system(s0ix_supported_systems))
+				enabled = S0IX_FORCE_ON;
 			/* default to off for ME configurations */
-			if (e1000e_check_me(hw->adapter->pdev->device))
+			else if (e1000e_check_me(hw->adapter->pdev->device))
 				enabled = S0IX_FORCE_OFF;
 		}