diff mbox

[net,3/3] r8169: increase the lifespan of the hardware counters dump area.

Message ID fb3db9039225ed3a030e779565b8e11f99fc8ae2.1441454454.git.romieu@fr.zoreil.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Francois Romieu Sept. 5, 2015, 12:18 p.m. UTC 
From: Francois Romieu <romieu@fr.zoreil.com>

net/core/net-sysfs.c::netstat_show retrieves stats with spinlock held.

This change avoids sleepable allocation and performs some housekeeping:
- receive ring, transmit ring and counters dump area allocation failures
  are all considered fatal during open()
- netif_warn is now redundant with rtl_reset_counters_cond built-in
  failure message.
- rtl_reset_counters_cond induced failures in open() are also considered
  fatal: it takes acceptable work to unwind comfortably.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=104031
Fixes: 6e85d5ad36a2 ("r8169: Add values missing in @get_stats64 from HW counters")
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
Cc: Corinna Vinschen <vinschen@redhat.com>
Cc: pomidorabelisima@gmail.com
---
 drivers/net/ethernet/realtek/r8169.c | 132 +++++++++++++++--------------------
 1 file changed, 55 insertions(+), 77 deletions(-)

Comments

Corinna Vinschen Sept. 6, 2015, 10:37 a.m. UTC | #1 
On Sep  5 14:18, romieu@fr.zoreil.com wrote:
> From: Francois Romieu <romieu@fr.zoreil.com>
> 
> net/core/net-sysfs.c::netstat_show retrieves stats with spinlock held.
> 
> This change avoids sleepable allocation and performs some housekeeping:
> - receive ring, transmit ring and counters dump area allocation failures
>   are all considered fatal during open()
> - netif_warn is now redundant with rtl_reset_counters_cond built-in
>   failure message.
> - rtl_reset_counters_cond induced failures in open() are also considered
>   fatal: it takes acceptable work to unwind comfortably.

Why?  The counter reset is not a fatal condition for the operation of
the NIC.  Even if the counters show incorrect values, the normal
operation of the NIC is not affected.  And to top that off, even if
resetting the counters actually doesn't work, the driver keeps the
offset values, so a failed reset won't even harm the statistics.  It
just doesn't make sense to break the NIC operation for such a minor
problem.  And worse:

> -static bool rtl8169_reset_counters(struct net_device *dev)
> +static int rtl8169_reset_counters(struct net_device *dev)
>  {
>  	struct rtl8169_private *tp = netdev_priv(dev);
> -	struct rtl8169_counters *counters;
> -	dma_addr_t paddr;
> -	bool ret = true;
>  
>  	/*
>  	 * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the
>  	 * tally counters.
>  	 */
>  	if (tp->mac_version < RTL_GIGA_MAC_VER_19)
> -		return true;
> -
> -	counters = rtl8169_map_counters(dev, &paddr, CounterReset);
> -	if (!counters)
> -		return false;
> -
> -	if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000))
> -		ret = false;
> -
> -	rtl8169_unmap_counters(dev, paddr, counters);
> +		return -EINVAL;

This returns -EINVAL even for older chip versions which are not capable
of resetting the counters.  The result is, this driver will not work at
all on chip versions prior to RTL_GIGA_MAC_VER_19 anymore, because
rtl_open will always fail.

Other then that, I can test the patch probably tomorrow.


Corinna
Francois Romieu Sept. 6, 2015, 8:21 p.m. UTC | #2 
Corinna Vinschen <vinschen@redhat.com> :
> On Sep  5 14:18, romieu@fr.zoreil.com wrote:
[...]
> > - rtl_reset_counters_cond induced failures in open() are also considered
> >   fatal: it takes acceptable work to unwind comfortably.
> 
> Why?


Crap, my description does not match the code wrt rtl_reset_counters_cond. :o/

s/rtl8169_reset_counters/rtl8169_update_counters/g

The code is right.

[...]
> This returns -EINVAL even for older chip versions which are not capable
> of resetting the counters.  The result is, this driver will not work at
> all on chip versions prior to RTL_GIGA_MAC_VER_19 anymore, because
> rtl_open will always fail.

No. My changelog was misleading. rtl_init_counter_offsets handles this part
correctly.
David Miller Sept. 7, 2015, 7:05 a.m. UTC | #3 
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sun, 6 Sep 2015 22:21:53 +0200

