[PATCH 1/3] e1000e: Separate TGP from SPT

[PATCH 1/3] e1000e: Separate TGP from SPT

[Kai-Heng Feng]

Separate TGP from SPT so we can apply specific quirks to TGP.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/ethernet/intel/e1000e/e1000.h   |  4 +++-
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 20 ++++++++++++++++++++
 drivers/net/ethernet/intel/e1000e/netdev.c  | 13 +++++++------
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index 5b2143f4b1f8..3178efd98006 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -113,7 +113,8 @@ enum e1000_boards {
 	board_pch2lan,
 	board_pch_lpt,
 	board_pch_spt,
-	board_pch_cnp
+	board_pch_cnp,
+	board_pch_tgp
 };
 
 struct e1000_ps_page {
@@ -499,6 +500,7 @@ extern const struct e1000_info e1000_pch2_info;
 extern const struct e1000_info e1000_pch_lpt_info;
 extern const struct e1000_info e1000_pch_spt_info;
 extern const struct e1000_info e1000_pch_cnp_info;
+extern const struct e1000_info e1000_pch_tgp_info;
 extern const struct e1000_info e1000_es2_info;
 
 void e1000e_ptp_init(struct e1000_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index cf7b3887da1d..654dbe798e55 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -5967,3 +5967,23 @@ const struct e1000_info e1000_pch_cnp_info = {
 	.phy_ops		= &ich8_phy_ops,
 	.nvm_ops		= &spt_nvm_ops,
 };
+
+const struct e1000_info e1000_pch_tgp_info = {
+	.mac			= e1000_pch_tgp,
+	.flags			= FLAG_IS_ICH
+				  | FLAG_HAS_WOL
+				  | FLAG_HAS_HW_TIMESTAMP
+				  | FLAG_HAS_CTRLEXT_ON_LOAD
+				  | FLAG_HAS_AMT
+				  | FLAG_HAS_FLASH
+				  | FLAG_HAS_JUMBO_FRAMES
+				  | FLAG_APME_IN_WUC,
+	.flags2			= FLAG2_HAS_PHY_STATS
+				  | FLAG2_HAS_EEE,
+	.pba			= 26,
+	.max_hw_frame_size	= 9022,
+	.get_variants		= e1000_get_variants_ich8lan,
+	.mac_ops		= &ich8_mac_ops,
+	.phy_ops		= &ich8_phy_ops,
+	.nvm_ops		= &spt_nvm_ops,
+};
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index d150dade06cf..5835d6cf2f51 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -51,6 +51,7 @@ static const struct e1000_info *e1000_info_tbl[] = {
 	[board_pch_lpt]		= &e1000_pch_lpt_info,
 	[board_pch_spt]		= &e1000_pch_spt_info,
 	[board_pch_cnp]		= &e1000_pch_cnp_info,
+	[board_pch_tgp]		= &e1000_pch_tgp_info,
 };
 
 struct e1000_reg_info {
@@ -7843,12 +7844,12 @@ static const struct pci_device_id e1000_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_V11), board_pch_cnp },
 	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_LM12), board_pch_spt },
 	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_V12), board_pch_spt },
-	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM13), board_pch_cnp },
-	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V13), board_pch_cnp },
-	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM14), board_pch_cnp },
-	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V14), board_pch_cnp },
-	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM15), board_pch_cnp },
-	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V15), board_pch_cnp },
+	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM13), board_pch_tgp },
+	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V13), board_pch_tgp },
+	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM14), board_pch_tgp },
+	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V14), board_pch_tgp },
+	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM15), board_pch_tgp },
+	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V15), board_pch_tgp },
 	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_LM16), board_pch_cnp },
 	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_V16), board_pch_cnp },
 	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_LM17), board_pch_cnp },
-- 
2.31.1

[PATCH 2/3] e1000e: Make mei_me active when e1000e is in use

[Kai-Heng Feng]

Many users report rather sluggish RX speed on TGP I219. Since
"intel_idle.max_cstate=1" doesn't help, so it's not caused by deeper
package C-state.

A workaround that always works is to make sure mei_me is runtime active
when e1000e is in use.

The root cause is still unknown, but since many users are affected by
the issue, implment the quirk in the driver as a temporary workaround.

Also adds mei_me as soft dependency to ensure the device link can be
created if e1000e is in initramfs.

BugLink: https://bugs.launchpad.net/bugs/1927925
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213377
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213651
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 5835d6cf2f51..e63445a8ce12 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -7317,6 +7317,27 @@ static const struct net_device_ops e1000e_netdev_ops = {
 	.ndo_features_check	= passthru_features_check,
 };
 
+static void e1000e_create_device_links(struct pci_dev *pdev)
+{
+	struct pci_dev *tgp_mei_me;
+
+	/* Find TGP mei_me devices and make e1000e power depend on mei_me */
+	tgp_mei_me = pci_get_device(PCI_VENDOR_ID_INTEL, 0xa0e0, NULL);
+	if (!tgp_mei_me) {
+		tgp_mei_me = pci_get_device(PCI_VENDOR_ID_INTEL, 0x43e0, NULL);
+		if (!tgp_mei_me)
+			return;
+	}
+
+	if (device_link_add(&pdev->dev, &tgp_mei_me->dev,
+			    DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE |
+			    DL_FLAG_AUTOREMOVE_CONSUMER))
+		pci_info(pdev, "System and runtime PM depends on %s\n",
+			 pci_name(tgp_mei_me));
+
+	pci_dev_put(tgp_mei_me);
+}
+
 /**
  * e1000_probe - Device Initialization Routine
  * @pdev: PCI device information struct
@@ -7645,6 +7666,9 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (pci_dev_run_wake(pdev) && hw->mac.type != e1000_pch_cnp)
 		pm_runtime_put_noidle(&pdev->dev);
 
+	if (hw->mac.type == e1000_pch_tgp)
+		e1000e_create_device_links(pdev);
+
 	return 0;
 
 err_register:
@@ -7917,6 +7941,8 @@ static void __exit e1000_exit_module(void)
 }
 module_exit(e1000_exit_module);
 
+/* Ensure device link can be created if e1000e is in the initramfs. */
+MODULE_SOFTDEP("pre: mei_me");
 MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
 MODULE_DESCRIPTION("Intel(R) PRO/1000 Network Driver");
 MODULE_LICENSE("GPL v2");
