[PATCH net 0/3] r8169: fix sleepable allocation during netdevice stats retrieval.

[PATCH net 0/3] r8169: fix sleepable allocation during netdevice stats retrieval.

[romieu]

From: Francois Romieu <romieu@fr.zoreil.com>

This series applies against davem's net as of
724a7636ad026a3a68f3fc626ccd04111f65cfd9 ("Merge branch 'sctp-fixes').

It's untested though reviewable. I should be able to try it this evening.

Francois Romieu (3):
  r8169: decouple the counters data and the device private area.
  r8169: move rtl_reset_counters_cond before the hardware counters helpers.
  r8169: increase the lifespan of the hardware counters dump area.

 drivers/net/ethernet/realtek/r8169.c | 200 +++++++++++++++++------------------
 1 file changed, 96 insertions(+), 104 deletions(-)

-- 
2.4.3

[PATCH net 1/3] r8169: decouple the counters data and the device private area.

[romieu]

From: Francois Romieu <romieu@fr.zoreil.com>

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 | 70 +++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 24dcbe6..73a60a7 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -833,7 +833,7 @@ struct rtl8169_private {
 	unsigned features;
 
 	struct mii_if_info mii;
-	struct rtl8169_counters counters;
+	struct rtl8169_counters *counters;
 	struct rtl8169_tc_offsets tc_offset;
 	u32 saved_wolopts;
 	u32 opts1_mask;
@@ -2296,6 +2296,8 @@ static bool rtl8169_update_counters(struct net_device *dev)
 static bool rtl8169_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;
 
 	/*
@@ -2313,7 +2315,7 @@ static bool rtl8169_init_counter_offsets(struct net_device *dev)
 	 * set at open time by rtl_hw_start.
 	 */
 
-	if (tp->tc_offset.inited)
+	if (offset->inited)
 		return true;
 
 	/* If both, reset and update fail, propagate to caller. */
@@ -2323,10 +2325,10 @@ static bool rtl8169_init_counter_offsets(struct net_device *dev)
 	if (rtl8169_update_counters(dev))
 		ret = true;
 
-	tp->tc_offset.tx_errors = tp->counters.tx_errors;
-	tp->tc_offset.tx_multi_collision = tp->counters.tx_multi_collision;
-	tp->tc_offset.tx_aborted = tp->counters.tx_aborted;
-	tp->tc_offset.inited = true;
+	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;
 }
@@ -2335,24 +2337,25 @@ static void rtl8169_get_ethtool_stats(struct net_device *dev,
 				      struct ethtool_stats *stats, u64 *data)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
+	struct rtl8169_counters *c = tp->counters;
 
 	ASSERT_RTNL();
 
 	rtl8169_update_counters(dev);
 
-	data[0] = le64_to_cpu(tp->counters.tx_packets);
-	data[1] = le64_to_cpu(tp->counters.rx_packets);
-	data[2] = le64_to_cpu(tp->counters.tx_errors);
-	data[3] = le32_to_cpu(tp->counters.rx_errors);
-	data[4] = le16_to_cpu(tp->counters.rx_missed);
-	data[5] = le16_to_cpu(tp->counters.align_errors);
-	data[6] = le32_to_cpu(tp->counters.tx_one_collision);
-	data[7] = le32_to_cpu(tp->counters.tx_multi_collision);
-	data[8] = le64_to_cpu(tp->counters.rx_unicast);
-	data[9] = le64_to_cpu(tp->counters.rx_broadcast);
-	data[10] = le32_to_cpu(tp->counters.rx_multicast);
-	data[11] = le16_to_cpu(tp->counters.tx_aborted);
-	data[12] = le16_to_cpu(tp->counters.tx_underun);
+	data[ 0] = le64_to_cpu(c->tx_packets);
+	data[ 1] = le64_to_cpu(c->rx_packets);
+	data[ 2] = le64_to_cpu(c->tx_errors);
+	data[ 3] = le32_to_cpu(c->rx_errors);
+	data[ 4] = le16_to_cpu(c->rx_missed);
+	data[ 5] = le16_to_cpu(c->align_errors);
+	data[ 6] = le32_to_cpu(c->tx_one_collision);
+	data[ 7] = le32_to_cpu(c->tx_multi_collision);
+	data[ 8] = le64_to_cpu(c->rx_unicast);
+	data[ 9] = le64_to_cpu(c->rx_broadcast);
+	data[10] = le32_to_cpu(c->rx_multicast);
+	data[11] = le16_to_cpu(c->tx_aborted);
+	data[12] = le16_to_cpu(c->tx_underun);
 }
 
 static void rtl8169_get_strings(struct net_device *dev, u32 stringset, u8 *data)