> Corinna Vinschen <vinschen@redhat.com> :
>> On Sep  5 14:18, romieu@fr.zoreil.com wrote:
> [...]
>> > - rtl_reset_counters_cond induced failures in open() are also considered
>> >   fatal: it takes acceptable work to unwind comfortably.
>> 
>> Why?
> 
> 
> Crap, my description does not match the code wrt rtl_reset_counters_cond. :o/
> 
> s/rtl8169_reset_counters/rtl8169_update_counters/g
> 
> The code is right.

Please resubmit with fixed commit log messages, thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Corinna Vinschen Sept. 7, 2015, 9:29 a.m. UTC | #4 
On Sep  6 22:21, Francois Romieu wrote:
> Corinna Vinschen <vinschen@redhat.com> :
> > On Sep  5 14:18, romieu@fr.zoreil.com wrote:
> [...]
> > > - rtl_reset_counters_cond induced failures in open() are also considered
> > >   fatal: it takes acceptable work to unwind comfortably.
> > 
> > Why?
> 
> 
> Crap, my description does not match the code wrt rtl_reset_counters_cond. :o/
> 
> s/rtl8169_reset_counters/rtl8169_update_counters/g
> 
> The code is right.
> 
> [...]
> > This returns -EINVAL even for older chip versions which are not capable
> > of resetting the counters.  The result is, this driver will not work at
> > all on chip versions prior to RTL_GIGA_MAC_VER_19 anymore, because
> > rtl_open will always fail.
> 
> No. My changelog was misleading. rtl_init_counter_offsets handles this part
> correctly.

Oh, right, I missed that rtl_init_counter_offsets checks for `if (!rc)',
not for `if (rc < 0)' as for the call to rtl8169_update_counters.

Still wondering though.  Given that the driver never failed before if
the counter values couldn't be updated, and given that these counter
values only have statistical relevance, why should this suddenly result
in a fatal failure at open time?


Corinna
David Miller Sept. 8, 2015, midnight UTC | #5 
From: Corinna Vinschen <vinschen@redhat.com>
Date: Mon, 7 Sep 2015 11:29:49 +0200

> Still wondering though.  Given that the driver never failed before if
> the counter values couldn't be updated, and given that these counter
> values only have statistical relevance, why should this suddenly result
> in a fatal failure at open time?

Failing to allocate such a small buffer means we have much deeper issues
at hand.  A GFP_KERNEL allocation of this size really should not fail.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Corinna Vinschen Sept. 8, 2015, 8:05 a.m. UTC | #6 
Hi David,

On Sep  7 17:00, David Miller wrote:
> From: Corinna Vinschen <vinschen@redhat.com>
> Date: Mon, 7 Sep 2015 11:29:49 +0200
> 
> > Still wondering though.  Given that the driver never failed before if
> > the counter values couldn't be updated, and given that these counter
> > values only have statistical relevance, why should this suddenly result
> > in a fatal failure at open time?
> 
> Failing to allocate such a small buffer means we have much deeper issues
> at hand.  A GFP_KERNEL allocation of this size really should not fail.

I'm not talking about the allocation.  I agree with you on that score.

What I'm talking about is the situation where the NIC hardware fails to
reset or update its own counters for whatever reason.  Apparently the
mechanism is supposed to be performed within a given timeframe.  The
code sets some registers and then waits for a flag bit to be set to 0.
For that it utilizes a busy loop checking the flag bit up to 1000 times
with a delay of about 10 us.

The error condition is that the flag bit hasn't been set to 0 when the
loop exits, after roughly 10ms, and *this* part does not constitute a
fatal error which breaks the operation of the NIC.  So, from my
perspective a timeout while trying to wait for updated counter values
from the NIC at @ndo_open time should not be treated as fatal.


Corinna
diff mbox

Patch

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index c0a5edb..ae07567 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -833,8 +833,11 @@  struct rtl8169_private {
 	unsigned features;
 
 	struct mii_if_info mii;
+
 	struct rtl8169_counters *counters;
 	struct rtl8169_tc_offsets tc_offset;
+	dma_addr_t counters_map;
+
 	u32 saved_wolopts;
 	u32 opts1_mask;
 
@@ -2197,64 +2200,33 @@  DECLARE_RTL_COND(rtl_reset_counters_cond)
 	return RTL_R32(CounterAddrLow) & CounterReset;
 }
 