-- 
2.31.1

[PATCH 3/3] e1000e: Serialize TGP e1000e PM ops

[Kai-Heng Feng]

On TGL systems, PCI_COMMAND may randomly flip to 0 on system resume.
This is devastating to drivers that use pci_set_master(), like NVMe and
xHCI, to enable DMA in their resume routine, as pci_set_master() can
inadvertently disable PCI_COMMAND_IO and PCI_COMMAND_MEMORY, making
resources inaccessible.

The issue is reproducible on all kernel releases, but the situation is
exacerbated by commit 6cecf02e77ab ("Revert "e1000e: disable s0ix entry
and exit flows for ME systems"").

Seems like ME can do many things to other PCI devices until it's finally out of
ULP polling. So ensure e1000e PM ops are serialized by enforcing suspend/resume
order to workaround the issue.

Of course this will make system suspend and resume a bit slower, but we
probably need to settle on this workaround until ME is fully supported.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212039
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index e63445a8ce12..0244d3dd90a3 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -7319,7 +7319,8 @@ static const struct net_device_ops e1000e_netdev_ops = {
 
 static void e1000e_create_device_links(struct pci_dev *pdev)
 {
-	struct pci_dev *tgp_mei_me;
+	struct pci_bus *bus = pdev->bus;
+	struct pci_dev *tgp_mei_me, *p;
 
 	/* Find TGP mei_me devices and make e1000e power depend on mei_me */
 	tgp_mei_me = pci_get_device(PCI_VENDOR_ID_INTEL, 0xa0e0, NULL);
@@ -7335,6 +7336,17 @@ static void e1000e_create_device_links(struct pci_dev *pdev)
 		pci_info(pdev, "System and runtime PM depends on %s\n",
 			 pci_name(tgp_mei_me));
 
+	/* Find other devices in the SoC and make them depend on e1000e */
+	list_for_each_entry(p, &bus->devices, bus_list) {
+		if (&p->dev == &pdev->dev || &p->dev == &tgp_mei_me->dev)
+			continue;
+
+		if (device_link_add(&p->dev, &pdev->dev,
+				    DL_FLAG_AUTOREMOVE_SUPPLIER))
+			pci_info(p, "System PM depends on %s\n",
+				 pci_name(pdev));
+	}
+
 	pci_dev_put(tgp_mei_me);
 }
 
-- 
2.31.1

Re: [Intel-wired-lan] [PATCH 1/3] e1000e: Separate TGP from SPT

[Sasha Neftin]

On 7/12/2021 16:34, Kai-Heng Feng wrote:
> Separate TGP from SPT so we can apply specific quirks to TGP.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>   drivers/net/ethernet/intel/e1000e/e1000.h   |  4 +++-
>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 20 ++++++++++++++++++++
>   drivers/net/ethernet/intel/e1000e/netdev.c  | 13 +++++++------
>   3 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
> index 5b2143f4b1f8..3178efd98006 100644
> --- a/drivers/net/ethernet/intel/e1000e/e1000.h
> +++ b/drivers/net/ethernet/intel/e1000e/e1000.h
> @@ -113,7 +113,8 @@ enum e1000_boards {
>   	board_pch2lan,
>   	board_pch_lpt,
>   	board_pch_spt,
> -	board_pch_cnp
> +	board_pch_cnp,
Hello Kai-Heng,
I would agree with you here. I would suggest extending it also for other 
PCH (at least ADP and MTP).  The same controller on a different PCH.
We will be able to differentiate between boards via MAC type and submit 
quirks if need.
> +	board_pch_tgp
>   };
>   
>   struct e1000_ps_page {
> @@ -499,6 +500,7 @@ extern const struct e1000_info e1000_pch2_info;
>   extern const struct e1000_info e1000_pch_lpt_info;
>   extern const struct e1000_info e1000_pch_spt_info;
>   extern const struct e1000_info e1000_pch_cnp_info;
> +extern const struct e1000_info e1000_pch_tgp_info;
>   extern const struct e1000_info e1000_es2_info;
>   
>   void e1000e_ptp_init(struct e1000_adapter *adapter);
> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> index cf7b3887da1d..654dbe798e55 100644
> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> @@ -5967,3 +5967,23 @@ const struct e1000_info e1000_pch_cnp_info = {
>   	.phy_ops		= &ich8_phy_ops,
>   	.nvm_ops		= &spt_nvm_ops,
>   };
> +
> +const struct e1000_info e1000_pch_tgp_info = {
> +	.mac			= e1000_pch_tgp,
> +	.flags			= FLAG_IS_ICH
> +				  | FLAG_HAS_WOL
> +				  | FLAG_HAS_HW_TIMESTAMP
> +				  | FLAG_HAS_CTRLEXT_ON_LOAD
> +				  | FLAG_HAS_AMT
> +				  | FLAG_HAS_FLASH
> +				  | FLAG_HAS_JUMBO_FRAMES
> +				  | FLAG_APME_IN_WUC,
> +	.flags2			= FLAG2_HAS_PHY_STATS
> +				  | FLAG2_HAS_EEE,
> +	.pba			= 26,
> +	.max_hw_frame_size	= 9022,
> +	.get_variants		= e1000_get_variants_ich8lan,
> +	.mac_ops		= &ich8_mac_ops,
> +	.phy_ops		= &ich8_phy_ops,
> +	.nvm_ops		= &spt_nvm_ops,
> +};
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index d150dade06cf..5835d6cf2f51 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -51,6 +51,7 @@ static const struct e1000_info *e1000_info_tbl[] = {
>   	[board_pch_lpt]		= &e1000_pch_lpt_info,
>   	[board_pch_spt]		= &e1000_pch_spt_info,
>   	[board_pch_cnp]		= &e1000_pch_cnp_info,
> +	[board_pch_tgp]		= &e1000_pch_tgp_info,
>   };
>   
>   struct e1000_reg_info {
> @@ -7843,12 +7844,12 @@ static const struct pci_device_id e1000_pci_tbl[] = {
>   	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_V11), board_pch_cnp },
>   	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_LM12), board_pch_spt },
>   	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_V12), board_pch_spt },
> -	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM13), board_pch_cnp },
> -	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V13), board_pch_cnp },
> -	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM14), board_pch_cnp },
> -	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V14), board_pch_cnp },
> -	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM15), board_pch_cnp },
> -	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V15), board_pch_cnp },
> +	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM13), board_pch_tgp },
> +	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V13), board_pch_tgp },
> +	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM14), board_pch_tgp },
> +	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V14), board_pch_tgp },
> +	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM15), board_pch_tgp },
> +	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V15), board_pch_tgp },
>   	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_LM16), board_pch_cnp },
>   	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_V16), board_pch_cnp },
>   	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_LM17), board_pch_cnp },
> 
Thanks,
Sasha