@@ -7671,6 +7674,8 @@ static int rtl8169_close(struct net_device *dev)
 
 	free_irq(pdev->irq, dev);
 
+	kfree(tp->counters);
+
 	dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
 			  tp->RxPhyAddr);
 	dma_free_coherent(&pdev->dev, R8169_TX_RING_BYTES, tp->TxDescArray,
@@ -7715,9 +7720,13 @@ static int rtl_open(struct net_device *dev)
 	if (!tp->RxDescArray)
 		goto err_free_tx_0;
 
+	tp->counters = kmalloc(sizeof(*tp->counters), GFP_KERNEL);
+	if (!tp->counters)
+		goto err_free_rx_1;
+
 	retval = rtl8169_init_ring(dev);
 	if (retval < 0)
-		goto err_free_rx_1;
+		goto err_free_counters_2;
 
 	INIT_WORK(&tp->wk.work, rtl_task);
 
@@ -7729,7 +7738,7 @@ static int rtl_open(struct net_device *dev)
 			     (tp->features & RTL_FEATURE_MSI) ? 0 : IRQF_SHARED,
 			     dev->name, dev);
 	if (retval < 0)
-		goto err_release_fw_2;
+		goto err_release_fw_3;
 
 	rtl_lock_work(tp);
 
@@ -7759,9 +7768,12 @@ static int rtl_open(struct net_device *dev)
 out:
 	return retval;
 
-err_release_fw_2:
+err_release_fw_3:
 	rtl_release_firmware(tp);
 	rtl8169_rx_clear(tp);
+err_free_counters_2:
+	kfree(tp->counters);
+	tp->counters = NULL;
 err_free_rx_1:
 	dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
 			  tp->RxPhyAddr);
@@ -7779,6 +7791,8 @@ static struct rtnl_link_stats64 *
 rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
+	struct rtl8169_tc_offsets *offset = &tp->tc_offset;
+	struct rtl8169_counters *counters = tp->counters;
 	void __iomem *ioaddr = tp->mmio_addr;
 	unsigned int start;
 
@@ -7816,12 +7830,12 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 	 * Subtract values fetched during initalization.
 	 * See rtl8169_init_counter_offsets for a description why we do that.
 	 */
-	stats->tx_errors = le64_to_cpu(tp->counters.tx_errors) -
-		le64_to_cpu(tp->tc_offset.tx_errors);
-	stats->collisions = le32_to_cpu(tp->counters.tx_multi_collision) -
-		le32_to_cpu(tp->tc_offset.tx_multi_collision);
-	stats->tx_aborted_errors = le16_to_cpu(tp->counters.tx_aborted) -
-		le16_to_cpu(tp->tc_offset.tx_aborted);
+	stats->tx_errors = le64_to_cpu(counters->tx_errors) -
+			   le64_to_cpu(offset->tx_errors);
+	stats->collisions = le32_to_cpu(counters->tx_multi_collision) -
+			    le32_to_cpu(offset->tx_multi_collision);
+	stats->tx_aborted_errors = le16_to_cpu(counters->tx_aborted) -
+				   le16_to_cpu(offset->tx_aborted);
 
 	return stats;
 }
-- 
2.4.3

[PATCH net 2/3] r8169: move rtl_reset_counters_cond before the hardware counters helpers.

[romieu]

From: Francois Romieu <romieu@fr.zoreil.com>

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 | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 73a60a7..c0a5edb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -2190,6 +2190,13 @@ static int rtl8169_get_sset_count(struct net_device *dev, int sset)
 	}
 }
 
+DECLARE_RTL_COND(rtl_reset_counters_cond)
+{
+	void __iomem *ioaddr = tp->mmio_addr;
+
+	return RTL_R32(CounterAddrLow) & CounterReset;
+}
+
 static struct rtl8169_counters *rtl8169_map_counters(struct net_device *dev,
 						     dma_addr_t *paddr,
 						     u32 counter_cmd)