-static struct rtl8169_counters *rtl8169_map_counters(struct net_device *dev,
-						     dma_addr_t *paddr,
-						     u32 counter_cmd)
+static int rtl8169_cmd_counters(struct net_device *dev, u32 counter_cmd)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	void __iomem *ioaddr = tp->mmio_addr;
-	struct device *d = &tp->pci_dev->dev;
-	struct rtl8169_counters *counters;
+	dma_addr_t paddr = tp->counters_map;
 	u32 cmd;
 
-	counters = dma_alloc_coherent(d, sizeof(*counters), paddr, GFP_KERNEL);
-	if (counters) {
-		RTL_W32(CounterAddrHigh, (u64)*paddr >> 32);
-		cmd = (u64)*paddr & DMA_BIT_MASK(32);
-		RTL_W32(CounterAddrLow, cmd);
-		RTL_W32(CounterAddrLow, cmd | counter_cmd);
-	}
-	return counters;
-}
+	RTL_W32(CounterAddrHigh, (u64)paddr >> 32);
+	cmd = (u64)paddr & DMA_BIT_MASK(32);
+	RTL_W32(CounterAddrLow, cmd);
+	RTL_W32(CounterAddrLow, cmd | counter_cmd);
 
-static void rtl8169_unmap_counters (struct net_device *dev,
-				    dma_addr_t paddr,
-				    struct rtl8169_counters *counters)
-{
-	struct rtl8169_private *tp = netdev_priv(dev);
-	void __iomem *ioaddr = tp->mmio_addr;
-	struct device *d = &tp->pci_dev->dev;
-
-	RTL_W32(CounterAddrLow, 0);
-	RTL_W32(CounterAddrHigh, 0);
-
-	dma_free_coherent(d, sizeof(*counters), counters, paddr);
+	return rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000);
 }
 
-static bool rtl8169_reset_counters(struct net_device *dev)
+static int rtl8169_reset_counters(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
-	struct rtl8169_counters *counters;
-	dma_addr_t paddr;
-	bool ret = true;
 
 	/*
 	 * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the
 	 * tally counters.
 	 */
 	if (tp->mac_version < RTL_GIGA_MAC_VER_19)
-		return true;
-
-	counters = rtl8169_map_counters(dev, &paddr, CounterReset);
-	if (!counters)
-		return false;
-
-	if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000))
-		ret = false;
-
-	rtl8169_unmap_counters(dev, paddr, counters);
+		return -EINVAL;
 
-	return ret;
+	return rtl8169_cmd_counters(dev, CounterReset);
 }
 
 DECLARE_RTL_COND(rtl_counters_cond)
@@ -2264,44 +2236,30 @@  DECLARE_RTL_COND(rtl_counters_cond)
 	return RTL_R32(CounterAddrLow) & CounterDump;
 }
 
-static bool rtl8169_update_counters(struct net_device *dev)
+static int rtl8169_update_counters(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	void __iomem *ioaddr = tp->mmio_addr;
-	struct rtl8169_counters *counters;
-	dma_addr_t paddr;
-	bool ret = true;
 
 	/*
 	 * Some chips are unable to dump tally counters when the receiver
 	 * is disabled.
 	 */
 	if ((RTL_R8(ChipCmd) & CmdRxEnb) == 0)
-		return true;
-
-	counters = rtl8169_map_counters(dev, &paddr, CounterDump);
-	if (!counters)
-		return false;
-
-	if (rtl_udelay_loop_wait_low(tp, &rtl_counters_cond, 10, 1000))
-		memcpy(&tp->counters, counters, sizeof(*counters));
-	else
-		ret = false;
-
-	rtl8169_unmap_counters(dev, paddr, counters);
+		return -EINVAL;
 
-	return ret;
+	return rtl8169_cmd_counters(dev, CounterDump);
 }
 
