NACK/Cmnt: [PATCH][SRU][OEM-5.10] UBUNTU: SAUCE: r8169: introduce polling method for link change
Koba Ko
koba.ko at canonical.com
Thu Jun 3 05:33:03 UTC 2021
On Thu, Jun 3, 2021 at 12:53 PM Kai-Heng Feng
<kai.heng.feng at canonical.com> wrote:
>
> Hi Koba,
>
> On Thu, Jun 3, 2021 at 12:21 PM Koba Ko <koba.ko at canonical.com> wrote:
> >
> > BugLink: https://bugs.launchpad.net/bugs/1930645
> >
> > 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.
>
> Is PME enabled? If it's enabled, is PME status marked?
> If PME is marked but there's no interrupt for PME, the
> pci_pme_list_scan() should do the work here.
PME-Enable is disabled.
>
> What happens if we disable ASPM before putting the device to D3?
As per this, https://bugs.launchpad.net/ubuntu/+source/linux-oem-osp1/+bug/1836030
if disable ASPM, during suspend, machine can't sleep deeper than PC3.
> >
> > 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.
> >
> > Signed-off-by: Koba Ko <koba.ko at 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 }
> > +};
> > +
> > 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);
> > +}
>
> rtl8169_xmii_link_ok() and rtl8169_check_link_status() were purposely
> replaced by realtek phylib. See commit "r8169: add basic phylib
> support".
>
> If this is really needed we should put it to realtek phylib instead.
>
> > +
> > +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;
> > +
> > + 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);
> > +}
>
> The function doesn't have any user. Maybe it should be called in
> rtl8169_close()?
>
> > +
> > +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;
>
> Maybe
> if (rtl_link_chng_polling_quirk(tp))
> tp->irq_mask &= ~LinkChg;
>
> Kai-Heng
>
> >
> > 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;
> > }
> >
> > --
> > 2.25.1
> >
> >
> > --
> > kernel-team mailing list
> > kernel-team at lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list