@@ -2224,13 +2231,6 @@ static void rtl8169_unmap_counters (struct net_device *dev,
 	dma_free_coherent(d, sizeof(*counters), counters, paddr);
 }
 
-DECLARE_RTL_COND(rtl_reset_counters_cond)
-{
-	void __iomem *ioaddr = tp->mmio_addr;
-
-	return RTL_R32(CounterAddrLow) & CounterReset;
-}
-
 static bool rtl8169_reset_counters(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
-- 
2.4.3

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

[romieu]

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(-)

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,
-- 
2.4.3

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH net 1/3] r8169: decouple the counters data and the device private area.

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

On Sep  5 14:18, romieu@fr.zoreil.com wrote:
> From: Francois Romieu <romieu@fr.zoreil.com>
> @@ -2335,24 +2337,25 @@ static void rtl8169_get_ethtool_stats(struct net_device *dev,
>  				      struct ethtool_stats *stats, u64 *data)
>  {
>  	struct rtl8169_private *tp = netdev_priv(dev);
> +	struct rtl8169_counters *c = tp->counters;

Minor style nit: Shouldn't that local var ideally be called "counters"
as you did in the other functions?


Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Francois Romieu]

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.

-- 
Ueimor

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

[David Miller]

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!

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 5041 bytes --]

On Sep  6 12:37, Corinna Vinschen wrote:
> 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.

I have a bit of a problem with this patch.  It crashes when loading the
driver manually w/ modprobe.  For some reason dev_get_stats is called
during rtl_init_one and at that time the counters pointer is NULL, so
the kernel gets a SEGV.  After I worked around that I got a SEGV in
rtl_remove_one, which I still have to find out why.  I didn't test with
the latest kernel, though, so I still have to check if that's the reason
for the crashes.  That takes a bit longer, I just wanted to let you
know.

There's also a problem with rtl8169_cmd_counters:  It always calls
rtl_udelay_loop_wait_low w/ rtl_reset_counters_cond, even when called
from rtl8169_update_counters, where it should call
rtl_udelay_loop_wait_low w/ rtl_counters_cond to check for the
CounterDump flag, rather than for the CounterReset flag.

For now I applied the below patch, but I think it's a bit ugly to
conditionalize the rtl_cond struct by the incoming flag value.


Corinna


diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index ae07567..cd7adbf 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -2200,6 +2200,13 @@ DECLARE_RTL_COND(rtl_reset_counters_cond)
 	return RTL_R32(CounterAddrLow) & CounterReset;
 }
 
+DECLARE_RTL_COND(rtl_counters_cond)
+{
+	void __iomem *ioaddr = tp->mmio_addr;
+
+	return RTL_R32(CounterAddrLow) & CounterDump;
+}
+
 static int rtl8169_cmd_counters(struct net_device *dev, u32 counter_cmd)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
@@ -2212,7 +2219,10 @@ static int rtl8169_cmd_counters(struct net_device *dev, u32 counter_cmd)
 	RTL_W32(CounterAddrLow, cmd);
 	RTL_W32(CounterAddrLow, cmd | counter_cmd);
 
-	return rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000);
+	return rtl_udelay_loop_wait_low(tp,
+		counter_cmd == CounterDump ? &rtl_counters_cond :
+					     &rtl_reset_counters_cond,
+		10, 1000);
 }
 
 static int rtl8169_reset_counters(struct net_device *dev)
@@ -2229,13 +2239,6 @@ static int rtl8169_reset_counters(struct net_device *dev)
 	return rtl8169_cmd_counters(dev, CounterReset);
 }
 
