| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 30 Nov 2020 22:49:39 +0000
From: "Limonciello, Mario" <Mario.Limonciello@...l.com>
To: Alexander Duyck <alexander.duyck@...il.com>,
Tony Nguyen <anthony.l.nguyen@...el.com>
CC: David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Netdev <netdev@...r.kernel.org>,
Stefan Assmann <sassmann@...hat.com>,
"Neftin, Sasha" <sasha.neftin@...el.com>,
"Shen, Yijun" <Yijun.Shen@...l.com>
Subject: RE: [net-next 2/4] e1000e: Add Dell's Comet Lake systems into s0ix
heuristics
> On Mon, Nov 30, 2020 at 1:32 PM Tony Nguyen <anthony.l.nguyen@...el.com>
> wrote:
> >
> > From: Mario Limonciello <mario.limonciello@...l.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@...l.com>
> > Tested-by: Yijun Shen <Yijun.shen@...l.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.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.
Powered by blists - more mailing lists