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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160222231301.GA17876@electric-eye.fr.zoreil.com>
Date:	Tue, 23 Feb 2016 00:13:01 +0100
From:	Francois Romieu <romieu@...zoreil.com>
To:	Chunhao Lin <hau@...ltek.com>
Cc:	netdev@...r.kernel.org, nic_swsd@...ltek.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2] r8169:fix "rtl_counters_cond == 1 (loop: 1000,
 delay: 10)" log spam.

Chunhao Lin <hau@...ltek.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