-DECLARE_RTL_COND(rtl_counters_cond)
-{
-	void __iomem *ioaddr = tp->mmio_addr;
-
-	return RTL_R32(CounterAddrLow) & CounterDump;
-}
-
 static int rtl8169_update_counters(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
@@ -7800,8 +7803,11 @@ 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.
+	 * from tally counters, but only if we're already initialized.
 	 */
+	if (!counters)
+	  return stats;
+
 	rtl8169_update_counters(dev);
 
 	/*

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Francois Romieu]

Corinna Vinschen <vinschen@redhat.com> :
[...]
> I have a bit of a problem with this patch.  It crashes when loading the
> driver manually w/ modprobe.  For some reason dev_get_stats is called
> during rtl_init_one and at that time the counters pointer is NULL, so
> the kernel gets a SEGV.
>
>  After I worked around that I got a SEGV in
> rtl_remove_one, which I still have to find out why.  I didn't test with
> the latest kernel, though, so I still have to check if that's the reason
> for the crashes.  That takes a bit longer, I just wanted to let you
> know.

Call me stupid: I forgot that it's perfectly fine to request the stats
of a down interface. :o/

Updated patch is on the way.

> There's also a problem with rtl8169_cmd_counters:  It always calls
> rtl_udelay_loop_wait_low w/ rtl_reset_counters_cond, even when called
> from rtl8169_update_counters, where it should call
> rtl_udelay_loop_wait_low w/ rtl_counters_cond to check for the
> CounterDump flag, rather than for the CounterReset flag.
> 
> For now I applied the below patch, but I think it's a bit ugly to
> conditionalize the rtl_cond struct by the incoming flag value.

<captain obvious>

Let's check both at once:

	return RTL_R32(CounterAddrLow) & (CounterDump | CounterReset);

</captain obvious>

Thanks for reviewing.

-- 
Ueimor

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

[David Miller]

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.

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

[Francois Romieu]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

Francois Romieu <romieu@fr.zoreil.com> :
[...]
> Updated patch is on the way.

Fixed memcpy in patch 0001, moved counters allocation from open() 
to probe(), returned open() to its original state but something is
still wrong: the link does not come up.

Patches attached.

I'll try again tomorrow.

-- 
Ueimor

[-- Attachment #2: 0001-r8169-decouple-the-counters-data-and-the-device-priv.patch --]
[-- Type: text/plain, Size: 6134 bytes --]

>From 8f1b87a115f6c340558b95f6a1748ddbf866d95f Mon Sep 17 00:00:00 2001
Message-Id: <8f1b87a115f6c340558b95f6a1748ddbf866d95f.1441670158.git.romieu@fr.zoreil.com>
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 5 Sep 2015 13:26:30 +0200
Subject: [PATCH 1/3] r8169: decouple the counters data and the device private
 area.
X-Organisation: Land of Sunshine Inc.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/realtek/r8169.c | 69 ++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 24dcbe6..ff1b834 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -833,7 +833,7 @@ struct rtl8169_private {
 	unsigned features;
 
 	struct mii_if_info mii;
-	struct rtl8169_counters counters;
+	struct rtl8169_counters *counters;
 	struct rtl8169_tc_offsets tc_offset;
 	u32 saved_wolopts;
 	u32 opts1_mask;
@@ -2284,7 +2284,7 @@ static bool rtl8169_update_counters(struct net_device *dev)
 		return false;
 
 	if (rtl_udelay_loop_wait_low(tp, &rtl_counters_cond, 10, 1000))
-		memcpy(&tp->counters, counters, sizeof(*counters));
+		memcpy(tp->counters, counters, sizeof(*counters));
 	else
 		ret = false;
 
@@ -2296,6 +2296,8 @@ static bool rtl8169_update_counters(struct net_device *dev)
 static bool rtl8169_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;
 
 	/*
@@ -2313,7 +2315,7 @@ static bool rtl8169_init_counter_offsets(struct net_device *dev)
 	 * set at open time by rtl_hw_start.
 	 */
 
-	if (tp->tc_offset.inited)
+	if (offset->inited)
 		return true;
 
 	/* If both, reset and update fail, propagate to caller. */
@@ -2323,10 +2325,10 @@ static bool rtl8169_init_counter_offsets(struct net_device *dev)
 	if (rtl8169_update_counters(dev))
 		ret = true;
 
-	tp->tc_offset.tx_errors = tp->counters.tx_errors;
-	tp->tc_offset.tx_multi_collision = tp->counters.tx_multi_collision;
-	tp->tc_offset.tx_aborted = tp->counters.tx_aborted;
-	tp->tc_offset.inited = true;
+	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;
 }