-static bool rtl8169_init_counter_offsets(struct net_device *dev)
+static int rtl_init_counter_offsets(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	struct rtl8169_counters *counters = tp->counters;
 	struct rtl8169_tc_offsets *offset = &tp->tc_offset;
-	bool ret = false;
+	int rc;
 
 	/*
-	 * rtl8169_init_counter_offsets is called from rtl_open.  On chip
+	 * rtl_init_counter_offsets is called from rtl_open.  On chip
 	 * versions prior to RTL_GIGA_MAC_VER_19 the tally counters are only
 	 * reset by a power cycle, while the counter values collected by the
 	 * driver are reset at every driver unload/load cycle.
@@ -2310,27 +2268,29 @@  static bool rtl8169_init_counter_offsets(struct net_device *dev)
 	 * values, we collect the initial values at first open(*) and use them
 	 * as offsets to normalize the values returned by @get_stats64.
 	 *
-	 * (*) We can't call rtl8169_init_counter_offsets from rtl_init_one
+	 * (*) We can't call rtl_init_counter_offsets from rtl_init_one
 	 * for the reason stated in rtl8169_update_counters; CmdRxEnb is only
 	 * set at open time by rtl_hw_start.
 	 */
 
 	if (offset->inited)
-		return true;
+		return 0;
 
 	/* If both, reset and update fail, propagate to caller. */
-	if (rtl8169_reset_counters(dev))
-		ret = true;
+	rc = rtl8169_reset_counters(dev);
+	if (!rc)
+		goto out;
 
-	if (rtl8169_update_counters(dev))
-		ret = true;
+	rc = rtl8169_update_counters(dev);
+	if (rc < 0)
+		goto out;
 
 	offset->tx_errors = counters->tx_errors;
 	offset->tx_multi_collision = counters->tx_multi_collision;
 	offset->tx_aborted = counters->tx_aborted;
 	offset->inited = true;
-
-	return ret;
+out:
+	return rc;
 }
 
 static void rtl8169_get_ethtool_stats(struct net_device *dev,
@@ -7674,8 +7634,8 @@  static int rtl8169_close(struct net_device *dev)
 
 	free_irq(pdev->irq, dev);
 
-	kfree(tp->counters);
-
+	dma_free_coherent(&pdev->dev, sizeof(*tp->counters), tp->counters,
+			  tp->counters_map);
 	dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
 			  tp->RxPhyAddr);
 	dma_free_coherent(&pdev->dev, R8169_TX_RING_BYTES, tp->TxDescArray,
@@ -7720,7 +7680,8 @@  static int rtl_open(struct net_device *dev)
 	if (!tp->RxDescArray)
 		goto err_free_tx_0;
 
-	tp->counters = kmalloc(sizeof(*tp->counters), GFP_KERNEL);
+	tp->counters = dma_alloc_coherent(&pdev->dev, sizeof(*tp->counters),
+					  &tp->counters_map, GFP_KERNEL);
 	if (!tp->counters)
 		goto err_free_rx_1;
 
@@ -7742,8 +7703,6 @@  static int rtl_open(struct net_device *dev)
 
 	rtl_lock_work(tp);
 
-	set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
-
 	napi_enable(&tp->napi);
 
 	rtl8169_init_phy(dev, tp);
@@ -7754,8 +7713,22 @@  static int rtl_open(struct net_device *dev)
 
 	rtl_hw_start(dev);
 
-	if (!rtl8169_init_counter_offsets(dev))
-		netif_warn(tp, hw, dev, "counter reset/update failed\n");
+	retval = rtl_init_counter_offsets(dev);
+	if (retval < 0) {
+		/*
+		 * Late error but the current thread is still in control:
+		 * - deferred work could have been scheduled - and it could
+		 *   be actually waiting for its mutex to be released - but
+		 *   we still haven't allowed it to perform real work.
+		 * - transmit timeout timer can't run either.
+		 */
+		rtl8169_hw_reset(tp);
+		rtl_pll_power_down(tp);
+		rtl_unlock_work(tp);
+		goto err_napi_disable_4;
+	}
+
+	set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
 
 	netif_start_queue(dev);
 
@@ -7768,11 +7741,16 @@  static int rtl_open(struct net_device *dev)
 out:
 	return retval;
 
+err_napi_disable_4:
+	napi_disable(&tp->napi);
+	cancel_work_sync(&tp->wk.work);
+	free_irq(pdev->irq, dev);
 err_release_fw_3:
 	rtl_release_firmware(tp);
 	rtl8169_rx_clear(tp);
 err_free_counters_2:
-	kfree(tp->counters);
+	dma_free_coherent(&pdev->dev, sizeof(*tp->counters), tp->counters,
+			  tp->counters_map);
 	tp->counters = NULL;
 err_free_rx_1:
 	dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,