* [PATCH net 0/2] r8169: fix PAUSE frames blasting issue @ 2023-11-27 17:57 ChunHao Lin 2023-11-27 17:57 ` [PATCH net 1/2] r8169: enable rtl8125b pause slot ChunHao Lin 2023-11-27 17:57 ` [PATCH net 2/2] r8169: fix deadlock in "r8169_phylink_handler" ChunHao Lin 0 siblings, 2 replies; 7+ messages in thread From: ChunHao Lin @ 2023-11-27 17:57 UTC (permalink / raw) To: hkallweit1 Cc: nic_swsd, davem, edumazet, kuba, pabeni, netdev, linux-kernel, grundler, 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net 1/2] r8169: enable rtl8125b pause slot 2023-11-27 17:57 [PATCH net 0/2] r8169: fix PAUSE frames blasting issue ChunHao Lin @ 2023-11-27 17:57 ` ChunHao Lin 2023-11-27 20:03 ` Heiner Kallweit 2023-11-27 17:57 ` [PATCH net 2/2] r8169: fix deadlock in "r8169_phylink_handler" ChunHao Lin 1 sibling, 1 reply; 7+ messages in thread From: ChunHao Lin @ 2023-11-27 17:57 UTC (permalink / raw) To: hkallweit1 Cc: nic_swsd, davem, edumazet, kuba, pabeni, netdev, linux-kernel, grundler, ChunHao Lin, stable 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] r8169: enable rtl8125b pause slot 2023-11-27 17:57 ` [PATCH net 1/2] r8169: enable rtl8125b pause slot ChunHao Lin @ 2023-11-27 20:03 ` Heiner Kallweit 2023-11-27 20:28 ` Grant Grundler 2023-11-29 15:08 ` Hau 0 siblings, 2 replies; 7+ messages in thread From: Heiner Kallweit @ 2023-11-27 20:03 UTC (permalink / raw) To: ChunHao Lin Cc: nic_swsd, davem, edumazet, kuba, pabeni, netdev, linux-kernel, grundler, stable 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] r8169: enable rtl8125b pause slot 2023-11-27 20:03 ` Heiner Kallweit @ 2023-11-27 20:28 ` Grant Grundler 2023-11-29 15:08 ` Hau 1 sibling, 0 replies; 7+ messages in thread From: Grant Grundler @ 2023-11-27 20:28 UTC (permalink / raw) To: Heiner Kallweit Cc: ChunHao Lin, nic_swsd, davem, edumazet, kuba, pabeni, netdev, linux-kernel, grundler, stable 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; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH net 1/2] r8169: enable rtl8125b pause slot 2023-11-27 20:03 ` Heiner Kallweit 2023-11-27 20:28 ` Grant Grundler @ 2023-11-29 15:08 ` Hau 1 sibling, 0 replies; 7+ messages in thread From: Hau @ 2023-11-29 15:08 UTC (permalink / raw) To: Heiner Kallweit Cc: nic_swsd, davem, edumazet, kuba, pabeni, netdev, linux-kernel, grundler, stable > > 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net 2/2] r8169: fix deadlock in "r8169_phylink_handler" 2023-11-27 17:57 [PATCH net 0/2] r8169: fix PAUSE frames blasting issue ChunHao Lin 2023-11-27 17:57 ` [PATCH net 1/2] r8169: enable rtl8125b pause slot ChunHao Lin @ 2023-11-27 17:57 ` ChunHao Lin 2023-11-27 19:37 ` Heiner Kallweit 1 sibling, 1 reply; 7+ messages in thread From: ChunHao Lin @ 2023-11-27 17:57 UTC (permalink / raw) To: hkallweit1 Cc: nic_swsd, davem, edumazet, kuba, pabeni, netdev, linux-kernel, grundler, ChunHao Lin, stable 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 2/2] r8169: fix deadlock in "r8169_phylink_handler" 2023-11-27 17:57 ` [PATCH net 2/2] r8169: fix deadlock in "r8169_phylink_handler" ChunHao Lin @ 2023-11-27 19:37 ` Heiner Kallweit 0 siblings, 0 replies; 7+ messages in thread From: Heiner Kallweit @ 2023-11-27 19:37 UTC (permalink / raw) To: ChunHao Lin Cc: nic_swsd, davem, edumazet, kuba, pabeni, netdev, linux-kernel, grundler, stable 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-29 15:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-27 17:57 [PATCH net 0/2] r8169: fix PAUSE frames blasting issue ChunHao Lin 2023-11-27 17:57 ` [PATCH net 1/2] r8169: enable rtl8125b pause slot ChunHao Lin 2023-11-27 20:03 ` Heiner Kallweit 2023-11-27 20:28 ` Grant Grundler 2023-11-29 15:08 ` Hau 2023-11-27 17:57 ` [PATCH net 2/2] r8169: fix deadlock in "r8169_phylink_handler" ChunHao Lin 2023-11-27 19:37 ` Heiner Kallweit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®