@@ -2335,24 +2337,25 @@ static void rtl8169_get_ethtool_stats(struct net_device *dev,
 				      struct ethtool_stats *stats, u64 *data)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
+	struct rtl8169_counters *c = tp->counters;
 
 	ASSERT_RTNL();
 
 	rtl8169_update_counters(dev);
 
-	data[0] = le64_to_cpu(tp->counters.tx_packets);
-	data[1] = le64_to_cpu(tp->counters.rx_packets);
-	data[2] = le64_to_cpu(tp->counters.tx_errors);
-	data[3] = le32_to_cpu(tp->counters.rx_errors);
-	data[4] = le16_to_cpu(tp->counters.rx_missed);
-	data[5] = le16_to_cpu(tp->counters.align_errors);
-	data[6] = le32_to_cpu(tp->counters.tx_one_collision);
-	data[7] = le32_to_cpu(tp->counters.tx_multi_collision);
-	data[8] = le64_to_cpu(tp->counters.rx_unicast);
-	data[9] = le64_to_cpu(tp->counters.rx_broadcast);
-	data[10] = le32_to_cpu(tp->counters.rx_multicast);
-	data[11] = le16_to_cpu(tp->counters.tx_aborted);
-	data[12] = le16_to_cpu(tp->counters.tx_underun);
+	data[ 0] = le64_to_cpu(c->tx_packets);
+	data[ 1] = le64_to_cpu(c->rx_packets);
+	data[ 2] = le64_to_cpu(c->tx_errors);
+	data[ 3] = le32_to_cpu(c->rx_errors);
+	data[ 4] = le16_to_cpu(c->rx_missed);
+	data[ 5] = le16_to_cpu(c->align_errors);
+	data[ 6] = le32_to_cpu(c->tx_one_collision);
+	data[ 7] = le32_to_cpu(c->tx_multi_collision);
+	data[ 8] = le64_to_cpu(c->rx_unicast);
+	data[ 9] = le64_to_cpu(c->rx_broadcast);
+	data[10] = le32_to_cpu(c->rx_multicast);
+	data[11] = le16_to_cpu(c->tx_aborted);
+	data[12] = le16_to_cpu(c->tx_underun);
 }
 
 static void rtl8169_get_strings(struct net_device *dev, u32 stringset, u8 *data)
@@ -7779,6 +7782,8 @@ static struct rtnl_link_stats64 *
 rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
+	struct rtl8169_tc_offsets *offset = &tp->tc_offset;
+	struct rtl8169_counters *counters = tp->counters;
 	void __iomem *ioaddr = tp->mmio_addr;
 	unsigned int start;
 
@@ -7816,12 +7821,12 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 	 * Subtract values fetched during initalization.
 	 * See rtl8169_init_counter_offsets for a description why we do that.
 	 */
-	stats->tx_errors = le64_to_cpu(tp->counters.tx_errors) -
-		le64_to_cpu(tp->tc_offset.tx_errors);
-	stats->collisions = le32_to_cpu(tp->counters.tx_multi_collision) -
-		le32_to_cpu(tp->tc_offset.tx_multi_collision);
-	stats->tx_aborted_errors = le16_to_cpu(tp->counters.tx_aborted) -
-		le16_to_cpu(tp->tc_offset.tx_aborted);
+	stats->tx_errors = le64_to_cpu(counters->tx_errors) -
+			   le64_to_cpu(offset->tx_errors);
+	stats->collisions = le32_to_cpu(counters->tx_multi_collision) -
+			    le32_to_cpu(offset->tx_multi_collision);
+	stats->tx_aborted_errors = le16_to_cpu(counters->tx_aborted) -
+				   le16_to_cpu(offset->tx_aborted);
 
 	return stats;
 }
@@ -8022,6 +8027,8 @@ static void rtl_remove_one(struct pci_dev *pdev)
 
 	unregister_netdev(dev);
 
+	kfree(tp->counters);
+
 	rtl_release_firmware(tp);
 
 	if (pci_dev_run_wake(pdev))
@@ -8447,9 +8454,15 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	tp->rtl_fw = RTL_FIRMWARE_UNKNOWN;
 
