[PATCH net-next] net: dlink: add synchronization for stats update

[PATCH net-next] net: dlink: add synchronization for stats update

[Moon Yeounsu]

There are two paths that call `get_stats()`:
    1. From user space via the `ip` command
    2. From interrupt context via `rio_interrupt()`

Case 1 is synchronized by `rtnl_lock()`, so it is safe.
However, the two cases above are not synchronized with each other.
Therefore, `spin_lock` is needed to protect `get_stats()` as it can be
preempted by an interrupt. In this context, `spin_lock_irq()` is required
(using `spin_lock_bh()` may result in a deadlock).

While `spin_lock` protects `get_stats()`, it does not protect updates to
`dev->stats.tx_errors` and `dev->stats.collisions`, which may be
concurrently modified by the interrupt handler and user space.
By using temporary variables in `np->tx_errors` and `np->collisions`,
we can safely update `dev->stats` without additional locking.

Tested-on: D-Link DGE-550T Rev-A3
Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
---
Question:
	This might be a bit off-topic, but I don’t fully understand why a single global
	`rtnl_lock` is used for synchronization. While I may not be fully aware of the 
	design rationale, it seems somewhat suboptimal. I believe it could be improved.
---
 drivers/net/ethernet/dlink/dl2k.c | 11 +++++++++--
 drivers/net/ethernet/dlink/dl2k.h |  5 +++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
index d0ea92607870..2d929a83e101 100644
--- a/drivers/net/ethernet/dlink/dl2k.c
+++ b/drivers/net/ethernet/dlink/dl2k.c
@@ -865,7 +865,7 @@ tx_error (struct net_device *dev, int tx_status)
 	frame_id = (tx_status & 0xffff0000);
 	printk (KERN_ERR "%s: Transmit error, TxStatus %4.4x, FrameId %d.\n",
 		dev->name, tx_status, frame_id);
-	dev->stats.tx_errors++;
+	np->tx_errors++;
 	/* Ttransmit Underrun */
 	if (tx_status & 0x10) {
 		dev->stats.tx_fifo_errors++;
@@ -904,7 +904,7 @@ tx_error (struct net_device *dev, int tx_status)
 	}
 	/* Maximum Collisions */
 	if (tx_status & 0x08)
-		dev->stats.collisions++;
+		np->collisions++;
 	/* Restart the Tx */
 	dw32(MACCtrl, dr16(MACCtrl) | TxEnable);
 }
@@ -1074,6 +1074,7 @@ get_stats (struct net_device *dev)
 #endif
 	unsigned int stat_reg;
 
+	spin_lock_irq(&np->stats_lock);
 	/* All statistics registers need to be acknowledged,
 	   else statistic overflow could cause problems */
 
@@ -1085,6 +1086,7 @@ get_stats (struct net_device *dev)
 	dev->stats.multicast = dr32(McstFramesRcvdOk);
 	dev->stats.collisions += dr32(SingleColFrames)
 			     +  dr32(MultiColFrames);
+	dev->stats.collisions += np->collisions;
 
 	/* detailed tx errors */
 	stat_reg = dr16(FramesAbortXSColls);
@@ -1095,6 +1097,8 @@ get_stats (struct net_device *dev)
 	dev->stats.tx_carrier_errors += stat_reg;
 	dev->stats.tx_errors += stat_reg;
 
+	dev->stats.tx_errors += np->tx_errors;
+
 	/* Clear all other statistic register. */
 	dr32(McstOctetXmtOk);
 	dr16(BcstFramesXmtdOk);
@@ -1123,6 +1127,9 @@ get_stats (struct net_device *dev)
 	dr16(TCPCheckSumErrors);
 	dr16(UDPCheckSumErrors);
 	dr16(IPCheckSumErrors);
+
+	spin_unlock_irq(&np->stats_lock);
+
 	return &dev->stats;
 }
 