Re: [Intel-wired-lan] [PATCH 1/3] e1000e: Separate TGP from SPT

[Kai-Heng Feng]

Hi Sasha,

On Wed, Jul 14, 2021 at 1:58 AM Sasha Neftin <sasha.neftin@intel.com> wrote:
>
> On 7/12/2021 16:34, Kai-Heng Feng wrote:
> > Separate TGP from SPT so we can apply specific quirks to TGP.
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >   drivers/net/ethernet/intel/e1000e/e1000.h   |  4 +++-
> >   drivers/net/ethernet/intel/e1000e/ich8lan.c | 20 ++++++++++++++++++++
> >   drivers/net/ethernet/intel/e1000e/netdev.c  | 13 +++++++------
> >   3 files changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
> > index 5b2143f4b1f8..3178efd98006 100644
> > --- a/drivers/net/ethernet/intel/e1000e/e1000.h
> > +++ b/drivers/net/ethernet/intel/e1000e/e1000.h
> > @@ -113,7 +113,8 @@ enum e1000_boards {
> >       board_pch2lan,
> >       board_pch_lpt,
> >       board_pch_spt,
> > -     board_pch_cnp
> > +     board_pch_cnp,
> Hello Kai-Heng,
> I would agree with you here. I would suggest extending it also for other
> PCH (at least ADP and MTP).  The same controller on a different PCH.
> We will be able to differentiate between boards via MAC type and submit
> quirks if need.

Sure, will do in v2.

The issue patch [3/3] addresses may be fixed by [1], but I'll need to
dig the affected system out and do some testing.
Meanwhile, many users are affected by the RX issue patch [2/3]
addresses, so it'll be great if someone can review it.

[1] https://patchwork.ozlabs.org/project/intel-wired-lan/list/?series=250480

Kai-Heng

> > +     board_pch_tgp
> >   };
> >
> >   struct e1000_ps_page {
> > @@ -499,6 +500,7 @@ extern const struct e1000_info e1000_pch2_info;
> >   extern const struct e1000_info e1000_pch_lpt_info;
> >   extern const struct e1000_info e1000_pch_spt_info;
> >   extern const struct e1000_info e1000_pch_cnp_info;
> > +extern const struct e1000_info e1000_pch_tgp_info;
> >   extern const struct e1000_info e1000_es2_info;
> >
> >   void e1000e_ptp_init(struct e1000_adapter *adapter);
> > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> > index cf7b3887da1d..654dbe798e55 100644
> > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> > @@ -5967,3 +5967,23 @@ const struct e1000_info e1000_pch_cnp_info = {
> >       .phy_ops                = &ich8_phy_ops,
> >       .nvm_ops                = &spt_nvm_ops,
> >   };
> > +
> > +const struct e1000_info e1000_pch_tgp_info = {
> > +     .mac                    = e1000_pch_tgp,
> > +     .flags                  = FLAG_IS_ICH
> > +                               | FLAG_HAS_WOL
> > +                               | FLAG_HAS_HW_TIMESTAMP
> > +                               | FLAG_HAS_CTRLEXT_ON_LOAD
> > +                               | FLAG_HAS_AMT
> > +                               | FLAG_HAS_FLASH
> > +                               | FLAG_HAS_JUMBO_FRAMES
> > +                               | FLAG_APME_IN_WUC,
> > +     .flags2                 = FLAG2_HAS_PHY_STATS
> > +                               | FLAG2_HAS_EEE,
> > +     .pba                    = 26,
> > +     .max_hw_frame_size      = 9022,
> > +     .get_variants           = e1000_get_variants_ich8lan,
> > +     .mac_ops                = &ich8_mac_ops,
> > +     .phy_ops                = &ich8_phy_ops,
> > +     .nvm_ops                = &spt_nvm_ops,
> > +};
> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > index d150dade06cf..5835d6cf2f51 100644
> > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > @@ -51,6 +51,7 @@ static const struct e1000_info *e1000_info_tbl[] = {
> >       [board_pch_lpt]         = &e1000_pch_lpt_info,
> >       [board_pch_spt]         = &e1000_pch_spt_info,
> >       [board_pch_cnp]         = &e1000_pch_cnp_info,
> > +     [board_pch_tgp]         = &e1000_pch_tgp_info,
> >   };
> >
> >   struct e1000_reg_info {
> > @@ -7843,12 +7844,12 @@ static const struct pci_device_id e1000_pci_tbl[] = {
> >       { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_V11), board_pch_cnp },
> >       { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_LM12), board_pch_spt },
> >       { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_V12), board_pch_spt },
> > -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM13), board_pch_cnp },
> > -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V13), board_pch_cnp },
> > -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM14), board_pch_cnp },
> > -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V14), board_pch_cnp },
> > -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM15), board_pch_cnp },
> > -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V15), board_pch_cnp },
> > +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM13), board_pch_tgp },
> > +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V13), board_pch_tgp },
> > +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM14), board_pch_tgp },
> > +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V14), board_pch_tgp },
> > +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM15), board_pch_tgp },
> > +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V15), board_pch_tgp },
> >       { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_LM16), board_pch_cnp },
> >       { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_V16), board_pch_cnp },
> >       { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_LM17), board_pch_cnp },
> >
> Thanks,
> Sasha

