Re: [PATCH net] r8169: correct the reset timing of RTL8125 for link-change event

From: En-Wei WU
Date: Wed Sep 11 2024 - 10:07:52 EST

Next message: Mario Limonciello: "Re: [PATCH] PCI/PM: Put devices to low power state on shutdown"
Previous message: Bjorn Helgaas: "Re: [PATCH v8 11/11] PCI: imx6: Add i.MX8Q PCIe root complex (RC) support"
In reply to: Heiner Kallweit: "Re: [PATCH net] r8169: correct the reset timing of RTL8125 for link-change event"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Thanks for your great support!

On Wed, 11 Sept 2024 at 19:58, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
>
> On 11.09.2024 12:38, En-Wei WU wrote:
> >> Also wrt ALDPS: Do you have the firmware for the NIC loaded?
> > The firmware is rtl8125b-2_0.0.2 07/13/20
> >
> Thanks. Question was because I found an older statement from Realtek
> stating that ALDPS requires firmware to work correctly.
>
> >> Just to be sure. Can you test with the following?
> > Your patch works for our machine. Seems like the root cause is indeed the ALDPS.
> >
> Great! Not sure what's going on, maybe a silicon bug. ALDPS may e.g. stop some
> clock and hw misses to re-enable it on link-up. Then I will submit the change
> to disable ALDPS. Later we maybe can remove the reset on link-down.
>
> Not having ALDPS shouldn't be too much of an issue. Runtime PM (if enabled)
> will put the NIC to D3hot after 10s anyway.
>
> > On Wed, 11 Sept 2024 at 17:16, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
> >>
> >> On 11.09.2024 09:01, En-Wei WU wrote:
> >>>> What is the link partner in your case?
> >>> My link partner is FS S3900-48T4S switch.
> >>>
> >>>> If you put a simple switch in between, does this help?
> >>> I just put a simple D-link switch in between with the original kernel,
> >>> the issue remains (re-plugging it after 3 seconds).
> >>>
> >>>> It makes more the impression that after 3s of link-down the chip (PHY?)
> >>>> transitions to a mode where it doesn't wake up after re-plugging the cable.
> >>> I've done a ftrace on the r8169.ko and the phy driver (realtek.ko),
> >>> and I found that the phy did wake up:
> >>>
> >>> kworker/u40:4-267 [003] 297.026314: funcgraph_entry:
> >>> | phy_link_change() {
> >>> 3533 kworker/u40:4-267 [003] 297.026315: funcgraph_entry:
> >>> 6.704 us | netif_carrier_on();
> >>> 3534 kworker/u40:4-267 [003] 297.026322: funcgraph_entry:
> >>> | r8169_phylink_handler() {
> >>> 3535 kworker/u40:4-267 [003] 297.026322: funcgraph_entry:
> >>> 0.257 us | rtl_link_chg_patch();
> >>> 3536 kworker/u40:4-267 [003] 297.026324: funcgraph_entry:
> >>> 4.026 us | netif_tx_wake_queue();
> >>> 3537 kworker/u40:4-267 [003] 297.026328: funcgraph_entry:
> >>> | phy_print_status() {
> >>> 3538 kworker/u40:4-267 [003] 297.026329: funcgraph_entry:
> >>> 0.245 us | phy_duplex_to_str();
> >>> 3539 kworker/u40:4-267 [003] 297.026329: funcgraph_entry:
> >>> 0.240 us | phy_speed_to_str();
> >>> 3540 kworker/u40:4-267 [003] 297.026329: funcgraph_entry:
> >>> + 12.798 us | netdev_info();
> >>> 3541 kworker/u40:4-267 [003] 297.026343: funcgraph_exit:
> >>> + 14.385 us | }
> >>> 3542 kworker/u40:4-267 [003] 297.026343: funcgraph_exit:
> >>> + 21.217 us | }
> >>> 3543 kworker/u40:4-267 [003] 297.026343: funcgraph_exit:
> >>> + 28.785 us | }
> >>>
> >>> So I doubt that the issue isn't necessarily related to the ALDPS,
> >>> because the PHY seems to have woken up.
> >>>
> >>> After looking at the reset function (plus the TX queue issue
> >>> previously reported by the user) , I'm wondering if the problem is
> >>> related to DMA:
> >>> static void rtl_reset_work(struct rtl8169_private *tp) {
> >>> ....
> >>> for (i = 0; i < NUM_RX_DESC; i++)
> >>> rtl8169_mark_to_asic(tp->RxDescArray + i);
> >>> ....
> >>> }
> >>>
> >>> On Wed, 11 Sept 2024 at 01:06, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
> >>>>
> >>>> On 09.09.2024 07:25, En-Wei WU wrote:
> >>>>> Hi Heiner,
> >>>>>
> >>>>> Thank you for the quick response.
> >>>>>
> >>>>> On Sat, 7 Sept 2024 at 05:17, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
> >>>>>>
> >>>>>> On 06.09.2024 10:35, En-Wei Wu wrote:
> >>>>>>> The commit 621735f59064 ("r8169: fix rare issue with broken rx after
> >>>>>>> link-down on RTL8125") set a reset work for RTL8125 in
> >>>>>>> r8169_phylink_handler() to avoid the MAC from locking up, this
> >>>>>>> makes the connection broken after unplugging then re-plugging the
> >>>>>>> Ethernet cable.
> >>>>>>>
> >>>>>>> This is because the commit mistakenly put the reset work in the
> >>>>>>> link-down path rather than the link-up path (The commit message says
> >>>>>>> it should be put in the link-up path).
> >>>>>>>
> >>>>>> That's not what the commit message is saying. It says vendor driver
> >>>>>> r8125 does it in the link-up path.
> >>>>>> I moved it intentionally to the link-down path, because traffic may
> >>>>>> be flowing already after link-up.
> >>>>>>
> >>>>>>> Moving the reset work from the link-down path to the link-up path fixes
> >>>>>>> the issue. Also, remove the unnecessary enum member.
> >>>>>>>
> >>>>>> The user who reported the issue at that time confirmed that the original
> >>>>>> change fixed the issue for him.
> >>>>>> Can you explain, from the NICs perspective, what exactly the difference
> >>>>>> is when doing the reset after link-up?
> >>>>>> Including an explanation how the original change suppresses the link-up
> >>>>>> interrupt. And why that's not the case when doing the reset after link-up.
> >>>>>
> >>>>> The host-plug test under original change does have the link-up
> >>>>> interrupt and r8169_phylink_handler() called. There is not much clue
> >>>>> why calling reset in link-down path doesn't work but in link-up does.
> >>>>>
> >>>>> After several new tests, I found that with the original change, the
> >>>>> link won't break if I unplug and then plug the cable within about 3
> >>>>> seconds. On the other hand, the connections always break if I re-plug
> >>>>> the cable after a few seconds.
> >>>>>
> >>>> Interesting finding. 3 seconds sounds like it's unrelated to runtime pm,
> >>>> because this has a 10s delay before the chip is transitioned to D3hot.
> >>>> It makes more the impression that after 3s of link-down the chip (PHY?)
> >>>> transitions to a mode where it doesn't wake up after re-plugging the cable.
> >>>>
> >>>> Just a wild guess: It may be some feature like ALDPS (advanced link-down
> >>>> power saving). Depending on the link partner this may result in not waking
> >>>> up again, namely if the link partner uses ALDPS too.
> >>>> What is the link partner in your case? If you put a simple switch in between,
> >>>> does this help?
> >>>>
> >>>> In the RTL8211F datasheet I found the following:
> >>>>
> >>>> Link Down Power Saving Mode.
> >>>> 1: Reflects local device entered Link Down Power Saving Mode,
> >>>> i.e., cable not plugged in (reflected after 3 sec)
> >>>> 0: With cable plugged in
> >>>>
> >>>> This is a 1Gbps PHY, but Realtek may use the same ALDPS mechanism with the
> >>>> integrated PHY of RTL8125. The 3s delay described there perfectly matches
> >>>> your finding.
> >>>>
> >>>>> With this new patch (reset in link-up path), both of the tests work
> >>>>> without any error.
> >>>>>
> >>>>>>
> >>>>>> I simply want to be convinced enough that your change doesn't break
> >>>>>> behavior for other users.
> >>>>>>
> >>>>>>> Fixes: 621735f59064 ("r8169: fix rare issue with broken rx after link-down on RTL8125")
> >>>>>>> Signed-off-by: En-Wei Wu <en-wei.wu@xxxxxxxxxxxxx>
> >>>>>>> ---
> >>>>>>> drivers/net/ethernet/realtek/r8169_main.c | 11 +++++------
> >>>>>>> 1 file changed, 5 insertions(+), 6 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> >>>>>>> index 3507c2e28110..632e661fc74b 100644
> >>>>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
> >>>>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >>>>>>> @@ -590,7 +590,6 @@ struct rtl8169_tc_offsets {
> >>>>>>> enum rtl_flag {
> >>>>>>> RTL_FLAG_TASK_ENABLED = 0,
> >>>>>>> RTL_FLAG_TASK_RESET_PENDING,
> >>>>>>> - RTL_FLAG_TASK_RESET_NO_QUEUE_WAKE,
> >>>>>>> RTL_FLAG_TASK_TX_TIMEOUT,
> >>>>>>> RTL_FLAG_MAX
> >>>>>>> };
> >>>>>>> @@ -4698,8 +4697,6 @@ static void rtl_task(struct work_struct *work)
> >>>>>>> reset:
> >>>>>>> rtl_reset_work(tp);
> >>>>>>> netif_wake_queue(tp->dev);
> >>>>>>> - } else if (test_and_clear_bit(RTL_FLAG_TASK_RESET_NO_QUEUE_WAKE, tp->wk.flags)) {
> >>>>>>> - rtl_reset_work(tp);
> >>>>>>> }
> >>>>>>> out_unlock:
> >>>>>>> rtnl_unlock();
> >>>>>>> @@ -4729,11 +4726,13 @@ static void r8169_phylink_handler(struct net_device *ndev)
> >>>>>>> if (netif_carrier_ok(ndev)) {
> >>>>>>> rtl_link_chg_patch(tp);
> >>>>>>> pm_request_resume(d);
> >>>>>>> - netif_wake_queue(tp->dev);
> >>>>>>> - } else {
> >>>>>>> +
> >>>>>>> /* In few cases rx is broken after link-down otherwise */
> >>>>>>> if (rtl_is_8125(tp))
> >>>>>>> - rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_NO_QUEUE_WAKE);
> >>>>>>> + rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
> >>>>>>> + else
> >>>>>>> + netif_wake_queue(tp->dev);
> >>>>>>
> >>>>>> This call to netif_wake_queue() isn't needed any longer, it was introduced with
> >>>>>> the original change only.
> >>>>>>
> >>>>>>> + } else {
> >>>>>>> pm_runtime_idle(d);
> >>>>>>> }
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>> CC. Martin Kjær Jørgensen <me@xxxxxxxx>, could you kindly test if
> >>>>> this new patch works on your environment? Thanks!
> >>>>>
> >>>>> En-Wei,
> >>>>> Best regards.
> >>>>
> >>
> >> Just to be sure. Can you test with the following?
> >>
> >>
> >> diff --git a/drivers/net/ethernet/realtek/r8169_phy_config.c b/drivers/net/ethernet/realtek/r8169_phy_config.c
> >> index 2c8845e08..cf29b1208 100644
> >> --- a/drivers/net/ethernet/realtek/r8169_phy_config.c
> >> +++ b/drivers/net/ethernet/realtek/r8169_phy_config.c
> >> @@ -1060,6 +1060,7 @@ static void rtl8125a_2_hw_phy_config(struct rtl8169_private *tp,
> >> phy_modify_paged(phydev, 0xa86, 0x15, 0x0001, 0x0000);
> >> rtl8168g_enable_gphy_10m(phydev);
> >>
> >> + rtl8168g_disable_aldps(phydev);
> >> rtl8125a_config_eee_phy(phydev);
> >> }
> >>
> >> @@ -1099,6 +1100,7 @@ static void rtl8125b_hw_phy_config(struct rtl8169_private *tp,
> >> phy_modify_paged(phydev, 0xbf8, 0x12, 0xe000, 0xa000);
> >>
> >> rtl8125_legacy_force_mode(phydev);
> >> + rtl8168g_disable_aldps(phydev);
> >> rtl8125b_config_eee_phy(phydev);
> >> }
> >>
> >> --
> >> 2.46.0
> >>
> >>
>

Next message: Mario Limonciello: "Re: [PATCH] PCI/PM: Put devices to low power state on shutdown"
Previous message: Bjorn Helgaas: "Re: [PATCH v8 11/11] PCI: imx6: Add i.MX8Q PCIe root complex (RC) support"
In reply to: Heiner Kallweit: "Re: [PATCH net] r8169: correct the reset timing of RTL8125 for link-change event"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]