diff --git a/drivers/net/ethernet/dlink/dl2k.h b/drivers/net/ethernet/dlink/dl2k.h
index 195dc6cfd895..dc8755a69b73 100644
--- a/drivers/net/ethernet/dlink/dl2k.h
+++ b/drivers/net/ethernet/dlink/dl2k.h
@@ -372,6 +372,8 @@ struct netdev_private {
 	struct pci_dev *pdev;
 	void __iomem *ioaddr;
 	void __iomem *eeprom_addr;
+	// To ensure synchronization when stats are updated.
+	spinlock_t stats_lock;
 	spinlock_t tx_lock;
 	spinlock_t rx_lock;
 	unsigned int rx_buf_sz;		/* Based on MTU+slack. */
@@ -401,6 +403,9 @@ struct netdev_private {
 	u16 negotiate;		/* Negotiated media */
 	int phy_addr;		/* PHY addresses. */
 	u16 led_mode;		/* LED mode read from EEPROM (IP1000A only) */
+
+	u64 collisions;
+	u64 tx_errors;
 };
 
 /* The station address location in the EEPROM. */
-- 
2.49.0

Re: [PATCH net-next] net: dlink: add synchronization for stats update

[Jacob Keller]

On 4/21/2025 12:16 PM, Moon Yeounsu wrote:
> There are two paths that call `get_stats()`:
>     1. From user space via the `ip` command
>     2. From interrupt context via `rio_interrupt()`
> 
> Case 1 is synchronized by `rtnl_lock()`, so it is safe.
> However, the two cases above are not synchronized with each other.
> Therefore, `spin_lock` is needed to protect `get_stats()` as it can be
> preempted by an interrupt. In this context, `spin_lock_irq()` is required
> (using `spin_lock_bh()` may result in a deadlock).
> 
> While `spin_lock` protects `get_stats()`, it does not protect updates to
> `dev->stats.tx_errors` and `dev->stats.collisions`, which may be
> concurrently modified by the interrupt handler and user space.
> By using temporary variables in `np->tx_errors` and `np->collisions`,
> we can safely update `dev->stats` without additional locking.
> 
> Tested-on: D-Link DGE-550T Rev-A3
> Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com>
> ---
> Question:
> 	This might be a bit off-topic, but I don’t fully understand why a single global
> 	`rtnl_lock` is used for synchronization. While I may not be fully aware of the 
> 	design rationale, it seems somewhat suboptimal. I believe it could be improved.

Its been a long standing effort to reduce the use of RTNL lock, with a
lot of effort going into switching calls to use the per-netdev lock instead.

I think you could switch the driver over to the ops locked method by
setting the relevant flag in netdev to avoid global lock contention,
since 605ef7aec060 ("net: add option to request netdev instance lock")
which is coming in v6.15

Thanks,
Jake

> ---
>  drivers/net/ethernet/dlink/dl2k.c | 11 +++++++++--
>  drivers/net/ethernet/dlink/dl2k.h |  5 +++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
> index d0ea92607870..2d929a83e101 100644
> --- a/drivers/net/ethernet/dlink/dl2k.c
> +++ b/drivers/net/ethernet/dlink/dl2k.c
> @@ -865,7 +865,7 @@ tx_error (struct net_device *dev, int tx_status)
>  	frame_id = (tx_status & 0xffff0000);
>  	printk (KERN_ERR "%s: Transmit error, TxStatus %4.4x, FrameId %d.\n",
>  		dev->name, tx_status, frame_id);
> -	dev->stats.tx_errors++;
> +	np->tx_errors++;
>  	/* Ttransmit Underrun */
>  	if (tx_status & 0x10) {
>  		dev->stats.tx_fifo_errors++;
> @@ -904,7 +904,7 @@ tx_error (struct net_device *dev, int tx_status)
>  	}
>  	/* Maximum Collisions */
>  	if (tx_status & 0x08)
> -		dev->stats.collisions++;
> +		np->collisions++;
>  	/* Restart the Tx */
>  	dw32(MACCtrl, dr16(MACCtrl) | TxEnable);
>  }
> @@ -1074,6 +1074,7 @@ get_stats (struct net_device *dev)
>  #endif
>  	unsigned int stat_reg;
>  
> +	spin_lock_irq(&np->stats_lock);
>  	/* All statistics registers need to be acknowledged,
>  	   else statistic overflow could cause problems */
>  
> @@ -1085,6 +1086,7 @@ get_stats (struct net_device *dev)
>  	dev->stats.multicast = dr32(McstFramesRcvdOk);
>  	dev->stats.collisions += dr32(SingleColFrames)
>  			     +  dr32(MultiColFrames);
> +	dev->stats.collisions += np->collisions;
>  
>  	/* detailed tx errors */
>  	stat_reg = dr16(FramesAbortXSColls);
> @@ -1095,6 +1097,8 @@ get_stats (struct net_device *dev)
>  	dev->stats.tx_carrier_errors += stat_reg;
>  	dev->stats.tx_errors += stat_reg;
>  
> +	dev->stats.tx_errors += np->tx_errors;
> +
>  	/* Clear all other statistic register. */
>  	dr32(McstOctetXmtOk);
>  	dr16(BcstFramesXmtdOk);
> @@ -1123,6 +1127,9 @@ get_stats (struct net_device *dev)
>  	dr16(TCPCheckSumErrors);
>  	dr16(UDPCheckSumErrors);
>  	dr16(IPCheckSumErrors);
> +
> +	spin_unlock_irq(&np->stats_lock);
> +
>  	return &dev->stats;
>  }
>  
> diff --git a/drivers/net/ethernet/dlink/dl2k.h b/drivers/net/ethernet/dlink/dl2k.h
> index 195dc6cfd895..dc8755a69b73 100644
> --- a/drivers/net/ethernet/dlink/dl2k.h
> +++ b/drivers/net/ethernet/dlink/dl2k.h
> @@ -372,6 +372,8 @@ struct netdev_private {
>  	struct pci_dev *pdev;
>  	void __iomem *ioaddr;
>  	void __iomem *eeprom_addr;
> +	// To ensure synchronization when stats are updated.
> +	spinlock_t stats_lock;
>  	spinlock_t tx_lock;
>  	spinlock_t rx_lock;
>  	unsigned int rx_buf_sz;		/* Based on MTU+slack. */
> @@ -401,6 +403,9 @@ struct netdev_private {
>  	u16 negotiate;		/* Negotiated media */
>  	int phy_addr;		/* PHY addresses. */
>  	u16 led_mode;		/* LED mode read from EEPROM (IP1000A only) */
> +
> +	u64 collisions;
> +	u64 tx_errors;
>  };

Code changes seem fine to me.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  
>  /* The station address location in the EEPROM. */

Re: [PATCH net-next] net: dlink: add synchronization for stats update

[Moon Yeounsu]

On Tue, Apr 22, 2025 at 8:24 AM Jacob Keller <jacob.e.keller@intel.com> wrote:
> Thanks,
> Jake

Thank you for replying to the patch!
I think this patch needs a bit more refinement.
Would it be okay if I submit a v2 patch?

Re: [PATCH net-next] net: dlink: add synchronization for stats update

[Jacob Keller]

On 4/22/2025 3:53 PM, Moon Yeounsu wrote:
> On Tue, Apr 22, 2025 at 8:24 AM Jacob Keller <jacob.e.keller@intel.com> wrote:
>> Thanks,
>> Jake
> 
> Thank you for replying to the patch!
> I think this patch needs a bit more refinement.
> Would it be okay if I submit a v2 patch?

I would probably switch to request_ops_lock as a separate change than
this synchronization fix/improvement.

Re: [PATCH net-next] net: dlink: add synchronization for stats update

[Jakub Kicinski]

On Tue, 22 Apr 2025 04:16:44 +0900 Moon Yeounsu wrote:
> -	dev->stats.tx_errors++;
> +	np->tx_errors++;
>  	/* Ttransmit Underrun */
>  	if (tx_status & 0x10) {
>  		dev->stats.tx_fifo_errors++;
> @@ -904,7 +904,7 @@ tx_error (struct net_device *dev, int tx_status)
>  	}
>  	/* Maximum Collisions */
>  	if (tx_status & 0x08)
> -		dev->stats.collisions++;
> +		np->collisions++;

These can be updated concurrently with the reading.
Since they are 64b on 32b machines the update may not be atomic.
So to be safe please take the spin lock around the increments,
or you could convert them to a atomic_t, or you can make them 32 bits
and update them with WRITE_ONCE() read with READ_ONCE()..
-- 
pw-bot: cr