+	tp->counters = kmalloc(sizeof(*tp->counters), GFP_KERNEL);
+	if (!tp->counters) {
+		rc = -ENOMEM;
+		goto err_out_msi_4;
+	}
+
 	rc = register_netdev(dev);
 	if (rc < 0)
-		goto err_out_msi_4;
+		goto err_out_counters_5;
 
 	pci_set_drvdata(pdev, dev);
 
@@ -8483,6 +8496,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 out:
 	return rc;
 
+err_out_counters_5:
+	kfree(tp->counters);
 err_out_msi_4:
 	netif_napi_del(&tp->napi);
 	rtl_disable_msi(pdev, tp);
-- 
2.4.3


[-- Attachment #3: 0002-r8169-use-a-single-condition-to-check-the-completion.patch --]
[-- Type: text/plain, Size: 2762 bytes --]

>From d7467f45553b5dca2c6ef094c49e15b2385e3659 Mon Sep 17 00:00:00 2001
Message-Id: <d7467f45553b5dca2c6ef094c49e15b2385e3659.1441670158.git.romieu@fr.zoreil.com>
In-Reply-To: <8f1b87a115f6c340558b95f6a1748ddbf866d95f.1441670158.git.romieu@fr.zoreil.com>
References: <8f1b87a115f6c340558b95f6a1748ddbf866d95f.1441670158.git.romieu@fr.zoreil.com>
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 5 Sep 2015 13:30:41 +0200
Subject: [PATCH 2/3] r8169: use a single condition to check the completion of
 counters commands.
X-Organisation: Land of Sunshine Inc.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/realtek/r8169.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index ff1b834..f8a81a9 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -2190,6 +2190,13 @@ static int rtl8169_get_sset_count(struct net_device *dev, int sset)
 	}
 }
 
+DECLARE_RTL_COND(rtl_cmd_counters_cond)
+{
+	void __iomem *ioaddr = tp->mmio_addr;
+
+	return RTL_R32(CounterAddrLow) & (CounterReset | CounterDump);
+}
+
 static struct rtl8169_counters *rtl8169_map_counters(struct net_device *dev,
 						     dma_addr_t *paddr,
 						     u32 counter_cmd)
@@ -2224,13 +2231,6 @@ static void rtl8169_unmap_counters (struct net_device *dev,
 	dma_free_coherent(d, sizeof(*counters), counters, paddr);
 }
 
-DECLARE_RTL_COND(rtl_reset_counters_cond)
-{
-	void __iomem *ioaddr = tp->mmio_addr;
-
-	return RTL_R32(CounterAddrLow) & CounterReset;
-}
-
 static bool rtl8169_reset_counters(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
@@ -2249,7 +2249,7 @@ static bool rtl8169_reset_counters(struct net_device *dev)
 	if (!counters)
 		return false;
 
-	if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000))
+	if (!rtl_udelay_loop_wait_low(tp, &rtl_cmd_counters_cond, 10, 1000))
 		ret = false;
 
 	rtl8169_unmap_counters(dev, paddr, counters);
@@ -2257,13 +2257,6 @@ static bool rtl8169_reset_counters(struct net_device *dev)
 	return ret;
 }
 
