[PATCH net] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.

[Chunhao Lin]

There will be a log spam when there is no cable plugged.
Please refer to following links.
https://bugzilla.kernel.org/show_bug.cgi?id=104351
https://bugzilla.kernel.org/show_bug.cgi?id=107421

This issue is caused by runtime power management. When there is no cable
plugged, the driver will be suspend (runtime suspend) by OS and NIC will
be put into the D3 state. During this time, if OS call rtl8169_get_stats64()
to dump tally counter, because NIC is in D3 state, the register value read by
driver will return all 0xff. This will let driver think tally counter flag is not
toggled and then sends the warning message "rtl_counters_cond == 1 (loop: 1000,
delay: 10)" to kernel log.

I add checking driver's pm runtime status in rtl8169_get_stats64() to fix
this issue.

Signed-off-by: Chunhao Lin <h...@realtek.com>
---
drivers/net/ethernet/realtek/r8169.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 537974c..798b30c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7730,10 +7730,13 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
{
struct rtl8169_private *tp = netdev_priv(dev);
void __iomem *ioaddr = tp->mmio_addr;
+ struct pci_dev *pdev = tp->pci_dev;
struct rtl8169_counters *counters = tp->counters;
unsigned int start;

- if (netif_running(dev))
+ pm_runtime_get_noresume(&pdev->dev);
+
+ if (netif_running(dev) && pm_runtime_active(&pdev->dev))
rtl8169_rx_missed(dev, ioaddr);

do {
@@ -7761,7 +7764,8 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
* Fetch additonal counter values missing in stats collected by driver
* from tally counters.
*/
- rtl8169_update_counters(dev);
+ if (pm_runtime_active(&pdev->dev))
+ rtl8169_update_counters(dev);

/*
* Subtract values fetched during initalization.
@@ -7774,6 +7778,8 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
stats->tx_aborted_errors = le16_to_cpu(counters->tx_aborted) -
le16_to_cpu(tp->tc_offset.tx_aborted);

+ pm_runtime_put_noidle(&pdev->dev);
+
return stats;
}

--
1.9.1

[Francois Romieu]

Chunhao Lin <h...@realtek.com> :
[...]

> I add checking driver's pm runtime status in rtl8169_get_stats64() to fix
> this issue.

Would you consider taking the device out of suspended mode during
rtl8169_get_stats64 to prevent outdated stats ?

--
Ueimor

[Hau]

When in runtime suspend, it mean there is no traffic on NIC, so I did not wake the device during rtl8169_get_stats64().
Maybe we can update tally counter before entering runtime suspend mode.

------Please consider the environment before printing this e-mail.

[David Miller]

From: Chunhao Lin <h...@realtek.com>
Date: Thu, 18 Feb 2016 22:57:07 +0800

> There will be a log spam when there is no cable plugged.
> Please refer to following links.
> https://bugzilla.kernel.org/show_bug.cgi?id=104351
> https://bugzilla.kernel.org/show_bug.cgi?id=107421
>
> This issue is caused by runtime power management. When there is no cable
> plugged, the driver will be suspend (runtime suspend) by OS and NIC will
> be put into the D3 state. During this time, if OS call rtl8169_get_stats64()
> to dump tally counter, because NIC is in D3 state, the register value read by
> driver will return all 0xff. This will let driver think tally counter flag is not
> toggled and then sends the warning message "rtl_counters_cond == 1 (loop: 1000,
> delay: 10)" to kernel log.
>
> I add checking driver's pm runtime status in rtl8169_get_stats64() to fix
> this issue.
>
> Signed-off-by: Chunhao Lin <h...@realtek.com>

If you are going to do this, then you need to quiesce the device RX/TX
and capture all of the statistics from the chip before suspending it.

Otherwise what we're returning from rtl8169_get_stats64() is inaccurate.

[Chunhao Lin]

There will be a log spam when there is no cable plugged.
Please refer to following links.
https://bugzilla.kernel.org/show_bug.cgi?id=104351
https://bugzilla.kernel.org/show_bug.cgi?id=107421

This issue is caused by runtime power management. When there is no cable
plugged, the driver will be suspend (runtime suspend) by OS and NIC will
be put into the D3 state. During this time, if OS call rtl8169_get_stats64()
to dump tally counter, because NIC is in D3 state, the register value read by
driver will return all 0xff. This will let driver think tally counter flag is not
toggled and then sends the warning message "rtl_counters_cond == 1 (loop: 1000,
delay: 10)" to kernel log.

For fixing this issue,
1.add checking driver's pm runtime status in rtl8169_get_stats64().
2.dump tally counter before going runtime suspend for counter accuracy in
runtime suspend.

Signed-off-by: Chunhao Lin <h...@realtek.com>
---

drivers/net/ethernet/realtek/r8169.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 537974c..a645f8d 100644

@@ -7842,6 +7848,12 @@ static int rtl8169_runtime_suspend(struct device *device)
struct pci_dev *pdev = to_pci_dev(device);
struct net_device *dev = pci_get_drvdata(pdev);

struct rtl8169_private *tp = netdev_priv(dev);

+ void __iomem *ioaddr = tp->mmio_addr;
+
+ /* Update counters before going runtime suspend */
+ if (netif_running(dev))
+ rtl8169_rx_missed(dev, ioaddr);
+ rtl8169_update_counters(dev);

if (!tp->TxDescArray)
return 0;
--
1.9.1

[Francois Romieu]

Chunhao Lin <h...@realtek.com> :
[...]

> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 537974c..a645f8d 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -7730,10 +7730,13 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
> {
> struct rtl8169_private *tp = netdev_priv(dev);
> void __iomem *ioaddr = tp->mmio_addr;
> + struct pci_dev *pdev = tp->pci_dev;

+ struct device *d = &tp->pci_dev->dev;

(the patch does not use pdev alone)

[...]

> @@ -7761,7 +7764,8 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
> * Fetch additonal counter values missing in stats collected by driver
> * from tally counters.
> */
> - rtl8169_update_counters(dev);
> + if (pm_runtime_active(&pdev->dev))
> + rtl8169_update_counters(dev);

pm_runtime_active() won't change after pm_runtime_get_noresume(). You may
set a boolean active = pm_runtime_active(d) before testing netif_running().

[...]

> @@ -7842,6 +7848,12 @@ static int rtl8169_runtime_suspend(struct device *device)
> struct pci_dev *pdev = to_pci_dev(device);
> struct net_device *dev = pci_get_drvdata(pdev);
> struct rtl8169_private *tp = netdev_priv(dev);
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + /* Update counters before going runtime suspend */
> + if (netif_running(dev))
> + rtl8169_rx_missed(dev, ioaddr);
> + rtl8169_update_counters(dev);
>
> if (!tp->TxDescArray)
> return 0;

Nits:

- the tp->TxDescArray test provides the required synchronization: see
rtl8169_{open/close} and their pm_runtime_{get / put}.

- ioaddr is not really needed : tp->mmio_addr appears only once and it does
not mess the 72..80 cols limit.

- even if the device can only be automatically runtime suspended some time
after a link down event, you may address davem's point regarding stats
reliability and move rtl8169_rx_missed + rtl8169_update_counters after
rtl8169_net_suspend.

--
Ueimor

[Hau]

> Nits:
>
> - the tp->TxDescArray test provides the required synchronization: see
> rtl8169_{open/close} and their pm_runtime_{get / put}.
>
> - ioaddr is not really needed : tp->mmio_addr appears only once and it does
> not mess the 72..80 cols limit.
>
> - even if the device can only be automatically runtime suspended some time
> after a link down event, you may address davem's point regarding stats
> reliability and move rtl8169_rx_missed + rtl8169_update_counters after
> rtl8169_net_suspend.

I will submit the new patch according to your advice.

Thanks.

[Chunhao Lin]

There will be a log spam when there is no cable plugged.
Please refer to following links.
https://bugzilla.kernel.org/show_bug.cgi?id=104351
https://bugzilla.kernel.org/show_bug.cgi?id=107421

This issue is caused by runtime power management. When there is no cable plugged, the driver will be suspend (runtime suspend) by OS and NIC will be put into the D3 state. During this time, if OS call rtl8169_get_stats64() to dump tally counter, because NIC is in D3 state, the register value read by driver will return all 0xff. This will let driver think tally counter flag is not toggled and then sends the warning message "rtl_counters_cond == 1 (loop: 1000,
delay: 10)" to kernel log.

For fixing this issue,
1.add checking driver's pm runtime status in rtl8169_get_stats64().
2.dump tally counter before going runtime suspend for counter accuracy in
runtime suspend.

Signed-off-by: Chunhao Lin <h...@realtek.com>
---

drivers/net/ethernet/realtek/r8169.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 537974c..404be51 100644

--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7730,10 +7730,13 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
{
struct rtl8169_private *tp = netdev_priv(dev);
void __iomem *ioaddr = tp->mmio_addr;
+ struct pci_dev *pdev = tp->pci_dev;

struct rtl8169_counters *counters = tp->counters;
unsigned int start;

- if (netif_running(dev))
+ pm_runtime_get_noresume(&pdev->dev);
+
+ if (netif_running(dev) && pm_runtime_active(&pdev->dev))
rtl8169_rx_missed(dev, ioaddr);

do {

@@ -7761,7 +7764,8 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
* Fetch additonal counter values missing in stats collected by driver
* from tally counters.
*/
- rtl8169_update_counters(dev);
+ if (pm_runtime_active(&pdev->dev))
+ rtl8169_update_counters(dev);

/*
* Subtract values fetched during initalization.
@@ -7774,6 +7778,8 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
stats->tx_aborted_errors = le16_to_cpu(counters->tx_aborted) -
le16_to_cpu(tp->tc_offset.tx_aborted);

+ pm_runtime_put_noidle(&pdev->dev);
+
return stats;
}

@@ -7853,6 +7859,11 @@ static int rtl8169_runtime_suspend(struct device *device)

rtl8169_net_suspend(dev);

+ /* Update counters before going runtime suspend */
+ if (netif_running(dev))

+ rtl8169_rx_missed(dev, tp->mmio_addr);
+ rtl8169_update_counters(dev);
+
return 0;
}

--
1.9.1

[Francois Romieu]

Chunhao Lin <h...@realtek.com> :
[...]

> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 537974c..404be51 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c

[...]

> @@ -7853,6 +7859,11 @@ static int rtl8169_runtime_suspend(struct device *device)
>
> rtl8169_net_suspend(dev);
>
> + /* Update counters before going runtime suspend */
> + if (netif_running(dev))

This test is useless (always true):

- rtl_open
[...]
pm_runtime_get_sync(&pdev->dev);
[...]
tp->TxDescArray = blah
[...]

- rtl8169_close
[...]
pm_runtime_get_sync(&pdev->dev);
[...]
tp->TxDescArray = NULL;

- rtl8169_runtime_suspend
[...]

if (!tp->TxDescArray)
return 0;

(the implicit smp barriers are mildly obvious, ok)

--
Ueimor

[Chunhao Lin]

There will be a log spam when there is no cable plugged. Please refer to
following links. https://bugzilla.kernel.org/show_bug.cgi?id=104351
https://bugzilla.kernel.org/show_bug.cgi?id=107421

This issue is caused by runtime power management. When there is no cable
plugged, the driver will be suspend (runtime suspend) by OS and NIC will be
put into the D3 state. During this time, if OS call rtl8169_get_stats64()
to dump tally counter, because NIC is in D3 state, the register value read
by driver will return all 0xff. This will let driver think tally counter
flag is not toggled and then sends the warning message "rtl_counters_cond
== 1 (loop: 1000, delay: 10)" to kernel log.

For fixing this issue, 1.add checking driver's pm runtime status in
rtl8169_get_stats64(). 2.dump tally counter before going runtime suspend
for counter accuracy in runtime suspend.

Signed-off-by: Chunhao Lin <h...@realtek.com>
---

drivers/net/ethernet/realtek/r8169.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 537974c..4caeacb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c

@@ -7853,6 +7859,10 @@ static int rtl8169_runtime_suspend(struct device *device)

rtl8169_net_suspend(dev);

+ /* Update counters before going runtime suspend */

[Francois Romieu]

Chunhao Lin <h...@realtek.com> :
[...]

Fine with me.

Is there any chance for the set of chipset dependent registers that
are safe to be read when in D3 state to be documented ?

--
Ueimor

[Hau]

> Fine with me.
>
> Is there any chance for the set of chipset dependent registers that are safe to
> be read when in D3 state to be documented ?
>

I think except registers in PCI configuration, all other registers should be read in D0 state.

------Please consider the environment before printing this e-mail.

[David Miller]

From: Chunhao Lin <h...@realtek.com>
Date: Wed, 24 Feb 2016 14:18:42 +0800

> There will be a log spam when there is no cable plugged. Please refer to
> following links. https://bugzilla.kernel.org/show_bug.cgi?id=104351
> https://bugzilla.kernel.org/show_bug.cgi?id=107421
>
> This issue is caused by runtime power management. When there is no cable
> plugged, the driver will be suspend (runtime suspend) by OS and NIC will be
> put into the D3 state. During this time, if OS call rtl8169_get_stats64()
> to dump tally counter, because NIC is in D3 state, the register value read
> by driver will return all 0xff. This will let driver think tally counter
> flag is not toggled and then sends the warning message "rtl_counters_cond
> == 1 (loop: 1000, delay: 10)" to kernel log.
>
> For fixing this issue, 1.add checking driver's pm runtime status in
> rtl8169_get_stats64(). 2.dump tally counter before going runtime suspend
> for counter accuracy in runtime suspend.
>
> Signed-off-by: Chunhao Lin <h...@realtek.com>

Applied, thanks.