Re: [PATCH] r8169: introduce polling method for link change

[Heiner Kallweit <hkallweit1@gmail.com>]

On 04.06.2021 09:22, Koba Ko wrote:
> On Thu, Jun 3, 2021 at 6:00 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 03.06.2021 04:54, Koba Ko wrote:
>>> For RTL8106E, it's a Fast-ethernet chip.
>>> If ASPM is enabled, the link chang interrupt wouldn't be triggered
>>> immediately and must wait a very long time to get link change interrupt.
>>> Even the link change interrupt isn't triggered, the phy link is already
>>> established.
>>>
>> At first please provide a full dmesg log and output of lspci -vv.
>> Do you have the firmware for the NIC loaded? Please provide "ethtool -i <if>"
>> output.
> 
> please get the logs from here,
> https://bugzilla.kernel.org/show_bug.cgi?id=213165
> 
>> Does the issue affect link-down and/or link-up detection?
>> Do you have runtime pm enabled? Then, after 10s of link-down NIC goes to
>> D3hot and link-up detection triggers a PME.
> 
> Issue affect link-up.
> yes, pm runtime is enabled, but rtl8106e always stays D0 even if the
> cable isn't present.
> 
Then runtime pm doesn't seem to be set to "auto". Else 10s after link loss
the chip runtime-suspends and is set to D3hot.

>>
>>> Introduce a polling method to watch the status of phy link and disable
>>> the link change interrupt.
>>> Also add a quirk for those realtek devices have the same issue.
>>>
>> Which are the affected chip versions? Did you check with Realtek?
>> Your patch switches to polling for all Fast Ethernet versions,
>> and that's not what we want.
> 
> I don't know the exact version, only the chip name 806e(pci device id 0x8165).
> ok, Im asking Realtek to help how to identify the chip issue is observed.
> 
At least your Bugzilla report refers to VER_39. PCI device id 0x8136 is shared
by all fast ethernet chip versions.
Do you know other affected chip versions apart from VER_39 ?

In the Bugzilla report you also write the issue occurs with GBit-capable
link partners. This sounds more like an aneg problem.
The issue doesn't occur with fast ethernet link partners?

Your bug report also includes a patch that disables L1_1 only.
Not sure how this is related because the chip version we speak about
here doesn't support L1 sub-states.

>>
>> My suspicion would be that something is system-dependent. Else I think
>> we would have seen such a report before.
> On the mainline, the aspm is disable, so you may not observe this.
> If you enable ASPM and must wait CHIP go to power-saving mode, then
> you can observe the issue.
>>

So what you're saying is that mainline is fine and your problem is with
a downstream kernel with re-enabled ASPM? So there's nothing broken in
mainline? In mainline you have the option to re-enable ASPM states
individually via sysfs (link subdir at pci device).

