[PATCH net 0/2] r8169: fix PAUSE frames blasting issue

[PATCH net 0/2] r8169: fix PAUSE frames blasting issue

[ChunHao Lin]

This series of patches are used to fix PAUSE frames blasting issue.

ChunHao Lin (2):
  r8169: enable rtl8125b pause slot
  r8169: fix deadlock in "r8169_phylink_handler"

 drivers/net/ethernet/realtek/r8169_main.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH net 1/2] r8169: enable rtl8125b pause slot

[ChunHao Lin]

When FIFO reach near full state, device will issue pause frame.
If pause slot is enabled(set to 1), in this time, device will issue
pause frame once. But if pause slot is disabled(set to 0), device
will keep sending pause frames until FIFO reach near empty state.

When pause slot is disabled, if there is no one to handle receive
packets (ex. unexpected shutdown), device FIFO will reach near full
state and keep sending pause frames. That will impact entire local
area network.

In this patch default enable pause slot to prevent this kind of
situation.

Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
Cc: stable@vger.kernel.org
Signed-off-by: ChunHao Lin <hau@realtek.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 295366a85c63..473b3245754f 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -196,6 +196,7 @@ enum rtl_registers {
 					/* No threshold before first PCI xfer */
 #define	RX_FIFO_THRESH			(7 << RXCFG_FIFO_SHIFT)
 #define	RX_EARLY_OFF			(1 << 11)
+#define	RX_PAUSE_SLOT_ON		(1 << 11)
 #define	RXCFG_DMA_SHIFT			8
 					/* Unlimited maximum PCI burst. */
 #define	RX_DMA_BURST			(7 << RXCFG_DMA_SHIFT)
@@ -2305,9 +2306,13 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
 	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
 		RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST | RX_EARLY_OFF);
 		break;
-	case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_63:
+	case RTL_GIGA_MAC_VER_61:
 		RTL_W32(tp, RxConfig, RX_FETCH_DFLT_8125 | RX_DMA_BURST);
 		break;
+	case RTL_GIGA_MAC_VER_63:
+		RTL_W32(tp, RxConfig, RX_FETCH_DFLT_8125 | RX_DMA_BURST |
+			RX_PAUSE_SLOT_ON);
+		break;
 	default:
 		RTL_W32(tp, RxConfig, RX128_INT_EN | RX_DMA_BURST);
 		break;
-- 
2.39.2

[PATCH net 2/2] r8169: fix deadlock in "r8169_phylink_handler"

[ChunHao Lin]

In "r8169_phylink_handler", for rtl8125, it will call "rtl_reset_work"->
"rtl_hw_start"->"rtl_jumbo_config"->"phy_start_aneg". When call
"r8169_phylink_handler", PHY lock is acquired. But "phy_start_aneg"
will also try to acquire PHY lock. That will cause deadlock.

In this path, use "_phy_start_aneg", unlocked version "phy_start_aneg",
to prevent deadlock in "r8169_phylink_handler".

Fixes: 453a77894efa ("r8169: don't advertise pause in jumbo mode")
Cc: stable@vger.kernel.org
Signed-off-by: ChunHao Lin <hau@realtek.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 473b3245754f..2e3e42a98edd 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2415,11 +2415,22 @@ static void rtl_jumbo_config(struct rtl8169_private *tp)
 
 	/* Chip doesn't support pause in jumbo mode */
 	if (jumbo) {
+		int lock;
+
 		linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
 				   tp->phydev->advertising);
 		linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
 				   tp->phydev->advertising);
-		phy_start_aneg(tp->phydev);
+
+		if (!mutex_trylock(&tp->phydev->lock))
+			lock = 0;
+		else
+			lock = 1;
+
+		_phy_start_aneg(tp->phydev);
+
+		if (lock)
+			mutex_unlock(&tp->phydev->lock);
 	}
 }
 
-- 
2.39.2

Re: [PATCH net 2/2] r8169: fix deadlock in "r8169_phylink_handler"

[Heiner Kallweit]