Re: [Intel-wired-lan] [PATCH 2/3] e1000e: Make mei_me active when e1000e is in use

[Sasha Neftin]

On 7/12/2021 16:34, Kai-Heng Feng wrote:
> Many users report rather sluggish RX speed on TGP I219. Since
> "intel_idle.max_cstate=1" doesn't help, so it's not caused by deeper
> package C-state.
> 
> A workaround that always works is to make sure mei_me is runtime active
> when e1000e is in use.
> 
> The root cause is still unknown, but since many users are affected by
> the issue, implment the quirk in the driver as a temporary workaround.
Hello Kai-Heng,
First - thanks for the investigation of this problem. As I know CSME/AMT 
not POR on Linux and not supported. Recently we started add support for 
CSME/AMT none provisioned version (handshake with CSME in s0ix flow - 
only CSME with s0ix will support). It is not related to rx bandwidth 
problem.
I do not know how MEI driver affect 1Gbe driver - so, I would suggest to 
involve our CSME engineer (alexander.usyskin@intel.com) and try to 
investigate this problem.
Does this problem observed on Dell systems? As I heard no reproduction 
on Intel's RVP platform.
Another question: does disable mei_me runpm solve your problem?
> 
> Also adds mei_me as soft dependency to ensure the device link can be
> created if e1000e is in initramfs.
> 
> BugLink: https://bugs.launchpad.net/bugs/1927925
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213377
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213651
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>   drivers/net/ethernet/intel/e1000e/netdev.c | 26 ++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 5835d6cf2f51..e63445a8ce12 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -7317,6 +7317,27 @@ static const struct net_device_ops e1000e_netdev_ops = {
>   	.ndo_features_check	= passthru_features_check,
>   };
>   
> +static void e1000e_create_device_links(struct pci_dev *pdev)
> +{
> +	struct pci_dev *tgp_mei_me;
> +
> +	/* Find TGP mei_me devices and make e1000e power depend on mei_me */
> +	tgp_mei_me = pci_get_device(PCI_VENDOR_ID_INTEL, 0xa0e0, NULL);
> +	if (!tgp_mei_me) {
> +		tgp_mei_me = pci_get_device(PCI_VENDOR_ID_INTEL, 0x43e0, NULL);
> +		if (!tgp_mei_me)
> +			return;
> +	}
> +
> +	if (device_link_add(&pdev->dev, &tgp_mei_me->dev,
> +			    DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE |
> +			    DL_FLAG_AUTOREMOVE_CONSUMER))
> +		pci_info(pdev, "System and runtime PM depends on %s\n",
> +			 pci_name(tgp_mei_me));
> +
> +	pci_dev_put(tgp_mei_me);
> +}
> +
>   /**
>    * e1000_probe - Device Initialization Routine
>    * @pdev: PCI device information struct
> @@ -7645,6 +7666,9 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	if (pci_dev_run_wake(pdev) && hw->mac.type != e1000_pch_cnp)
>   		pm_runtime_put_noidle(&pdev->dev);
>   
> +	if (hw->mac.type == e1000_pch_tgp)
> +		e1000e_create_device_links(pdev);
> +
>   	return 0;
>   
>   err_register:
> @@ -7917,6 +7941,8 @@ static void __exit e1000_exit_module(void)
>   }
>   module_exit(e1000_exit_module);
>   
> +/* Ensure device link can be created if e1000e is in the initramfs. */
> +MODULE_SOFTDEP("pre: mei_me");
>   MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
>   MODULE_DESCRIPTION("Intel(R) PRO/1000 Network Driver");
>   MODULE_LICENSE("GPL v2");
> 
Thanks,Sasha