-DECLARE_RTL_COND(rtl_counters_cond)
-{
-	void __iomem *ioaddr = tp->mmio_addr;
-
-	return RTL_R32(CounterAddrLow) & CounterDump;
-}
-
 static bool rtl8169_update_counters(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
@@ -2283,7 +2276,7 @@ static bool rtl8169_update_counters(struct net_device *dev)
 	if (!counters)
 		return false;
 
-	if (rtl_udelay_loop_wait_low(tp, &rtl_counters_cond, 10, 1000))
+	if (rtl_udelay_loop_wait_low(tp, &rtl_cmd_counters_cond, 10, 1000))
 		memcpy(tp->counters, counters, sizeof(*counters));
 	else
 		ret = false;
-- 
2.4.3


[-- Attachment #4: 0003-r8169-increase-the-lifespan-of-the-hardware-counters.patch --]
[-- Type: text/plain, Size: 7985 bytes --]

>From 2f02fd78d644e076bc11b24e4f16c89883aa8a24 Mon Sep 17 00:00:00 2001
Message-Id: <2f02fd78d644e076bc11b24e4f16c89883aa8a24.1441670158.git.romieu@fr.zoreil.com>
In-Reply-To: <8f1b87a115f6c340558b95f6a1748ddbf866d95f.1441670158.git.romieu@fr.zoreil.com>
References: <8f1b87a115f6c340558b95f6a1748ddbf866d95f.1441670158.git.romieu@fr.zoreil.com>
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Tue, 8 Sep 2015 00:11:58 +0200
Subject: [PATCH 3/3] r8169: increase the lifespan of the hardware counters
 dump area.
X-Organisation: Land of Sunshine Inc.

It avoids sleepable allocation when retrieving statistics as they can
happen with spinlock held (see net/core/net-sysfs.c::netstat_show).

- 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.

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>
---
 drivers/net/ethernet/realtek/r8169.c | 112 +++++++++++++----------------------
 1 file changed, 42 insertions(+), 70 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f8a81a9..afe7810 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,104 +2200,65 @@ DECLARE_RTL_COND(rtl_cmd_counters_cond)
 	return RTL_R32(CounterAddrLow) & (CounterReset | CounterDump);
 }
 
-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;
+	int rc;
 
-	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;
+	rc = rtl_udelay_loop_wait_low(tp, &rtl_cmd_counters_cond, 10, 1000);
 
 	RTL_W32(CounterAddrLow, 0);
 	RTL_W32(CounterAddrHigh, 0);
 
-	dma_free_coherent(d, sizeof(*counters), counters, paddr);
+	return rc;
 }
 
-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_cmd_counters_cond, 10, 1000))
-		ret = false;
-
-	rtl8169_unmap_counters(dev, paddr, counters);
+		return -EINVAL;
 
-	return ret;
+	return rtl8169_cmd_counters(dev, CounterReset);
 }
 
-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_cmd_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.
@@ -2303,27 +2267,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,
@@ -7741,7 +7707,8 @@ static int rtl_open(struct net_device *dev)
 
 	rtl_hw_start(dev);
 
-	if (!rtl8169_init_counter_offsets(dev))
+	retval = rtl_init_counter_offsets(dev);
+	if (retval < 0)
 		netif_warn(tp, hw, dev, "counter reset/update failed\n");
 
 	netif_start_queue(dev);
@@ -8020,8 +7987,6 @@ static void rtl_remove_one(struct pci_dev *pdev)
 
 	unregister_netdev(dev);
 
-	kfree(tp->counters);
-
 	rtl_release_firmware(tp);
 
 	if (pci_dev_run_wake(pdev))
@@ -8031,6 +7996,10 @@ static void rtl_remove_one(struct pci_dev *pdev)
 	rtl_rar_set(tp, dev->perm_addr);
 
 	rtl_disable_msi(pdev, tp);
+
+	dma_free_coherent(&pdev->dev, sizeof(*tp->counters), tp->counters,
+			  tp->counters_map);
+
 	rtl8169_release_board(pdev, dev, tp->mmio_addr);
 }
 
@@ -8447,7 +8416,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	tp->rtl_fw = RTL_FIRMWARE_UNKNOWN;
 