>>> Signed-off-by: Koba Ko <koba.ko@canonical.com>
>>> ---
>>>  drivers/net/ethernet/realtek/r8169.h      |   2 +
>>>  drivers/net/ethernet/realtek/r8169_main.c | 112 ++++++++++++++++++----
>>>  2 files changed, 98 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169.h b/drivers/net/ethernet/realtek/r8169.h
>>> index 2728df46ec41..a8c71adb1b57 100644
>>> --- a/drivers/net/ethernet/realtek/r8169.h
>>> +++ b/drivers/net/ethernet/realtek/r8169.h
>>> @@ -11,6 +11,8 @@
>>>  #include <linux/types.h>
>>>  #include <linux/phy.h>
>>>
>>> +#define RTL8169_LINK_TIMEOUT (1 * HZ)
>>> +
>>>  enum mac_version {
>>>       /* support for ancient RTL_GIGA_MAC_VER_01 has been removed */
>>>       RTL_GIGA_MAC_VER_02,
>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>>> index 2c89cde7da1e..70aacc83d641 100644
>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>> @@ -178,6 +178,11 @@ static const struct pci_device_id rtl8169_pci_tbl[] = {
>>>
>>>  MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
>>>
>>> +static const struct pci_device_id rtl8169_linkChg_polling_enabled[] = {
>>> +     { PCI_VDEVICE(REALTEK, 0x8136), RTL_CFG_NO_GBIT },
>>> +     { 0 }
>>> +};
>>> +
>>
>> This doesn't seem to be used.
>>
>>>  enum rtl_registers {
>>>       MAC0            = 0,    /* Ethernet hardware address. */
>>>       MAC4            = 4,
>>> @@ -618,6 +623,7 @@ struct rtl8169_private {
>>>       u16 cp_cmd;
>>>       u32 irq_mask;
>>>       struct clk *clk;
>>> +     struct timer_list link_timer;
>>>
>>>       struct {
>>>               DECLARE_BITMAP(flags, RTL_FLAG_MAX);
>>> @@ -1179,6 +1185,16 @@ static void rtl8168ep_stop_cmac(struct rtl8169_private *tp)
>>>       RTL_W8(tp, IBCR0, RTL_R8(tp, IBCR0) & ~0x01);
>>>  }
>>>
>>> +static int rtl_link_chng_polling_quirk(struct rtl8169_private *tp)
>>> +{
>>> +     struct pci_dev *pdev = tp->pci_dev;
>>> +
>>> +     if (pdev->vendor == 0x10ec && pdev->device == 0x8136 && !tp->supports_gmii)
>>> +             return 1;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>  static void rtl8168dp_driver_start(struct rtl8169_private *tp)
>>>  {
>>>       r8168dp_oob_notify(tp, OOB_CMD_DRIVER_START);
>>> @@ -4608,6 +4624,75 @@ static void rtl_task(struct work_struct *work)
>>>       rtnl_unlock();
>>>  }
>>>
>>> +static void r8169_phylink_handler(struct net_device *ndev)
>>> +{
>>> +     struct rtl8169_private *tp = netdev_priv(ndev);
>>> +
>>> +     if (netif_carrier_ok(ndev)) {
>>> +             rtl_link_chg_patch(tp);
>>> +             pm_request_resume(&tp->pci_dev->dev);
>>> +     } else {
>>> +             pm_runtime_idle(&tp->pci_dev->dev);
>>> +     }
>>> +
>>> +     if (net_ratelimit())
>>> +             phy_print_status(tp->phydev);
>>> +}
>>> +
>>> +static unsigned int
>>> +rtl8169_xmii_link_ok(struct net_device *dev)
>>> +{
>>> +     struct rtl8169_private *tp = netdev_priv(dev);
>>> +     unsigned int retval;
>>> +
>>> +     retval = (RTL_R8(tp, PHYstatus) & LinkStatus) ? 1 : 0;
>>> +
>>> +     return retval;
>>> +}
>>> +
>>> +static void
>>> +rtl8169_check_link_status(struct net_device *dev)
>>> +{
>>> +     struct rtl8169_private *tp = netdev_priv(dev);
>>> +     int link_status_on;
>>> +
>>> +     link_status_on = rtl8169_xmii_link_ok(dev);
>>> +
>>> +     if (netif_carrier_ok(dev) == link_status_on)
>>> +             return;
>>> +
>>> +     phy_mac_interrupt(tp->phydev);
>>> +
>>> +     r8169_phylink_handler (dev);
>>> +}
>>> +
>>> +static void rtl8169_link_timer(struct timer_list *t)
>>> +{
>>> +     struct rtl8169_private *tp = from_timer(tp, t, link_timer);
>>> +     struct net_device *dev = tp->dev;
>>> +     struct timer_list *timer = t;
>>> +     unsigned long flags;
>>
>> flags isn't used and triggers a compiler warning. Did you even
>> compile-test your patch?
>>
>>> +
>>> +     rtl8169_check_link_status(dev);
>>> +
>>> +     if (timer_pending(&tp->link_timer))
>>> +             return;
>>> +
>>> +     mod_timer(timer, jiffies + RTL8169_LINK_TIMEOUT);
>>> +}
>>> +
>>> +static inline void rtl8169_delete_link_timer(struct net_device *dev, struct timer_list *timer)
>>> +{
>>> +     del_timer_sync(timer);
>>> +}
>>> +
>>> +static inline void rtl8169_request_link_timer(struct net_device *dev)
>>> +{
>>> +     struct rtl8169_private *tp = netdev_priv(dev);
>>> +
>>> +     timer_setup(&tp->link_timer, rtl8169_link_timer, TIMER_INIT_FLAGS);
>>> +}
>>> +
>>>  static int rtl8169_poll(struct napi_struct *napi, int budget)
>>>  {
>>>       struct rtl8169_private *tp = container_of(napi, struct rtl8169_private, napi);
>>> @@ -4624,21 +4709,6 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
>>>       return work_done;
>>>  }
>>>
>>> -static void r8169_phylink_handler(struct net_device *ndev)
>>> -{
>>> -     struct rtl8169_private *tp = netdev_priv(ndev);
>>> -
>>> -     if (netif_carrier_ok(ndev)) {
>>> -             rtl_link_chg_patch(tp);
>>> -             pm_request_resume(&tp->pci_dev->dev);
>>> -     } else {
>>> -             pm_runtime_idle(&tp->pci_dev->dev);
>>> -     }
>>> -
>>> -     if (net_ratelimit())
>>> -             phy_print_status(tp->phydev);
>>> -}
>>> -
>>>  static int r8169_phy_connect(struct rtl8169_private *tp)
>>>  {
>>>       struct phy_device *phydev = tp->phydev;
>>> @@ -4769,6 +4839,10 @@ static int rtl_open(struct net_device *dev)
>>>               goto err_free_irq;
>>>
>>>       rtl8169_up(tp);
>>> +
>>> +     if (rtl_link_chng_polling_quirk(tp))
>>> +             mod_timer(&tp->link_timer, jiffies + RTL8169_LINK_TIMEOUT);
>>> +
>>>       rtl8169_init_counter_offsets(tp);
>>>       netif_start_queue(dev);
>>>  out:
>>> @@ -4991,7 +5065,10 @@ static const struct net_device_ops rtl_netdev_ops = {
>>>
>>>  static void rtl_set_irq_mask(struct rtl8169_private *tp)
>>>  {
>>> -     tp->irq_mask = RxOK | RxErr | TxOK | TxErr | LinkChg;
>>> +     tp->irq_mask = RxOK | RxErr | TxOK | TxErr;
>>> +
>>> +     if (!rtl_link_chng_polling_quirk(tp))
>>> +             tp->irq_mask |= LinkChg;
>>>
>>>       if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
>>>               tp->irq_mask |= SYSErr | RxOverflow | RxFIFOOver;
>>> @@ -5436,6 +5513,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>       if (pci_dev_run_wake(pdev))
>>>               pm_runtime_put_sync(&pdev->dev);
>>>
>>> +     if (rtl_link_chng_polling_quirk(tp))
>>> +             rtl8169_request_link_timer(dev);
>>> +
>>>       return 0;
>>>  }
>>>
>>>
>>
>> All this isn't needed. If you want to switch to link status polling,
>> why don't you simply let phylib do it? PHY_MAC_INTERRUPT -> PHY_POLL
> 
> Thanks for suggestions, I tried to use PHY_POLL, it could do the same
> thing that I did.
> 
>> Your timer-based code most likely would have problems if runtime pm
>> is enabled. Then you try to read the link status whilst NIC is in
>> D3hot.