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

[Heiner Kallweit <hkallweit1@gmail.com>]

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@gmail.com> 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@gmail.com> 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@gmail.com> 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@canonical.com>
>>>>>>> ---
>>>>>>>  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@lagy.org>, 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
>>
>>