Re: [Intel-wired-lan] [PATCH 2/3] e1000e: Make mei_me active when e1000e is in use

[Kai-Heng Feng]

Hi Sasha,

On Wed, Jul 14, 2021 at 1:39 PM Sasha Neftin <sasha.neftin@intel.com> wrote:
>
> On 7/12/2021 16:34, Kai-Heng Feng wrote:
> > Many users report rather sluggish RX speed on TGP I219. Since
> > "intel_idle.max_cstate=1" doesn't help, so it's not caused by deeper
> > package C-state.
> >
> > A workaround that always works is to make sure mei_me is runtime active
> > when e1000e is in use.
> >
> > The root cause is still unknown, but since many users are affected by
> > the issue, implment the quirk in the driver as a temporary workaround.
> Hello Kai-Heng,
> First - thanks for the investigation of this problem. As I know CSME/AMT
> not POR on Linux and not supported. Recently we started add support for
> CSME/AMT none provisioned version (handshake with CSME in s0ix flow -
> only CSME with s0ix will support). It is not related to rx bandwidth
> problem.

I am aware that ME is not POR under Linux, so the commit message
states clearly that the patch is just a "temporary workaround".
Not every laptop can disable ME/AMT, and I don't think asking user to
fiddle with BIOS is a good thing, hence the patch.

> I do not know how MEI driver affect 1Gbe driver - so, I would suggest to
> involve our CSME engineer (alexander.usyskin@intel.com) and try to
> investigate this problem.
> Does this problem observed on Dell systems? As I heard no reproduction
> on Intel's RVP platform.
> Another question: does disable mei_me runpm solve your problem?

Yes, disabling runpm on mei_me can workaround the issue, and that's
essentially what this patch does by adding DL_FLAG_PM_RUNTIME |
DL_FLAG_RPM_ACTIVE flag.

Kai-Heng

> >
> > Also adds mei_me as soft dependency to ensure the device link can be
> > created if e1000e is in initramfs.
> >
> > BugLink: https://bugs.launchpad.net/bugs/1927925
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213377
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213651
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >   drivers/net/ethernet/intel/e1000e/netdev.c | 26 ++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > index 5835d6cf2f51..e63445a8ce12 100644
> > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > @@ -7317,6 +7317,27 @@ static const struct net_device_ops e1000e_netdev_ops = {
> >       .ndo_features_check     = passthru_features_check,
> >   };
> >
> > +static void e1000e_create_device_links(struct pci_dev *pdev)
> > +{
> > +     struct pci_dev *tgp_mei_me;
> > +
> > +     /* Find TGP mei_me devices and make e1000e power depend on mei_me */
> > +     tgp_mei_me = pci_get_device(PCI_VENDOR_ID_INTEL, 0xa0e0, NULL);
> > +     if (!tgp_mei_me) {
> > +             tgp_mei_me = pci_get_device(PCI_VENDOR_ID_INTEL, 0x43e0, NULL);
> > +             if (!tgp_mei_me)
> > +                     return;
> > +     }
> > +
> > +     if (device_link_add(&pdev->dev, &tgp_mei_me->dev,
> > +                         DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE |
> > +                         DL_FLAG_AUTOREMOVE_CONSUMER))
> > +             pci_info(pdev, "System and runtime PM depends on %s\n",
> > +                      pci_name(tgp_mei_me));
> > +
> > +     pci_dev_put(tgp_mei_me);
> > +}
> > +
> >   /**
> >    * e1000_probe - Device Initialization Routine
> >    * @pdev: PCI device information struct
> > @@ -7645,6 +7666,9 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >       if (pci_dev_run_wake(pdev) && hw->mac.type != e1000_pch_cnp)
> >               pm_runtime_put_noidle(&pdev->dev);
> >
> > +     if (hw->mac.type == e1000_pch_tgp)
> > +             e1000e_create_device_links(pdev);
> > +
> >       return 0;
> >
> >   err_register:
> > @@ -7917,6 +7941,8 @@ static void __exit e1000_exit_module(void)
> >   }
> >   module_exit(e1000_exit_module);
> >
> > +/* Ensure device link can be created if e1000e is in the initramfs. */
> > +MODULE_SOFTDEP("pre: mei_me");
> >   MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
> >   MODULE_DESCRIPTION("Intel(R) PRO/1000 Network Driver");
> >   MODULE_LICENSE("GPL v2");
> >
> Thanks,Sasha

Re: [Intel-wired-lan] [PATCH 1/3] e1000e: Separate TGP from SPT

[Sasha Neftin]