On 27.11.2023 18:57, ChunHao Lin wrote:
> In "r8169_phylink_handler", for rtl8125, it will call "rtl_reset_work"->
> "rtl_hw_start"->"rtl_jumbo_config"->"phy_start_aneg". When call
> "r8169_phylink_handler", PHY lock is acquired. But "phy_start_aneg"
> will also try to acquire PHY lock. That will cause deadlock.
> 
> In this path, use "_phy_start_aneg", unlocked version "phy_start_aneg",
> to prevent deadlock in "r8169_phylink_handler".
> 
> Fixes: 453a77894efa ("r8169: don't advertise pause in jumbo mode")
> Cc: stable@vger.kernel.org
> Signed-off-by: ChunHao Lin <hau@realtek.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 473b3245754f..2e3e42a98edd 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2415,11 +2415,22 @@ static void rtl_jumbo_config(struct rtl8169_private *tp)
>  
>  	/* Chip doesn't support pause in jumbo mode */
>  	if (jumbo) {
> +		int lock;
> +
>  		linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>  				   tp->phydev->advertising);
>  		linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
>  				   tp->phydev->advertising);
> -		phy_start_aneg(tp->phydev);
> +
> +		if (!mutex_trylock(&tp->phydev->lock))
> +			lock = 0;
> +		else
> +			lock = 1;
> +
> +		_phy_start_aneg(tp->phydev);
> +
> +		if (lock)
> +			mutex_unlock(&tp->phydev->lock);
>  	}
>  }
>  

Hi Hau,
the deadlock issue has been reported by few users, and I submitted a fix already.
It's waiting to be applied.

Re: [PATCH net 1/2] r8169: enable rtl8125b pause slot

[Heiner Kallweit]

On 27.11.2023 18:57, ChunHao Lin wrote:
> When FIFO reach near full state, device will issue pause frame.
> If pause slot is enabled(set to 1), in this time, device will issue
> pause frame once. But if pause slot is disabled(set to 0), device
> will keep sending pause frames until FIFO reach near empty state.
> 
> When pause slot is disabled, if there is no one to handle receive
> packets (ex. unexpected shutdown), device FIFO will reach near full
> state and keep sending pause frames. That will impact entire local
> area network.
> 
> In this patch default enable pause slot to prevent this kind of
> situation.
> 
Can this change have any side effect? I'm asking because apparently
the hw engineers had a reason to make the behavior configurable.

> Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
> Cc: stable@vger.kernel.org
> Signed-off-by: ChunHao Lin <hau@realtek.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 295366a85c63..473b3245754f 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -196,6 +196,7 @@ enum rtl_registers {
>  					/* No threshold before first PCI xfer */
>  #define	RX_FIFO_THRESH			(7 << RXCFG_FIFO_SHIFT)
>  #define	RX_EARLY_OFF			(1 << 11)
> +#define	RX_PAUSE_SLOT_ON		(1 << 11)

Depending on the chip version this bit has different meanings. Therefore it
would be good to add a comment that RX_PAUSE_SLOT_ON is specific to RTL8125B.

>  #define	RXCFG_DMA_SHIFT			8
>  					/* Unlimited maximum PCI burst. */
>  #define	RX_DMA_BURST			(7 << RXCFG_DMA_SHIFT)
> @@ -2305,9 +2306,13 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
>  	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
>  		RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST | RX_EARLY_OFF);
>  		break;
> -	case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_63:
> +	case RTL_GIGA_MAC_VER_61:
>  		RTL_W32(tp, RxConfig, RX_FETCH_DFLT_8125 | RX_DMA_BURST);
>  		break;
> +	case RTL_GIGA_MAC_VER_63:
> +		RTL_W32(tp, RxConfig, RX_FETCH_DFLT_8125 | RX_DMA_BURST |
> +			RX_PAUSE_SLOT_ON);
> +		break;
>  	default:
>  		RTL_W32(tp, RxConfig, RX128_INT_EN | RX_DMA_BURST);
>  		break;

Re: [PATCH net 1/2] r8169: enable rtl8125b pause slot

[Grant Grundler]

On Mon, Nov 27, 2023 at 12:03 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 27.11.2023 18:57, ChunHao Lin wrote:
> > When FIFO reach near full state, device will issue pause frame.
> > If pause slot is enabled(set to 1), in this time, device will issue
> > pause frame once. But if pause slot is disabled(set to 0), device
> > will keep sending pause frames until FIFO reach near empty state.
> >
> > When pause slot is disabled, if there is no one to handle receive
> > packets (ex. unexpected shutdown), device FIFO will reach near full
> > state and keep sending pause frames. That will impact entire local
> > area network.

The comment is correct but should mention that this is true after a
suspend. In other words, when an idle device goes into a lower power
state, eventually the NIC will start blasting PAUSE frames on the
local network.

I was able to reproduce the problem very easily with a recent
Chromebox (not Chromebook) in developer mode running a test image (and
v5.10 kernel):
1) ping -f $CHROMEBOX (from workstation on same local network)
2) run "powerd_dbus_suspend" from command line on the $CHROMEBOX
3) ping $ROUTER (wait until ping fails from workstation)

Takes about ~20-30 seconds after step 2 for the local network to stop working.
At that point, tcpdump from the workstation is full of PAUSE frames.

I did not check that WOL still works.

The exact patches I used on chromeos-5.10 kernel branch are publicly
visible here:
    https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5056381