-	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) {
 		rc = -ENOMEM;
 		goto err_out_msi_4;
@@ -8490,7 +8460,9 @@ out:
 	return rc;
 
 err_out_counters_5:
-	kfree(tp->counters);
+	dma_free_coherent(&pdev->dev, sizeof(*tp->counters), tp->counters,
+			  tp->counters_map);
+	tp->counters = NULL;
 err_out_msi_4:
 	netif_napi_del(&tp->napi);
 	rtl_disable_msi(pdev, tp);
-- 
2.4.3

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

On Sep  7 23:52, Francois Romieu wrote:
> Corinna Vinschen <vinschen@redhat.com> :
> [...]
> > I have a bit of a problem with this patch.  It crashes when loading the
> > driver manually w/ modprobe.  For some reason dev_get_stats is called
> > during rtl_init_one and at that time the counters pointer is NULL, so
> > the kernel gets a SEGV.
> >
> >  After I worked around that I got a SEGV in
> > rtl_remove_one, which I still have to find out why.  I didn't test with
> > the latest kernel, though, so I still have to check if that's the reason
> > for the crashes.  That takes a bit longer, I just wanted to let you
> > know.
> 
> Call me stupid: I forgot that it's perfectly fine to request the stats
> of a down interface. :o/
> 
> Updated patch is on the way.
> 
> > There's also a problem with rtl8169_cmd_counters:  It always calls
> > rtl_udelay_loop_wait_low w/ rtl_reset_counters_cond, even when called
> > from rtl8169_update_counters, where it should call
> > rtl_udelay_loop_wait_low w/ rtl_counters_cond to check for the
> > CounterDump flag, rather than for the CounterReset flag.
> > 
> > For now I applied the below patch, but I think it's a bit ugly to
> > conditionalize the rtl_cond struct by the incoming flag value.
> 
> <captain obvious>
> 
> Let's check both at once:
> 
> 	return RTL_R32(CounterAddrLow) & (CounterDump | CounterReset);

If you're sure that's valid, then why not?  It's certainly cleaner
code.


Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

On Sep  8 02:02, Francois Romieu wrote:
> Francois Romieu <romieu@fr.zoreil.com> :
> [...]
> > Updated patch is on the way.
> 
> Fixed memcpy in patch 0001, moved counters allocation from open() 
> to probe(), returned open() to its original state but something is
> still wrong: the link does not come up.

I tested and debugged the attached patches.  Just as you noticed, the
interfaces (my test machine has two) don't come up at boot time and
subsequently I can also reproduce two kinds of crashes:

- Calling `ip link ... up' crashes the kernel in rtl_open like this:

[  138.031190]  [<ffffffff81670f92>] dump_stack+0x44/0x55
[  138.036311]  [<ffffffff810d92d5>] __setup_irq+0x515/0x580
[  138.041693]  [<ffffffffa006cf00>] ? rtl8169_gset_xmii+0x20/0x20 [r8169]
[  138.048284]  [<ffffffff810d94c4>] request_threaded_irq+0xf4/0x1a0
[  138.054357]  [<ffffffffa0075cf7>] rtl_open+0x3a7/0xab4 [r8169]
[...]

- Alternatively I can still reproduce the SEGV in rtl_remove_one
  when trying to rmmod the module, I just don't have the stack dump
  handy while writing this mail.  I can show it if needed.

I debugged this on and off the entire day (tweaking, compiling, rebooting,
kernel crash, rinse and repeat).

And the result of my debugging is totally crazy:

If I disable the call to rtl_init_counter_offsets in rtl_open, as in

  #if 0
        retval = rtl_init_counter_offsets(dev);
	if (retval < 0)
		netif_warn(tp, hw, dev, "counter reset/update failed\n");
  #endif

the interfaces come up just fine.

If I reenable the rtl_init_counter_offsets call in rtl_open, and reduce
the rtl_init_counter_offsets function to just this:

  static int rtl_init_counter_offsets(struct net_device *dev)
  {
	  return 1;
  }

then the interfaces refuse to come up, and a subsequent `ip link ... up'
crashes the kernel.

No, I do not understand this :(


Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Francois Romieu]

Corinna Vinschen <vinschen@redhat.com> :
[...]
> - Alternatively I can still reproduce the SEGV in rtl_remove_one
>   when trying to rmmod the module, I just don't have the stack dump
>   handy while writing this mail.  I can show it if needed.

I see it too. 

> I debugged this on and off the entire day (tweaking, compiling, rebooting,
> kernel crash, rinse and repeat).
> 
> And the result of my debugging is totally crazy:

My patch corrupts memory.

-- 
Ueimor

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

On Sep  9 01:27, Francois Romieu wrote:
> Corinna Vinschen <vinschen@redhat.com> :
> [...]
> > - Alternatively I can still reproduce the SEGV in rtl_remove_one
> >   when trying to rmmod the module, I just don't have the stack dump
> >   handy while writing this mail.  I can show it if needed.
> 
> I see it too. 
> 
> > I debugged this on and off the entire day (tweaking, compiling, rebooting,
> > kernel crash, rinse and repeat).
> > 
> > And the result of my debugging is totally crazy:
> 
> My patch corrupts memory.

That's rather obvious, I just fail to see how.

I try to implement this from scratch, maybe I stumble over the cause
by accident...


Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]