On 7/14/2021 07:19, Kai-Heng Feng wrote:
> Hi Sasha,
> 
> On Wed, Jul 14, 2021 at 1:58 AM Sasha Neftin <sasha.neftin@intel.com> wrote:
>>
>> On 7/12/2021 16:34, Kai-Heng Feng wrote:
>>> Separate TGP from SPT so we can apply specific quirks to TGP.
>>>
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>> ---
>>>    drivers/net/ethernet/intel/e1000e/e1000.h   |  4 +++-
>>>    drivers/net/ethernet/intel/e1000e/ich8lan.c | 20 ++++++++++++++++++++
>>>    drivers/net/ethernet/intel/e1000e/netdev.c  | 13 +++++++------
>>>    3 files changed, 30 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
>>> index 5b2143f4b1f8..3178efd98006 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/e1000.h
>>> +++ b/drivers/net/ethernet/intel/e1000e/e1000.h
>>> @@ -113,7 +113,8 @@ enum e1000_boards {
>>>        board_pch2lan,
>>>        board_pch_lpt,
>>>        board_pch_spt,
>>> -     board_pch_cnp
>>> +     board_pch_cnp,
>> Hello Kai-Heng,
>> I would agree with you here. I would suggest extending it also for other
>> PCH (at least ADP and MTP).  The same controller on a different PCH.
>> We will be able to differentiate between boards via MAC type and submit
>> quirks if need.
> 
> Sure, will do in v2.
> 
> The issue patch [3/3] addresses may be fixed by [1], but I'll need to
> dig the affected system out and do some testing.
> Meanwhile, many users are affected by the RX issue patch [2/3]
> addresses, so it'll be great if someone can review it.
> 
> [1] https://patchwork.ozlabs.org/project/intel-wired-lan/list/?series=250480
regards patches 2/3 and 3/3: looks it is not right place for temporary 
w.a. Let's work with alexander.usyskin@intel.com to understand to root 
cause and right place.
> 
> Kai-Heng
> 
>>> +     board_pch_tgp
>>>    };
>>>
>>>    struct e1000_ps_page {
>>> @@ -499,6 +500,7 @@ extern const struct e1000_info e1000_pch2_info;
>>>    extern const struct e1000_info e1000_pch_lpt_info;
>>>    extern const struct e1000_info e1000_pch_spt_info;
>>>    extern const struct e1000_info e1000_pch_cnp_info;
>>> +extern const struct e1000_info e1000_pch_tgp_info;
>>>    extern const struct e1000_info e1000_es2_info;
>>>
>>>    void e1000e_ptp_init(struct e1000_adapter *adapter);
>>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> index cf7b3887da1d..654dbe798e55 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> @@ -5967,3 +5967,23 @@ const struct e1000_info e1000_pch_cnp_info = {
>>>        .phy_ops                = &ich8_phy_ops,
>>>        .nvm_ops                = &spt_nvm_ops,
>>>    };
>>> +
>>> +const struct e1000_info e1000_pch_tgp_info = {
>>> +     .mac                    = e1000_pch_tgp,
>>> +     .flags                  = FLAG_IS_ICH
>>> +                               | FLAG_HAS_WOL
>>> +                               | FLAG_HAS_HW_TIMESTAMP
>>> +                               | FLAG_HAS_CTRLEXT_ON_LOAD
>>> +                               | FLAG_HAS_AMT
>>> +                               | FLAG_HAS_FLASH
>>> +                               | FLAG_HAS_JUMBO_FRAMES
>>> +                               | FLAG_APME_IN_WUC,
>>> +     .flags2                 = FLAG2_HAS_PHY_STATS
>>> +                               | FLAG2_HAS_EEE,
>>> +     .pba                    = 26,
>>> +     .max_hw_frame_size      = 9022,
>>> +     .get_variants           = e1000_get_variants_ich8lan,
>>> +     .mac_ops                = &ich8_mac_ops,
>>> +     .phy_ops                = &ich8_phy_ops,
>>> +     .nvm_ops                = &spt_nvm_ops,
>>> +};
>>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> index d150dade06cf..5835d6cf2f51 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> @@ -51,6 +51,7 @@ static const struct e1000_info *e1000_info_tbl[] = {
>>>        [board_pch_lpt]         = &e1000_pch_lpt_info,
>>>        [board_pch_spt]         = &e1000_pch_spt_info,
>>>        [board_pch_cnp]         = &e1000_pch_cnp_info,
>>> +     [board_pch_tgp]         = &e1000_pch_tgp_info,
>>>    };
>>>
>>>    struct e1000_reg_info {
>>> @@ -7843,12 +7844,12 @@ static const struct pci_device_id e1000_pci_tbl[] = {
>>>        { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_V11), board_pch_cnp },
>>>        { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_LM12), board_pch_spt },
>>>        { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_CMP_I219_V12), board_pch_spt },
>>> -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM13), board_pch_cnp },
>>> -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V13), board_pch_cnp },
>>> -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM14), board_pch_cnp },
>>> -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V14), board_pch_cnp },
>>> -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM15), board_pch_cnp },
>>> -     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V15), board_pch_cnp },
>>> +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM13), board_pch_tgp },
>>> +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V13), board_pch_tgp },
>>> +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM14), board_pch_tgp },
>>> +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V14), board_pch_tgp },
>>> +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_LM15), board_pch_tgp },
>>> +     { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_TGP_I219_V15), board_pch_tgp },
>>>        { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_LM16), board_pch_cnp },
>>>        { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_V16), board_pch_cnp },
>>>        { PCI_VDEVICE(INTEL, E1000_DEV_ID_PCH_ADP_I219_LM17), board_pch_cnp },
>>>
>> Thanks,
>> Sasha

Re: [Intel-wired-lan] [PATCH 2/3] e1000e: Make mei_me active when e1000e is in use

[Ruinskiy, Dima]

On 14/07/2021 9:28, Kai-Heng Feng wrote:
>> I do not know how MEI driver affect 1Gbe driver - so, I would suggest to
>> involve our CSME engineer (alexander.usyskin@intel.com) and try to
>> investigate this problem.
>> Does this problem observed on Dell systems? As I heard no reproduction
>> on Intel's RVP platform.
>> Another question: does disable mei_me runpm solve your problem?
> 
> Yes, disabling runpm on mei_me can workaround the issue, and that's
> essentially what this patch does by adding DL_FLAG_PM_RUNTIME |
> DL_FLAG_RPM_ACTIVE flag.
> 
> Kai-Heng
Hi, Kai-Heng,

