| Message ID | 20210603042057.231226-2-koba.ko@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | UBUNTU: SAUCE: Fix Ethernet not working by hotplug - RTL8106E | expand |
Hi Koba, On Thu, Jun 3, 2021 at 12:21 PM Koba Ko <koba.ko@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. What happens if we disable ASPM before putting the device to D3? > > 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@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@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Thu, Jun 3, 2021 at 12:53 PM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > Hi Koba, > > On Thu, Jun 3, 2021 at 12:21 PM Koba Ko <koba.ko@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@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@lists.ubuntu.com > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 03/06/2021 06:20, Koba Ko 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. > > 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@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[] = { This looks unused - what piece references it? Best regards, Krzysztof > + { 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); > +} > + > +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); > +} > + > +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; > } >
On Mon, Jun 7, 2021 at 5:06 PM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 03/06/2021 06:20, Koba Ko 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. > > > > 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@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[] = { > > This looks unused - what piece references it? > > Best regards, > Krzysztof you're right, it is unused. This patch is nacked and I will send v2, using PHY_POLL. Thanks Koba > > > + { 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); > > +} > > + > > +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); > > +} > > + > > +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; > > } > >
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); +} + +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); +} + +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; }
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. 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@canonical.com> --- drivers/net/ethernet/realtek/r8169.h | 2 + drivers/net/ethernet/realtek/r8169_main.c | 112 ++++++++++++++++++---- 2 files changed, 98 insertions(+), 16 deletions(-)