> > In this patch default enable pause slot to prevent this kind of
> > situation.
> >
> Can this change have any side effect? I'm asking because apparently
> the hw engineers had a reason to make the behavior configurable.
>
> > Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: ChunHao Lin <hau@realtek.com>

Tested-by: Grant Grundler <grundler@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromiuim.org>

(adding my reviewed-by to indicate I think the code is fine... I
appreciate Heiner asking for better comments though.)

cheers,
grant

> > ---
> >  drivers/net/ethernet/realtek/r8169_main.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> > index 295366a85c63..473b3245754f 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -196,6 +196,7 @@ enum rtl_registers {
> >                                       /* No threshold before first PCI xfer */
> >  #define      RX_FIFO_THRESH                  (7 << RXCFG_FIFO_SHIFT)
> >  #define      RX_EARLY_OFF                    (1 << 11)
> > +#define      RX_PAUSE_SLOT_ON                (1 << 11)
>
> Depending on the chip version this bit has different meanings. Therefore it
> would be good to add a comment that RX_PAUSE_SLOT_ON is specific to RTL8125B.
>
> >  #define      RXCFG_DMA_SHIFT                 8
> >                                       /* Unlimited maximum PCI burst. */
> >  #define      RX_DMA_BURST                    (7 << RXCFG_DMA_SHIFT)
> > @@ -2305,9 +2306,13 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
> >       case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> >               RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST | RX_EARLY_OFF);
> >               break;
> > -     case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_63:
> > +     case RTL_GIGA_MAC_VER_61:
> >               RTL_W32(tp, RxConfig, RX_FETCH_DFLT_8125 | RX_DMA_BURST);
> >               break;
> > +     case RTL_GIGA_MAC_VER_63:
> > +             RTL_W32(tp, RxConfig, RX_FETCH_DFLT_8125 | RX_DMA_BURST |
> > +                     RX_PAUSE_SLOT_ON);
> > +             break;
> >       default:
> >               RTL_W32(tp, RxConfig, RX128_INT_EN | RX_DMA_BURST);
> >               break;
>

RE: [PATCH net 1/2] r8169: enable rtl8125b pause slot

[Hau]

> > When FIFO reach near full state, device will issue pause frame.
> > If pause slot is enabled(set to 1), in this time, device will issue
> > pause frame once. But if pause slot is disabled(set to 0), device will
> > keep sending pause frames until FIFO reach near empty state.
> >
> > When pause slot is disabled, if there is no one to handle receive
> > packets (ex. unexpected shutdown), device FIFO will reach near full
> > state and keep sending pause frames. That will impact entire local
> > area network.
> >
> > In this patch default enable pause slot to prevent this kind of
> > situation.
> >
> Can this change have any side effect? I'm asking because apparently the hw
> engineers had a reason to make the behavior configurable.

It should not have any side effect. This setting is also used in Realtek driver.

> > Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: ChunHao Lin <hau@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/r8169_main.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > b/drivers/net/ethernet/realtek/r8169_main.c
> > index 295366a85c63..473b3245754f 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -196,6 +196,7 @@ enum rtl_registers {
> >                                       /* No threshold before first PCI xfer */
> >  #define      RX_FIFO_THRESH                  (7 << RXCFG_FIFO_SHIFT)
> >  #define      RX_EARLY_OFF                    (1 << 11)
> > +#define      RX_PAUSE_SLOT_ON                (1 << 11)
> 
> Depending on the chip version this bit has different meanings. Therefore it
> would be good to add a comment that RX_PAUSE_SLOT_ON is specific to
> RTL8125B.

I will do that and submit again.

> >  #define      RXCFG_DMA_SHIFT                 8
> >                                       /* Unlimited maximum PCI burst. */
> >  #define      RX_DMA_BURST                    (7 << RXCFG_DMA_SHIFT)
> > @@ -2305,9 +2306,13 @@ static void rtl_init_rxcfg(struct rtl8169_private
> *tp)
> >       case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> >               RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN |
> RX_DMA_BURST | RX_EARLY_OFF);
> >               break;
> > -     case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_63:
> > +     case RTL_GIGA_MAC_VER_61:
> >               RTL_W32(tp, RxConfig, RX_FETCH_DFLT_8125 | RX_DMA_BURST);
> >               break;
> > +     case RTL_GIGA_MAC_VER_63:
> > +             RTL_W32(tp, RxConfig, RX_FETCH_DFLT_8125 | RX_DMA_BURST |
> > +                     RX_PAUSE_SLOT_ON);
> > +             break;
> >       default:
> >               RTL_W32(tp, RxConfig, RX128_INT_EN | RX_DMA_BURST);
> >               break;