If the goal of the patch is to essentially disable runpm on mei_me, then 
why is the patch touching code in the e1000e driver?

I agree with Sasha Neftin; it seems like the wrong location, and the 
wrong way to do it, even if it currently works. We need to understand 
what causes runpm of mei_me to adversely affect LAN Rx, and for this we 
need the involvement of mei_me owners.

--Dima
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [Intel-wired-lan] [PATCH 2/3] e1000e: Make mei_me active when e1000e is in use

[Kai-Heng Feng]

On Wed, Jul 14, 2021 at 5:06 PM Ruinskiy, Dima <dima.ruinskiy@intel.com> wrote:
>
> On 14/07/2021 9:28, Kai-Heng Feng wrote:
> >> I do not know how MEI driver affect 1Gbe driver - so, I would suggest to
> >> involve our CSME engineer (alexander.usyskin@intel.com) and try to
> >> investigate this problem.
> >> Does this problem observed on Dell systems? As I heard no reproduction
> >> on Intel's RVP platform.
> >> Another question: does disable mei_me runpm solve your problem?
> >
> > Yes, disabling runpm on mei_me can workaround the issue, and that's
> > essentially what this patch does by adding DL_FLAG_PM_RUNTIME |
> > DL_FLAG_RPM_ACTIVE flag.
> >
> > Kai-Heng
> Hi, Kai-Heng,
>
> If the goal of the patch is to essentially disable runpm on mei_me, then
> why is the patch touching code in the e1000e driver?

We can put the workaround in e1000e, mei_me or as PCI quirk.
But since the bug itself manifests in e1000e, I think it's more
appropriate to put it here.

To be more specific, it doesn't disable runtime suspend on mei_me, it
makes mei_me the power supplier of e1000e.
So when e1000e can be runtime suspended (i.e. no link partner), mei_me
can also get runtime suspended too.

>
> I agree with Sasha Neftin; it seems like the wrong location, and the
> wrong way to do it, even if it currently works. We need to understand
> what causes runpm of mei_me to adversely affect LAN Rx, and for this we
> need the involvement of mei_me owners.

I think it's the right location, however I totally agree with your
other arguments.
There are many users already affected by this bug, so if a proper fix
isn't available for now, the temporary workaround can help here.

Kai-Heng

>
> --Dima
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

Re: [Intel-wired-lan] [PATCH 2/3] e1000e: Make mei_me active when e1000e is in use

[Sasha Neftin]

On 7/14/2021 12:52, Kai-Heng Feng wrote:
> On Wed, Jul 14, 2021 at 5:06 PM Ruinskiy, Dima <dima.ruinskiy@intel.com> wrote:
>>
>> On 14/07/2021 9:28, Kai-Heng Feng wrote:
>>>> I do not know how MEI driver affect 1Gbe driver - so, I would suggest to
>>>> involve our CSME engineer (alexander.usyskin@intel.com) and try to
>>>> investigate this problem.
>>>> Does this problem observed on Dell systems? As I heard no reproduction
>>>> on Intel's RVP platform.
>>>> Another question: does disable mei_me runpm solve your problem?
>>>
>>> Yes, disabling runpm on mei_me can workaround the issue, and that's
>>> essentially what this patch does by adding DL_FLAG_PM_RUNTIME |
>>> DL_FLAG_RPM_ACTIVE flag.
>>>
>>> Kai-Heng
>> Hi, Kai-Heng,
>>
>> If the goal of the patch is to essentially disable runpm on mei_me, then
>> why is the patch touching code in the e1000e driver?
> 
> We can put the workaround in e1000e, mei_me or as PCI quirk.
> But since the bug itself manifests in e1000e, I think it's more
> appropriate to put it here.
> 
> To be more specific, it doesn't disable runtime suspend on mei_me, it
> makes mei_me the power supplier of e1000e.
> So when e1000e can be runtime suspended (i.e. no link partner), mei_me
> can also get runtime suspended too.
>>
>> I agree with Sasha Neftin; it seems like the wrong location, and the
>> wrong way to do it, even if it currently works. We need to understand
>> what causes runpm of mei_me to adversely affect LAN Rx, and for this we
>> need the involvement of mei_me owners.
> 
> I think it's the right location, however I totally agree with your
> other arguments.
> There are many users already affected by this bug, so if a proper fix
> isn't available for now, the temporary workaround can help here.
Hello Kai-Heng,
The temporary workaround without root cause is vague. Please, let's work 
with the manageability FW engineer (alexander.usyskin@intel.com) and 
understand the root cause.
Also, here is interfere with runpm flow of CSME - we must their inputs.
sasha
> 
> Kai-Heng
> 
>>
>> --Dima
>> ---------------------------------------------------------------------
>> Intel Israel (74) Limited
>>
>> This e-mail and any attachments may contain confidential material for
>> the sole use of the intended recipient(s). Any review or distribution
>> by others is strictly prohibited. If you are not the intended
>> recipient, please contact the sender and delete all copies.

Re: [PATCH 3/3] e1000e: Serialize TGP e1000e PM ops

[Kai-Heng Feng]

Hi Sasha,

