[PATCH net] r8169: fix jumbo packet handling on resume from suspend

[PATCH net] r8169: fix jumbo packet handling on resume from suspend

[Heiner Kallweit]

Mariusz reported that invalid packets are sent after resume from
suspend if jumbo packets are active. It turned out that his BIOS
resets chip settings to non-jumbo on resume. Most chip settings are
re-initialized on resume from suspend by calling rtl_hw_start(),
so let's add configuring jumbo to this function.
There's nothing wrong with the commit marked as fixed, it's just
the first one where the patch applies cleanly.

Fixes: 7366016d2d4c ("r8169: read common register for PCI commit")
Reported-by: Mariusz Bialonczyk <manio@skyboo.net>
Tested-by: Mariusz Bialonczyk <manio@skyboo.net>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 35 +++++++----------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 74f81fe03..350b0d949 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -4146,6 +4146,14 @@ static void rtl_hw_jumbo_disable(struct rtl8169_private *tp)
 	rtl_lock_config_regs(tp);
 }
 
+static void rtl_jumbo_config(struct rtl8169_private *tp, int mtu)
+{
+	if (mtu > ETH_DATA_LEN)
+		rtl_hw_jumbo_enable(tp);
+	else
+		rtl_hw_jumbo_disable(tp);
+}
+
 DECLARE_RTL_COND(rtl_chipcmd_cond)
 {
 	return RTL_R8(tp, ChipCmd) & CmdReset;
@@ -4442,11 +4450,6 @@ static void rtl8168g_set_pause_thresholds(struct rtl8169_private *tp,
 static void rtl_hw_start_8168bb(struct rtl8169_private *tp)
 {
 	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en);
-
-	if (tp->dev->mtu <= ETH_DATA_LEN) {
-		rtl_tx_performance_tweak(tp, PCI_EXP_DEVCTL_READRQ_4096B |
-					 PCI_EXP_DEVCTL_NOSNOOP_EN);
-	}
 }
 
 static void rtl_hw_start_8168bef(struct rtl8169_private *tp)
@@ -4462,9 +4465,6 @@ static void __rtl_hw_start_8168cp(struct rtl8169_private *tp)
 
 	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en);
 
-	if (tp->dev->mtu <= ETH_DATA_LEN)
-		rtl_tx_performance_tweak(tp, PCI_EXP_DEVCTL_READRQ_4096B);
-
 	rtl_disable_clock_request(tp);
 }
 
@@ -4490,9 +4490,6 @@ static void rtl_hw_start_8168cp_2(struct rtl8169_private *tp)
 	rtl_set_def_aspm_entry_latency(tp);
 
 	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en);
-
-	if (tp->dev->mtu <= ETH_DATA_LEN)
-		rtl_tx_performance_tweak(tp, PCI_EXP_DEVCTL_READRQ_4096B);
 }
 
 static void rtl_hw_start_8168cp_3(struct rtl8169_private *tp)
@@ -4503,9 +4500,6 @@ static void rtl_hw_start_8168cp_3(struct rtl8169_private *tp)
 
 	/* Magic. */
 	RTL_W8(tp, DBG_REG, 0x20);
-
-	if (tp->dev->mtu <= ETH_DATA_LEN)
-		rtl_tx_performance_tweak(tp, PCI_EXP_DEVCTL_READRQ_4096B);
 }
 
 static void rtl_hw_start_8168c_1(struct rtl8169_private *tp)
@@ -4611,9 +4605,6 @@ static void rtl_hw_start_8168e_1(struct rtl8169_private *tp)
 
 	rtl_ephy_init(tp, e_info_8168e_1);
 
-	if (tp->dev->mtu <= ETH_DATA_LEN)
-		rtl_tx_performance_tweak(tp, PCI_EXP_DEVCTL_READRQ_4096B);
-
 	rtl_disable_clock_request(tp);
 
 	/* Reset tx FIFO pointer */
@@ -4636,9 +4627,6 @@ static void rtl_hw_start_8168e_2(struct rtl8169_private *tp)
 
 	rtl_ephy_init(tp, e_info_8168e_2);
 
-	if (tp->dev->mtu <= ETH_DATA_LEN)
-		rtl_tx_performance_tweak(tp, PCI_EXP_DEVCTL_READRQ_4096B);
-
 	rtl_eri_write(tp, 0xc0, ERIAR_MASK_0011, 0x0000);
 	rtl_eri_write(tp, 0xb8, ERIAR_MASK_0011, 0x0000);
 	rtl_set_fifo_size(tp, 0x10, 0x10, 0x02, 0x06);
@@ -5485,6 +5473,8 @@ static void rtl_hw_start(struct  rtl8169_private *tp)
 	rtl_set_rx_tx_desc_registers(tp);
 	rtl_lock_config_regs(tp);
 
+	rtl_jumbo_config(tp, tp->dev->mtu);
+
 	/* Initially a 10 us delay. Turned it into a PCI commit. - FR */
 	RTL_R16(tp, CPlusCmd);
 	RTL_W8(tp, ChipCmd, CmdTxEnb | CmdRxEnb);
