diff mbox series

[v1,2/2] e1000e: Fixing packet loss issues on new platforms

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

Commit Message

Sasha Neftin Sept. 22, 2021, 6:55 a.m. UTC
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(-)

Comments

Paul Menzel Sept. 22, 2021, 7:09 a.m. UTC | #1
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);
>
Sasha Neftin Sept. 22, 2021, 7:30 a.m. UTC | #2
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);
>>
Paul Menzel Sept. 22, 2021, 11:01 a.m. UTC | #3
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
Mark Pearson Sept. 22, 2021, 1:56 p.m. UTC | #4
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&amp;data=04%7C01%7Cmarkpearson%40lenovo.com%7Cb5b073b630794c8ff7b908d97dcd03a3%7C5c7d0b28bdf8410caa934df372b16203%7C1%7C0%7C637679141742309105%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=1TJGrHWxUne7m%2BOPaAw5TuMAJHzOldg5Ws4xSY2Ew40%3D&amp;reserved=0 
> 
> Bugzilla: 
> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D213377&amp;data=04%7C01%7Cmarkpearson%40lenovo.com%7Cb5b073b630794c8ff7b908d97dcd03a3%7C5c7d0b28bdf8410caa934df372b16203%7C1%7C0%7C637679141742319058%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=BRuJa4pf%2FhTau%2BfDY1lMOI%2F0wCMUYh5bB%2FJHZjTJmQw%3D&amp;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);
Kraus, NechamaX Oct. 6, 2021, 12:20 p.m. UTC | #5
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 mbox series

Patch

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