On Mon, Jul 12, 2021 at 9:35 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> On TGL systems, PCI_COMMAND may randomly flip to 0 on system resume.
> This is devastating to drivers that use pci_set_master(), like NVMe and
> xHCI, to enable DMA in their resume routine, as pci_set_master() can
> inadvertently disable PCI_COMMAND_IO and PCI_COMMAND_MEMORY, making
> resources inaccessible.
>
> The issue is reproducible on all kernel releases, but the situation is
> exacerbated by commit 6cecf02e77ab ("Revert "e1000e: disable s0ix entry
> and exit flows for ME systems"").
>
> Seems like ME can do many things to other PCI devices until it's finally out of
> ULP polling. So ensure e1000e PM ops are serialized by enforcing suspend/resume
> order to workaround the issue.
>
> Of course this will make system suspend and resume a bit slower, but we
> probably need to settle on this workaround until ME is fully supported.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212039
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Series "e1000e: Add handshake with the CSME to support s0ix" doesn't
fix the issue, so this patch is still needed.

Kai-Heng

> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index e63445a8ce12..0244d3dd90a3 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -7319,7 +7319,8 @@ static const struct net_device_ops e1000e_netdev_ops = {
>
>  static void e1000e_create_device_links(struct pci_dev *pdev)
>  {
> -       struct pci_dev *tgp_mei_me;
> +       struct pci_bus *bus = pdev->bus;
> +       struct pci_dev *tgp_mei_me, *p;
>
>         /* Find TGP mei_me devices and make e1000e power depend on mei_me */
>         tgp_mei_me = pci_get_device(PCI_VENDOR_ID_INTEL, 0xa0e0, NULL);
> @@ -7335,6 +7336,17 @@ static void e1000e_create_device_links(struct pci_dev *pdev)
>                 pci_info(pdev, "System and runtime PM depends on %s\n",
>                          pci_name(tgp_mei_me));
>
> +       /* Find other devices in the SoC and make them depend on e1000e */
> +       list_for_each_entry(p, &bus->devices, bus_list) {
> +               if (&p->dev == &pdev->dev || &p->dev == &tgp_mei_me->dev)
> +                       continue;
> +
> +               if (device_link_add(&p->dev, &pdev->dev,
> +                                   DL_FLAG_AUTOREMOVE_SUPPLIER))
> +                       pci_info(p, "System PM depends on %s\n",
> +                                pci_name(pdev));
> +       }
> +
>         pci_dev_put(tgp_mei_me);
>  }
>
> --
> 2.31.1
>

Re: [PATCH 3/3] e1000e: Serialize TGP e1000e PM ops

[Sasha Neftin]

On 7/27/2021 09:53, Kai-Heng Feng wrote:
> Hi Sasha,
> 
> On Mon, Jul 12, 2021 at 9:35 PM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>>
>> On TGL systems, PCI_COMMAND may randomly flip to 0 on system resume.
>> This is devastating to drivers that use pci_set_master(), like NVMe and
>> xHCI, to enable DMA in their resume routine, as pci_set_master() can
>> inadvertently disable PCI_COMMAND_IO and PCI_COMMAND_MEMORY, making
>> resources inaccessible.
>>
>> The issue is reproducible on all kernel releases, but the situation is
>> exacerbated by commit 6cecf02e77ab ("Revert "e1000e: disable s0ix entry
>> and exit flows for ME systems"").
>>
>> Seems like ME can do many things to other PCI devices until it's finally out of
>> ULP polling. So ensure e1000e PM ops are serialized by enforcing suspend/resume
>> order to workaround the issue.
>>
>> Of course this will make system suspend and resume a bit slower, but we
>> probably need to settle on this workaround until ME is fully supported.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212039
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> Series "e1000e: Add handshake with the CSME to support s0ix" doesn't
> fix the issue, so this patch is still needed.
Hello Kai-Heng,
This problem is still under investigation by the ME team. Let's wait for 
their response.
Series "e1000e: Add handshake with the CSME to support s0ix" - support 
only s0ix flow on AMT/CSME none provisioned systems and not related to 
this problem.
> 
> Kai-Heng
> 
>> ---
>>   drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>> index e63445a8ce12..0244d3dd90a3 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -7319,7 +7319,8 @@ static const struct net_device_ops e1000e_netdev_ops = {
>>
>>   static void e1000e_create_device_links(struct pci_dev *pdev)
>>   {
>> -       struct pci_dev *tgp_mei_me;
>> +       struct pci_bus *bus = pdev->bus;
>> +       struct pci_dev *tgp_mei_me, *p;
>>
>>          /* Find TGP mei_me devices and make e1000e power depend on mei_me */
>>          tgp_mei_me = pci_get_device(PCI_VENDOR_ID_INTEL, 0xa0e0, NULL);
>> @@ -7335,6 +7336,17 @@ static void e1000e_create_device_links(struct pci_dev *pdev)
>>                  pci_info(pdev, "System and runtime PM depends on %s\n",
>>                           pci_name(tgp_mei_me));
>>
>> +       /* Find other devices in the SoC and make them depend on e1000e */
>> +       list_for_each_entry(p, &bus->devices, bus_list) {
>> +               if (&p->dev == &pdev->dev || &p->dev == &tgp_mei_me->dev)
>> +                       continue;
>> +
>> +               if (device_link_add(&p->dev, &pdev->dev,
>> +                                   DL_FLAG_AUTOREMOVE_SUPPLIER))
>> +                       pci_info(p, "System PM depends on %s\n",
>> +                                pci_name(pdev));
>> +       }
>> +
>>          pci_dev_put(tgp_mei_me);
>>   }
>>
>> --
>> 2.31.1
>>
sasha