| Message ID | 20210922065542.3780389-1-sasha.neftin@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [v1,1/2] e1000e: Separate TGP board type from SPT | expand |
Dear Sasha, THank you for the patch. Am 22.09.21 um 08:55 schrieb Sasha Neftin: Please use imperative mood in the commit message summary: Fix …. Maybe: e1000e: Fix packet loss on Tiger Lake and later > Update the HW MAC initialization flow. Do not gate DMA clock from > the modPHY block. Keeping this clock will prevent drop packets sent dropped > in burst mode on the Kumeran interface. What is Kumeran? Where is the new HW MAC initialization flow documented? The spec or some errata? How can the bug be reproduced? > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213651 > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213377 > Fixes: bc7f75fa9788 ("New pci-express e1000 driver"); > Signed-off-by: Sasha Neftin <sasha.neftin@intel.com> > --- > drivers/net/ethernet/intel/e1000e/ich8lan.c | 11 ++++++++++- > drivers/net/ethernet/intel/e1000e/ich8lan.h | 3 +++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c > index 66d7196310e2..5e4fc9b4e2ad 100644 > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c > @@ -4813,7 +4813,7 @@ static s32 e1000_reset_hw_ich8lan(struct e1000_hw *hw) > static s32 e1000_init_hw_ich8lan(struct e1000_hw *hw) > { > struct e1000_mac_info *mac = &hw->mac; > - u32 ctrl_ext, txdctl, snoop; > + u32 ctrl_ext, txdctl, snoop, fflt_dbg; > s32 ret_val; > u16 i; > > @@ -4872,6 +4872,15 @@ static s32 e1000_init_hw_ich8lan(struct e1000_hw *hw) > snoop = (u32)~(PCIE_NO_SNOOP_ALL); > e1000e_set_pcie_no_snoop(hw, snoop); > > + /* Enable workaround for packet loss issue on TGP PCH Maybe: > Work around packet loss issue on TGP PCH and later > + * Do not gate DMA clock from the modPHY block > + */ > + if (mac->type >= e1000_pch_tgp) { > + fflt_dbg = er32(FFLT_DBG); Maybe the variable `ctrl_ext` could be renamed to `tmp` or `tmp32`, and reused. > + fflt_dbg |= E1000_FFLT_DBG_DONT_GATE_WAKE_DMA_CLK; > + ew32(FFLT_DBG, fflt_dbg); > + } > + > ctrl_ext = er32(CTRL_EXT); > ctrl_ext |= E1000_CTRL_EXT_RO_DIS; > ew32(CTRL_EXT, ctrl_ext); > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.h b/drivers/net/ethernet/intel/e1000e/ich8lan.h > index d6a092e5ee74..2504b11c3169 100644 > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.h > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.h > @@ -289,6 +289,9 @@ > /* Proprietary Latency Tolerance Reporting PCI Capability */ > #define E1000_PCI_LTR_CAP_LPT 0xA8 > > +/* Don't gate wake DMA clock */ > +#define E1000_FFLT_DBG_DONT_GATE_WAKE_DMA_CLK 0x1000 > + > void e1000e_write_protect_nvm_ich8lan(struct e1000_hw *hw); > void e1000e_set_kmrn_lock_loss_workaround_ich8lan(struct e1000_hw *hw, > bool state); >
On 9/22/2021 10:09, Paul Menzel wrote: > Dear Sasha, > > > THank you for the patch. > > Am 22.09.21 um 08:55 schrieb Sasha Neftin: > > Please use imperative mood in the commit message summary: Fix …. Maybe: > > e1000e: Fix packet loss on Tiger Lake and later > >> Update the HW MAC initialization flow. Do not gate DMA clock from >> the modPHY block. Keeping this clock will prevent drop packets sent > > dropped > >> in burst mode on the Kumeran interface. > > What is Kumeran? interface to external Gigabit Ethernet PHY > > Where is the new HW MAC initialization flow documented? The spec or some > errata? > > How can the bug be reproduced? Described in bugzilla - please, make sure the burst traffic: run commands: tc qdisc add dev eno1 (lan device name) root netem delay 5 ms on client side iperf -s on server side iperf -c server_IP -R on client side > >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213651 >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213377 >> Fixes: bc7f75fa9788 ("New pci-express e1000 driver"); >> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com> >> --- >> drivers/net/ethernet/intel/e1000e/ich8lan.c | 11 ++++++++++- >> drivers/net/ethernet/intel/e1000e/ich8lan.h | 3 +++ >> 2 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c >> b/drivers/net/ethernet/intel/e1000e/ich8lan.c >> index 66d7196310e2..5e4fc9b4e2ad 100644 >> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c >> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c >> @@ -4813,7 +4813,7 @@ static s32 e1000_reset_hw_ich8lan(struct >> e1000_hw *hw) >> static s32 e1000_init_hw_ich8lan(struct e1000_hw *hw) >> { >> struct e1000_mac_info *mac = &hw->mac; >> - u32 ctrl_ext, txdctl, snoop; >> + u32 ctrl_ext, txdctl, snoop, fflt_dbg; >> s32 ret_val; >> u16 i; >> @@ -4872,6 +4872,15 @@ static s32 e1000_init_hw_ich8lan(struct >> e1000_hw *hw) >> snoop = (u32)~(PCIE_NO_SNOOP_ALL); >> e1000e_set_pcie_no_snoop(hw, snoop); >> + /* Enable workaround for packet loss issue on TGP PCH > > Maybe: > >> Work around packet loss issue on TGP PCH and later > >> + * Do not gate DMA clock from the modPHY block >> + */ >> + if (mac->type >= e1000_pch_tgp) { >> + fflt_dbg = er32(FFLT_DBG); > > Maybe the variable `ctrl_ext` could be renamed to `tmp` or `tmp32`, and > reused. I prefer to stay with a meaningful name > >> + fflt_dbg |= E1000_FFLT_DBG_DONT_GATE_WAKE_DMA_CLK; >> + ew32(FFLT_DBG, fflt_dbg); >> + } >> + >> ctrl_ext = er32(CTRL_EXT); >> ctrl_ext |= E1000_CTRL_EXT_RO_DIS; >> ew32(CTRL_EXT, ctrl_ext); >> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.h >> b/drivers/net/ethernet/intel/e1000e/ich8lan.h >> index d6a092e5ee74..2504b11c3169 100644 >> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.h >> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.h >> @@ -289,6 +289,9 @@ >> /* Proprietary Latency Tolerance Reporting PCI Capability */ >> #define E1000_PCI_LTR_CAP_LPT 0xA8 >> +/* Don't gate wake DMA clock */ >> +#define E1000_FFLT_DBG_DONT_GATE_WAKE_DMA_CLK 0x1000 >> + >> void e1000e_write_protect_nvm_ich8lan(struct e1000_hw *hw); >> void e1000e_set_kmrn_lock_loss_workaround_ich8lan(struct e1000_hw *hw, >> bool state); >>
Dear Sasha, Am 22.09.21 um 09:30 schrieb Sasha Neftin: > On 9/22/2021 10:09, Paul Menzel wrote: >> Am 22.09.21 um 08:55 schrieb Sasha Neftin: >> >> Please use imperative mood in the commit message summary: Fix …. Maybe: >> >> e1000e: Fix packet loss on Tiger Lake and later >> >>> Update the HW MAC initialization flow. Do not gate DMA clock from >>> the modPHY block. Keeping this clock will prevent drop packets sent >> >> dropped >> >>> in burst mode on the Kumeran interface. >> >> What is Kumeran? > interface to external Gigabit Ethernet PHY Thank you. For somebody not having all names memorized, this would be good to know in my opinion. >> Where is the new HW MAC initialization flow documented? The spec or >> some errata? >> >> How can the bug be reproduced? > Described in bugzilla - please, make sure the burst traffic: > run commands: > tc qdisc add dev eno1 (lan device name) root netem delay 5 ms on client side > iperf -s on server side > iperf -c server_IP -R on client side Thank you. In my opinion, reviewers should have this in the commit message, instead of reading through several comments in the Linux Bugzilla. Also, at least in #213651 the title and first comment talks about Intel ME (Management Engine). >>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213651 >>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213377 >>> Fixes: bc7f75fa9788 ("New pci-express e1000 driver"); >>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com> >>> --- >>> drivers/net/ethernet/intel/e1000e/ich8lan.c | 11 ++++++++++- >>> drivers/net/ethernet/intel/e1000e/ich8lan.h | 3 +++ >>> 2 files changed, 13 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c >>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c >>> index 66d7196310e2..5e4fc9b4e2ad 100644 >>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c >>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c >>> @@ -4813,7 +4813,7 @@ static s32 e1000_reset_hw_ich8lan(struct >>> e1000_hw *hw) >>> static s32 e1000_init_hw_ich8lan(struct e1000_hw *hw) >>> { >>> struct e1000_mac_info *mac = &hw->mac; >>> - u32 ctrl_ext, txdctl, snoop; >>> + u32 ctrl_ext, txdctl, snoop, fflt_dbg; >>> s32 ret_val; >>> u16 i; >>> @@ -4872,6 +4872,15 @@ static s32 e1000_init_hw_ich8lan(struct >>> e1000_hw *hw) >>> snoop = (u32)~(PCIE_NO_SNOOP_ALL); >>> e1000e_set_pcie_no_snoop(hw, snoop); >>> + /* Enable workaround for packet loss issue on TGP PCH >> >> Maybe: >> >>> Work around packet loss issue on TGP PCH and later >> >>> + * Do not gate DMA clock from the modPHY block >>> + */ >>> + if (mac->type >= e1000_pch_tgp) { >>> + fflt_dbg = er32(FFLT_DBG); >> >> Maybe the variable `ctrl_ext` could be renamed to `tmp` or `tmp32`, >> and reused. > I prefer to stay with a meaningful name Understood. I know it’s personal preference. >>> + fflt_dbg |= E1000_FFLT_DBG_DONT_GATE_WAKE_DMA_CLK; >>> + ew32(FFLT_DBG, fflt_dbg); >>> + } >>> + >>> ctrl_ext = er32(CTRL_EXT); >>> ctrl_ext |= E1000_CTRL_EXT_RO_DIS; >>> ew32(CTRL_EXT, ctrl_ext); >>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.h >>> b/drivers/net/ethernet/intel/e1000e/ich8lan.h >>> index d6a092e5ee74..2504b11c3169 100644 >>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.h >>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.h >>> @@ -289,6 +289,9 @@ >>> /* Proprietary Latency Tolerance Reporting PCI Capability */ >>> #define E1000_PCI_LTR_CAP_LPT 0xA8 >>> +/* Don't gate wake DMA clock */ >>> +#define E1000_FFLT_DBG_DONT_GATE_WAKE_DMA_CLK 0x1000 >>> + >>> void e1000e_write_protect_nvm_ich8lan(struct e1000_hw *hw); >>> void e1000e_set_kmrn_lock_loss_workaround_ich8lan(struct e1000_hw *hw, >>> bool state); >>> Kind regards, Paul
Thanks Sasha, Confirmed this fixes the problem on my X13 G2 Tested-by: Mark Pearson <markpearson@lenovo.com> Mark On 2021-09-22 09:29, Sasha Neftin wrote: > > > > -------- Forwarded Message -------- > Subject: [PATCH v1 2/2] e1000e: Fixing packet loss issues on new platforms > Date: Wed, 22 Sep 2021 09:55:42 +0300 > From: Sasha Neftin <sasha.neftin@intel.com> > To: intel-wired-lan@lists.osuosl.org > CC: Sasha Neftin <sasha.neftin@intel.com> > > Update the HW MAC initialization flow. Do not gate DMA clock from > the modPHY block. Keeping this clock will prevent drop packets sent > in burst mode on the Kumeran interface. > > Bugzilla: > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D213651&data=04%7C01%7Cmarkpearson%40lenovo.com%7Cb5b073b630794c8ff7b908d97dcd03a3%7C5c7d0b28bdf8410caa934df372b16203%7C1%7C0%7C637679141742309105%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1TJGrHWxUne7m%2BOPaAw5TuMAJHzOldg5Ws4xSY2Ew40%3D&reserved=0 > > Bugzilla: > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D213377&data=04%7C01%7Cmarkpearson%40lenovo.com%7Cb5b073b630794c8ff7b908d97dcd03a3%7C5c7d0b28bdf8410caa934df372b16203%7C1%7C0%7C637679141742319058%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BRuJa4pf%2FhTau%2BfDY1lMOI%2F0wCMUYh5bB%2FJHZjTJmQw%3D&reserved=0 > > Fixes: bc7f75fa9788 ("New pci-express e1000 driver"); > Signed-off-by: Sasha Neftin <sasha.neftin@intel.com> > --- > drivers/net/ethernet/intel/e1000e/ich8lan.c | 11 ++++++++++- > drivers/net/ethernet/intel/e1000e/ich8lan.h | 3 +++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c > b/drivers/net/ethernet/intel/e1000e/ich8lan.c > index 66d7196310e2..5e4fc9b4e2ad 100644 > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c > @@ -4813,7 +4813,7 @@ static s32 e1000_reset_hw_ich8lan(struct e1000_hw > *hw) > static s32 e1000_init_hw_ich8lan(struct e1000_hw *hw) > { > struct e1000_mac_info *mac = &hw->mac; > - u32 ctrl_ext, txdctl, snoop; > + u32 ctrl_ext, txdctl, snoop, fflt_dbg; > s32 ret_val; > u16 i; > @@ -4872,6 +4872,15 @@ static s32 e1000_init_hw_ich8lan(struct > e1000_hw *hw) > snoop = (u32)~(PCIE_NO_SNOOP_ALL); > e1000e_set_pcie_no_snoop(hw, snoop); > + /* Enable workaround for packet loss issue on TGP PCH > + * Do not gate DMA clock from the modPHY block > + */ > + if (mac->type >= e1000_pch_tgp) { > + fflt_dbg = er32(FFLT_DBG); > + fflt_dbg |= E1000_FFLT_DBG_DONT_GATE_WAKE_DMA_CLK; > + ew32(FFLT_DBG, fflt_dbg); > + } > + > ctrl_ext = er32(CTRL_EXT); > ctrl_ext |= E1000_CTRL_EXT_RO_DIS; > ew32(CTRL_EXT, ctrl_ext); > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.h > b/drivers/net/ethernet/intel/e1000e/ich8lan.h > index d6a092e5ee74..2504b11c3169 100644 > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.h > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.h > @@ -289,6 +289,9 @@ > /* Proprietary Latency Tolerance Reporting PCI Capability */ > #define E1000_PCI_LTR_CAP_LPT 0xA8 > +/* Don't gate wake DMA clock */ > +#define E1000_FFLT_DBG_DONT_GATE_WAKE_DMA_CLK 0x1000 > + > void e1000e_write_protect_nvm_ich8lan(struct e1000_hw *hw); > void e1000e_set_kmrn_lock_loss_workaround_ich8lan(struct e1000_hw *hw, > bool state);
On 9/22/2021 09:55, Sasha Neftin wrote: > Update the HW MAC initialization flow. Do not gate DMA clock from > the modPHY block. Keeping this clock will prevent drop packets sent > in burst mode on the Kumeran interface. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213651 > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213377 > Fixes: bc7f75fa9788 ("New pci-express e1000 driver"); > Signed-off-by: Sasha Neftin <sasha.neftin@intel.com> > --- > drivers/net/ethernet/intel/e1000e/ich8lan.c | 11 ++++++++++- > drivers/net/ethernet/intel/e1000e/ich8lan.h | 3 +++ > 2 files changed, 13 insertions(+), 1 deletion(-) > Tested-by: Nechama Kraus <nechamax.kraus@linux.intel.com>
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c index 66d7196310e2..5e4fc9b4e2ad 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c @@ -4813,7 +4813,7 @@ static s32 e1000_reset_hw_ich8lan(struct e1000_hw *hw) static s32 e1000_init_hw_ich8lan(struct e1000_hw *hw) { struct e1000_mac_info *mac = &hw->mac; - u32 ctrl_ext, txdctl, snoop; + u32 ctrl_ext, txdctl, snoop, fflt_dbg; s32 ret_val; u16 i; @@ -4872,6 +4872,15 @@ static s32 e1000_init_hw_ich8lan(struct e1000_hw *hw) snoop = (u32)~(PCIE_NO_SNOOP_ALL); e1000e_set_pcie_no_snoop(hw, snoop); + /* Enable workaround for packet loss issue on TGP PCH + * Do not gate DMA clock from the modPHY block + */ + if (mac->type >= e1000_pch_tgp) { + fflt_dbg = er32(FFLT_DBG); + fflt_dbg |= E1000_FFLT_DBG_DONT_GATE_WAKE_DMA_CLK; + ew32(FFLT_DBG, fflt_dbg); + } + ctrl_ext = er32(CTRL_EXT); ctrl_ext |= E1000_CTRL_EXT_RO_DIS; ew32(CTRL_EXT, ctrl_ext); diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.h b/drivers/net/ethernet/intel/e1000e/ich8lan.h index d6a092e5ee74..2504b11c3169 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.h +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.h @@ -289,6 +289,9 @@ /* Proprietary Latency Tolerance Reporting PCI Capability */ #define E1000_PCI_LTR_CAP_LPT 0xA8 +/* Don't gate wake DMA clock */ +#define E1000_FFLT_DBG_DONT_GATE_WAKE_DMA_CLK 0x1000 + void e1000e_write_protect_nvm_ich8lan(struct e1000_hw *hw); void e1000e_set_kmrn_lock_loss_workaround_ich8lan(struct e1000_hw *hw, bool state);
Update the HW MAC initialization flow. Do not gate DMA clock from the modPHY block. Keeping this clock will prevent drop packets sent in burst mode on the Kumeran interface. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213651 Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213377 Fixes: bc7f75fa9788 ("New pci-express e1000 driver"); Signed-off-by: Sasha Neftin <sasha.neftin@intel.com> --- drivers/net/ethernet/intel/e1000e/ich8lan.c | 11 ++++++++++- drivers/net/ethernet/intel/e1000e/ich8lan.h | 3 +++ 2 files changed, 13 insertions(+), 1 deletion(-)