| Message ID | 20231127175736.5738-2-hau@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | r8169: fix PAUSE frames blasting issue | expand |
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;
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; >
> > 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;
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;
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(-)