@@ -5498,10 +5488,7 @@ static int rtl8169_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 
-	if (new_mtu > ETH_DATA_LEN)
-		rtl_hw_jumbo_enable(tp);
-	else
-		rtl_hw_jumbo_disable(tp);
+	rtl_jumbo_config(tp, new_mtu);
 
 	dev->mtu = new_mtu;
 	netdev_update_features(dev);
-- 
2.23.0

Re: [PATCH net] r8169: fix jumbo packet handling on resume from suspend

[Jakub Kicinski]

On Wed, 9 Oct 2019 20:55:48 +0200, Heiner Kallweit wrote:
> Mariusz reported that invalid packets are sent after resume from
> suspend if jumbo packets are active. It turned out that his BIOS
> resets chip settings to non-jumbo on resume. Most chip settings are
> re-initialized on resume from suspend by calling rtl_hw_start(),
> so let's add configuring jumbo to this function.
> There's nothing wrong with the commit marked as fixed, it's just
> the first one where the patch applies cleanly.
> 
> Fixes: 7366016d2d4c ("r8169: read common register for PCI commit")
> Reported-by: Mariusz Bialonczyk <manio@skyboo.net>
> Tested-by: Mariusz Bialonczyk <manio@skyboo.net>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied, somewhat begrudgingly - this really isn't the way the Fixes
tag should be used, but I appreciate it may be hard at this point to
pin down a commit to blame given how many generations of HW this driver
supports and how old it is.. perhaps I should have removed the tag in
this case, hm.

Since the selected commit came in 5.4 I'm not queuing for stable.

Re: [PATCH net] r8169: fix jumbo packet handling on resume from suspend

[Heiner Kallweit]

On 11.10.2019 01:36, Jakub Kicinski wrote:
> On Wed, 9 Oct 2019 20:55:48 +0200, Heiner Kallweit wrote:
>> Mariusz reported that invalid packets are sent after resume from
>> suspend if jumbo packets are active. It turned out that his BIOS
>> resets chip settings to non-jumbo on resume. Most chip settings are
>> re-initialized on resume from suspend by calling rtl_hw_start(),
>> so let's add configuring jumbo to this function.
>> There's nothing wrong with the commit marked as fixed, it's just
>> the first one where the patch applies cleanly.
>>
>> Fixes: 7366016d2d4c ("r8169: read common register for PCI commit")
>> Reported-by: Mariusz Bialonczyk <manio@skyboo.net>
>> Tested-by: Mariusz Bialonczyk <manio@skyboo.net>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Applied, somewhat begrudgingly - this really isn't the way the Fixes
> tag should be used, but I appreciate it may be hard at this point to
> pin down a commit to blame given how many generations of HW this driver
> supports and how old it is.. perhaps I should have removed the tag in
> this case, hm.
> 
> Since the selected commit came in 5.4 I'm not queuing for stable.
> 
The issue seems to have been there forever, but patch applies from a
certain kernel version only. I agree that using the Fixes tag to provide
this information is kind of a misuse. How would you prefer to get that
information, add a comment below the commit message similar to the list
of changes in a new version of a patch series?

Heiner

Re: [PATCH net] r8169: fix jumbo packet handling on resume from suspend

[Jakub Kicinski]

On Fri, 11 Oct 2019 08:03:24 +0200, Heiner Kallweit wrote:
> On 11.10.2019 01:36, Jakub Kicinski wrote:
> > On Wed, 9 Oct 2019 20:55:48 +0200, Heiner Kallweit wrote:  
> >> Mariusz reported that invalid packets are sent after resume from
> >> suspend if jumbo packets are active. It turned out that his BIOS
> >> resets chip settings to non-jumbo on resume. Most chip settings are
> >> re-initialized on resume from suspend by calling rtl_hw_start(),
> >> so let's add configuring jumbo to this function.
> >> There's nothing wrong with the commit marked as fixed, it's just
> >> the first one where the patch applies cleanly.
> >>
> >> Fixes: 7366016d2d4c ("r8169: read common register for PCI commit")
> >> Reported-by: Mariusz Bialonczyk <manio@skyboo.net>
> >> Tested-by: Mariusz Bialonczyk <manio@skyboo.net>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>  
> > 
> > Applied, somewhat begrudgingly - this really isn't the way the Fixes
> > tag should be used, but I appreciate it may be hard at this point to
> > pin down a commit to blame given how many generations of HW this driver
> > supports and how old it is.. perhaps I should have removed the tag in
> > this case, hm.
> > 
> > Since the selected commit came in 5.4 I'm not queuing for stable.
> >   
> The issue seems to have been there forever, but patch applies from a
> certain kernel version only. I agree that using the Fixes tag to provide
> this information is kind of a misuse. How would you prefer to get that
> information, add a comment below the commit message similar to the list
> of changes in a new version of a patch series?

I'd put the backport help under the --- lines, maybe additionally
mentioning its presence in the commit message (lore link would
complete the picture). Like we do for merges. 

Although I think Dave queues for stable immediately when patch is 
merged to net, so if the backport is to last release or two I think 
the info under --- could be as useful as in the commit message.

Another way would be posting the backported patch (say for the most
recent LTS) if the backport is hard and fix important 🤔