Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp895373pxj; Fri, 4 Jun 2021 00:27:48 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzpc851FKZ8sXJwRfxJ71IOiRQrotwpjii3poeMmT3iU7Q94F0KSCzTcLgE+chvgRrSCNsd X-Received: by 2002:aa7:cfc7:: with SMTP id r7mr53530edy.13.1622791668491; Fri, 04 Jun 2021 00:27:48 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1622791668; cv=none; d=google.com; s=arc-20160816; b=uOwjzVH1+NN8LNv+wJvWxJ3SYjIO7f5n6aogiwtpKIJQi3merQaVGw2Vb5HpYfPwZJ LO2DJ/K5qufJsW6qIiu61xM/ve9nN0Y0jaZ5nb0j7fZnKRMvCJqrt5cLmWmIyxNGlcP/ I3SJJjLiWzvzc2m3m3Ysdaj/Ohgqq4aqGARjVpO74PYnNFP6cWG9M36CA6xpEKsVwm13 fOR5uzJUy9u1EmX3w/fC1tN9e7mkqmig9kCGboLrQhHK88EjBhr92M7SRqzwBYzcSR6O oL258X5wN+B6Lr4Z1T1lmf89OYKJD9aFIjeQqoxQ/+Lj6yetM/JremWkoQRYa5hMr1ns OvtA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version; bh=q04LbnKPu+/6G+YpEaHfIACjyCXncUzaze9YgvNZXnI=; b=JDkC4c7HAQLiFiNYRfGojTenHMWnWE+tsQtCYTdMZnde269Tvwl/5gYTnvP3SBg7MZ fF35JuTWyjwF8hoj7HIZb8F08/i3YKMM4pwUtSwTDylmDStEH8eFeTKpaJTmkwj0tAQf ZV2Rfvr5E8pHNvcPcYpsEzp2wIvA+fnQ4Fn9wXkOwMEz6UNeQW9cDjoMEK/6WnXNQCCa OR8J/LewsMq/9znLQ5CF3YKjkHQUPWogDd2vEgf5jk0eKw/GpHzCxKQ4XJvd3uhLstH9 3VwQgYbS1cTghsNT2PmiHIwyeecdIMvuvqMx2fnNFRK/SVv27H4hNokO1f+Ai11QM59q 1zdQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=canonical.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id m1si4101281ejj.214.2021.06.04.00.27.25; Fri, 04 Jun 2021 00:27:48 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=canonical.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230022AbhFDHYl (ORCPT + 99 others); Fri, 4 Jun 2021 03:24:41 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:38557 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229886AbhFDHYl (ORCPT ); Fri, 4 Jun 2021 03:24:41 -0400 Received: from mail-oo1-f71.google.com ([209.85.161.71]) by youngberry.canonical.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.93) (envelope-from ) id 1lp4AQ-0007st-83 for linux-kernel@vger.kernel.org; Fri, 04 Jun 2021 07:22:54 +0000 Received: by mail-oo1-f71.google.com with SMTP id e10-20020a4ab14a0000b029020e1573bdb7so4973370ooo.9 for ; Fri, 04 Jun 2021 00:22:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=q04LbnKPu+/6G+YpEaHfIACjyCXncUzaze9YgvNZXnI=; b=lodS0Wg2Sg95TPuW7BfBmKoZJC+EA5o67nhwCeSCwUgzm6tgK7QCFCpB2ZWbCJ7Pn7 LNUz8FbXythJJhJO9nnM+xWYxex7jU9WM18T0XyTPvOh20FbtWtOglWTntN4OyEgAOOj LAd4yhXa3xoHT+RQrnL2nZ+yHMSM8052axu9DgYCCLiMudi0lu2qDWxTrcvZv4ebAivK 5p8LX5g16PJ0q1hWP6pJqVlhLwG6YEFbuY69FjUl4PS2sjUTeLvH52Eu1Rcboj2oXJeR 6Qw1A5lCpbikXrYhGPM2zn35jn9wgdR61IljglMGzWcUGv1tGDmvEb1cj3e5xHnIdxYy KrFw== X-Gm-Message-State: AOAM532JIdI2LF9USMy9Osqp5ARja1S1bWSeFe+nZpyPU27cva3ZQlC1 AiODepkgZ4tk3w580Pjo4q+La5MlZR3yyGU1KHitB4bb4p7ZkxewdUJkcVt+YaQgPJWeDK5XCBg BwN9E7DoekX8ZleuDa7W6X1IGiJyPL1+pUE9u95tEq97z4FOBTb5mZdL/dg== X-Received: by 2002:a9d:6186:: with SMTP id g6mr2593189otk.246.1622791373044; Fri, 04 Jun 2021 00:22:53 -0700 (PDT) X-Received: by 2002:a9d:6186:: with SMTP id g6mr2593168otk.246.1622791372692; Fri, 04 Jun 2021 00:22:52 -0700 (PDT) MIME-Version: 1.0 References: <20210603025414.226526-1-koba.ko@canonical.com> <3d2e7a11-92ad-db06-177b-c6602ef1acd4@gmail.com> In-Reply-To: <3d2e7a11-92ad-db06-177b-c6602ef1acd4@gmail.com> From: Koba Ko Date: Fri, 4 Jun 2021 15:22:41 +0800 Message-ID: Subject: Re: [PATCH] r8169: introduce polling method for link change To: Heiner Kallweit Cc: "David S. Miller" , Jakub Kicinski , netdev@vger.kernel.org, Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 3, 2021 at 6:00 PM Heiner Kallweit 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 " > 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. > > > 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. > > 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. > > > Signed-off-by: Koba Ko > > --- > > 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 > > #include > > > > +#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.