Message-ID: <1f8c55ea-4995-4879-9505-8f80fcf67e67@gmail.com>
Date: Wed, 11 Sep 2024 11:14:29 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: En-Wei WU <en-wei.wu@...onical.com>
Cc: nic_swsd@...ltek.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, kuan-ying.lee@...onical.com,
 kai.heng.feng@...onical.com, me@...y.org
Subject: Re: [PATCH net] r8169: correct the reset timing of RTL8125 for
 link-change event

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);
>     ....
> }
> 
Thanks for re-testing. I don't think it's something on the MAC side.
For the MAC it should make no difference whether the reset is done
during link-down or after link-up. Therefore I believe it's something
on the PHY side.
Also wrt ALDPS: Do you have the firmware for the NIC loaded?

> On Wed, 11 Sept 2024 at 01:06, Heiner Kallweit <hkallweit1@...il.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@...il.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@...onical.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@...y.org>, could you kindly test if
>>> this new patch works on your environment? Thanks!
>>>
>>> En-Wei,
>>> Best regards.
>>

Powered by blists - more mailing lists