Linux-RISC-V Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] net: spacemit: Make stats_lock softirq-safe
@ 2025-09-19 12:04 ` Vivian Wang
  2025-09-19 12:25   ` Marek Szyprowski
  2025-09-22 19:00   ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 3+ messages in thread
From: Vivian Wang @ 2025-09-19 12:04 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Yixun Lan, Maxime Chevallier, Troy Mitchell,
	Vadim Fedorenko, Vivian Wang
  Cc: netdev, linux-riscv, spacemit, linux-kernel, Vivian Wang,
	Marek Szyprowski

While most of the statistics functions (emac_get_stats64() and such) are
called with softirqs enabled, emac_stats_timer() is, as its name
suggests, also called from a timer, i.e. called in softirq context.

All of these take stats_lock. Therefore, make stats_lock softirq-safe by
changing spin_lock() into spin_lock_bh() for the functions that get
statistics.

Also, instead of directly calling emac_stats_timer() in emac_up() and
emac_resume(), set the timer to trigger instead, so that
emac_stats_timer() is only called from the timer. It will keep using
spin_lock().

This fixes a lockdep warning, and potential deadlock when stats_timer is
triggered in the middle of getting statistics.

Fixes: bfec6d7f2001 ("net: spacemit: Add K1 Ethernet MAC")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Closes: https://lore.kernel.org/all/a52c0cf5-0444-41aa-b061-a0a1d72b02fe@samsung.com/
Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
---
Thanks a lot for catching this, Marek!
---
 drivers/net/ethernet/spacemit/k1_emac.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
index 928fea02198c3754f63a7b33fc25c5dd8c2b59f9..e1c5faff3b71c7d4ceba2ea194d9c888f0e71b70 100644
--- a/drivers/net/ethernet/spacemit/k1_emac.c
+++ b/drivers/net/ethernet/spacemit/k1_emac.c
@@ -135,7 +135,7 @@ struct emac_priv {
 	bool flow_control_autoneg;
 	u8 flow_control;
 
-	/* Hold while touching hardware statistics */
+	/* Softirq-safe, hold while touching hardware statistics */
 	spinlock_t stats_lock;
 };
 
@@ -1239,7 +1239,7 @@ static void emac_get_stats64(struct net_device *dev,
 	/* This is the only software counter */
 	storage->tx_dropped = emac_get_stat_tx_drops(priv);
 
-	spin_lock(&priv->stats_lock);
+	spin_lock_bh(&priv->stats_lock);
 
 	emac_stats_update(priv);
 
@@ -1261,7 +1261,7 @@ static void emac_get_stats64(struct net_device *dev,
 	storage->rx_missed_errors = rx_stats->stats.rx_drp_fifo_full_pkts;
 	storage->rx_missed_errors += rx_stats->stats.rx_truncate_fifo_full_pkts;
 
-	spin_unlock(&priv->stats_lock);
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 static void emac_get_rmon_stats(struct net_device *dev,
@@ -1275,7 +1275,7 @@ static void emac_get_rmon_stats(struct net_device *dev,
 
 	*ranges = emac_rmon_hist_ranges;
 
-	spin_lock(&priv->stats_lock);
+	spin_lock_bh(&priv->stats_lock);
 
 	emac_stats_update(priv);
 
@@ -1294,7 +1294,7 @@ static void emac_get_rmon_stats(struct net_device *dev,
 	rmon_stats->hist[5] = rx_stats->stats.rx_1024_1518_pkts;
 	rmon_stats->hist[6] = rx_stats->stats.rx_1519_plus_pkts;
 
-	spin_unlock(&priv->stats_lock);
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 static void emac_get_eth_mac_stats(struct net_device *dev,
@@ -1307,7 +1307,7 @@ static void emac_get_eth_mac_stats(struct net_device *dev,
 	tx_stats = &priv->tx_stats;
 	rx_stats = &priv->rx_stats;
 
-	spin_lock(&priv->stats_lock);
+	spin_lock_bh(&priv->stats_lock);
 
 	emac_stats_update(priv);
 
@@ -1325,7 +1325,7 @@ static void emac_get_eth_mac_stats(struct net_device *dev,
 	mac_stats->FramesAbortedDueToXSColls =
 		tx_stats->stats.tx_excessclsn_pkts;
 
-	spin_unlock(&priv->stats_lock);
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 static void emac_get_pause_stats(struct net_device *dev,
@@ -1338,14 +1338,14 @@ static void emac_get_pause_stats(struct net_device *dev,
 	tx_stats = &priv->tx_stats;
 	rx_stats = &priv->rx_stats;
 
-	spin_lock(&priv->stats_lock);
+	spin_lock_bh(&priv->stats_lock);
 
 	emac_stats_update(priv);
 
 	pause_stats->tx_pause_frames = tx_stats->stats.tx_pause_pkts;
 	pause_stats->rx_pause_frames = rx_stats->stats.rx_pause_pkts;
 
-	spin_unlock(&priv->stats_lock);
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 /* Other statistics that are not derivable from standard statistics */
@@ -1393,14 +1393,14 @@ static void emac_get_ethtool_stats(struct net_device *dev,
 	u64 *rx_stats = (u64 *)&priv->rx_stats;
 	int i;
 
-	spin_lock(&priv->stats_lock);
+	spin_lock_bh(&priv->stats_lock);
 
 	emac_stats_update(priv);
 
 	for (i = 0; i < ARRAY_SIZE(emac_ethtool_rx_stats); i++)
 		data[i] = rx_stats[emac_ethtool_rx_stats[i].offset];
 
-	spin_unlock(&priv->stats_lock);
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 static int emac_ethtool_get_regs_len(struct net_device *dev)
@@ -1769,7 +1769,7 @@ static int emac_up(struct emac_priv *priv)
 
 	netif_start_queue(ndev);
 
-	emac_stats_timer(&priv->stats_timer);
+	mod_timer(&priv->stats_timer, jiffies);
 
 	return 0;
 
@@ -1807,14 +1807,14 @@ static int emac_down(struct emac_priv *priv)
 
 	/* Update and save current stats, see emac_stats_update() for usage */
 
-	spin_lock(&priv->stats_lock);
+	spin_lock_bh(&priv->stats_lock);
 
 	emac_stats_update(priv);
 
 	priv->tx_stats_off = priv->tx_stats;
 	priv->rx_stats_off = priv->rx_stats;
 
-	spin_unlock(&priv->stats_lock);
+	spin_unlock_bh(&priv->stats_lock);
 
 	pm_runtime_put_sync(&pdev->dev);
 	return 0;
@@ -2111,7 +2111,7 @@ static int emac_resume(struct device *dev)
 
 	netif_device_attach(ndev);
 
-	emac_stats_timer(&priv->stats_timer);
+	mod_timer(&priv->stats_timer, jiffies);
 
 	return 0;
 }

---
base-commit: 315f423be0d1ebe720d8fd4fa6bed68586b13d34
change-id: 20250919-k1-ethernet-fix-lock-c99681a9aa5d

Best regards,
-- 
Vivian "dramforever" Wang


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH net-next] net: spacemit: Make stats_lock softirq-safe
  2025-09-19 12:04 ` [PATCH net-next] net: spacemit: Make stats_lock softirq-safe Vivian Wang
@ 2025-09-19 12:25   ` Marek Szyprowski
  2025-09-22 19:00   ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 3+ messages in thread
From: Marek Szyprowski @ 2025-09-19 12:25 UTC (permalink / raw)
  To: Vivian Wang, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yixun Lan, Maxime Chevallier,
	Troy Mitchell, Vadim Fedorenko
  Cc: netdev, linux-riscv, spacemit, linux-kernel, Vivian Wang

On 19.09.2025 14:04, Vivian Wang wrote:
> While most of the statistics functions (emac_get_stats64() and such) are
> called with softirqs enabled, emac_stats_timer() is, as its name
> suggests, also called from a timer, i.e. called in softirq context.
>
> All of these take stats_lock. Therefore, make stats_lock softirq-safe by
> changing spin_lock() into spin_lock_bh() for the functions that get
> statistics.
>
> Also, instead of directly calling emac_stats_timer() in emac_up() and
> emac_resume(), set the timer to trigger instead, so that
> emac_stats_timer() is only called from the timer. It will keep using
> spin_lock().
>
> This fixes a lockdep warning, and potential deadlock when stats_timer is
> triggered in the middle of getting statistics.
>
> Fixes: bfec6d7f2001 ("net: spacemit: Add K1 Ethernet MAC")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Closes: https://lore.kernel.org/all/a52c0cf5-0444-41aa-b061-a0a1d72b02fe@samsung.com/
> Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Thanks a lot for catching this, Marek!
> ---
>   drivers/net/ethernet/spacemit/k1_emac.c | 30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
> index 928fea02198c3754f63a7b33fc25c5dd8c2b59f9..e1c5faff3b71c7d4ceba2ea194d9c888f0e71b70 100644
> --- a/drivers/net/ethernet/spacemit/k1_emac.c
> +++ b/drivers/net/ethernet/spacemit/k1_emac.c
> @@ -135,7 +135,7 @@ struct emac_priv {
>   	bool flow_control_autoneg;
>   	u8 flow_control;
>   
> -	/* Hold while touching hardware statistics */
> +	/* Softirq-safe, hold while touching hardware statistics */
>   	spinlock_t stats_lock;
>   };
>   
> @@ -1239,7 +1239,7 @@ static void emac_get_stats64(struct net_device *dev,
>   	/* This is the only software counter */
>   	storage->tx_dropped = emac_get_stat_tx_drops(priv);
>   
> -	spin_lock(&priv->stats_lock);
> +	spin_lock_bh(&priv->stats_lock);
>   
>   	emac_stats_update(priv);
>   
> @@ -1261,7 +1261,7 @@ static void emac_get_stats64(struct net_device *dev,
>   	storage->rx_missed_errors = rx_stats->stats.rx_drp_fifo_full_pkts;
>   	storage->rx_missed_errors += rx_stats->stats.rx_truncate_fifo_full_pkts;
>   
> -	spin_unlock(&priv->stats_lock);
> +	spin_unlock_bh(&priv->stats_lock);
>   }
>   
>   static void emac_get_rmon_stats(struct net_device *dev,
> @@ -1275,7 +1275,7 @@ static void emac_get_rmon_stats(struct net_device *dev,
>   
>   	*ranges = emac_rmon_hist_ranges;
>   
> -	spin_lock(&priv->stats_lock);
> +	spin_lock_bh(&priv->stats_lock);
>   
>   	emac_stats_update(priv);
>   
> @@ -1294,7 +1294,7 @@ static void emac_get_rmon_stats(struct net_device *dev,
>   	rmon_stats->hist[5] = rx_stats->stats.rx_1024_1518_pkts;
>   	rmon_stats->hist[6] = rx_stats->stats.rx_1519_plus_pkts;
>   
> -	spin_unlock(&priv->stats_lock);
> +	spin_unlock_bh(&priv->stats_lock);
>   }
>   
>   static void emac_get_eth_mac_stats(struct net_device *dev,
> @@ -1307,7 +1307,7 @@ static void emac_get_eth_mac_stats(struct net_device *dev,
>   	tx_stats = &priv->tx_stats;
>   	rx_stats = &priv->rx_stats;
>   
> -	spin_lock(&priv->stats_lock);
> +	spin_lock_bh(&priv->stats_lock);
>   
>   	emac_stats_update(priv);
>   
> @@ -1325,7 +1325,7 @@ static void emac_get_eth_mac_stats(struct net_device *dev,
>   	mac_stats->FramesAbortedDueToXSColls =
>   		tx_stats->stats.tx_excessclsn_pkts;
>   
> -	spin_unlock(&priv->stats_lock);
> +	spin_unlock_bh(&priv->stats_lock);
>   }
>   
>   static void emac_get_pause_stats(struct net_device *dev,
> @@ -1338,14 +1338,14 @@ static void emac_get_pause_stats(struct net_device *dev,
>   	tx_stats = &priv->tx_stats;
>   	rx_stats = &priv->rx_stats;
>   
> -	spin_lock(&priv->stats_lock);
> +	spin_lock_bh(&priv->stats_lock);
>   
>   	emac_stats_update(priv);
>   
>   	pause_stats->tx_pause_frames = tx_stats->stats.tx_pause_pkts;
>   	pause_stats->rx_pause_frames = rx_stats->stats.rx_pause_pkts;
>   
> -	spin_unlock(&priv->stats_lock);
> +	spin_unlock_bh(&priv->stats_lock);
>   }
>   
>   /* Other statistics that are not derivable from standard statistics */
> @@ -1393,14 +1393,14 @@ static void emac_get_ethtool_stats(struct net_device *dev,
>   	u64 *rx_stats = (u64 *)&priv->rx_stats;
>   	int i;
>   
> -	spin_lock(&priv->stats_lock);
> +	spin_lock_bh(&priv->stats_lock);
>   
>   	emac_stats_update(priv);
>   
>   	for (i = 0; i < ARRAY_SIZE(emac_ethtool_rx_stats); i++)
>   		data[i] = rx_stats[emac_ethtool_rx_stats[i].offset];
>   
> -	spin_unlock(&priv->stats_lock);
> +	spin_unlock_bh(&priv->stats_lock);
>   }
>   
>   static int emac_ethtool_get_regs_len(struct net_device *dev)
> @@ -1769,7 +1769,7 @@ static int emac_up(struct emac_priv *priv)
>   
>   	netif_start_queue(ndev);
>   
> -	emac_stats_timer(&priv->stats_timer);
> +	mod_timer(&priv->stats_timer, jiffies);
>   
>   	return 0;
>   
> @@ -1807,14 +1807,14 @@ static int emac_down(struct emac_priv *priv)
>   
>   	/* Update and save current stats, see emac_stats_update() for usage */
>   
> -	spin_lock(&priv->stats_lock);
> +	spin_lock_bh(&priv->stats_lock);
>   
>   	emac_stats_update(priv);
>   
>   	priv->tx_stats_off = priv->tx_stats;
>   	priv->rx_stats_off = priv->rx_stats;
>   
> -	spin_unlock(&priv->stats_lock);
> +	spin_unlock_bh(&priv->stats_lock);
>   
>   	pm_runtime_put_sync(&pdev->dev);
>   	return 0;
> @@ -2111,7 +2111,7 @@ static int emac_resume(struct device *dev)
>   
>   	netif_device_attach(ndev);
>   
> -	emac_stats_timer(&priv->stats_timer);
> +	mod_timer(&priv->stats_timer, jiffies);
>   
>   	return 0;
>   }
>
> ---
> base-commit: 315f423be0d1ebe720d8fd4fa6bed68586b13d34
> change-id: 20250919-k1-ethernet-fix-lock-c99681a9aa5d
>
> Best regards,

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH net-next] net: spacemit: Make stats_lock softirq-safe
  2025-09-19 12:04 ` [PATCH net-next] net: spacemit: Make stats_lock softirq-safe Vivian Wang
  2025-09-19 12:25   ` Marek Szyprowski
@ 2025-09-22 19:00   ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-09-22 19:00 UTC (permalink / raw)
  To: Vivian Wang
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, dlan,
	maxime.chevallier, troy.mitchell, vadim.fedorenko, netdev,
	linux-riscv, spacemit, linux-kernel, uwu, m.szyprowski

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 19 Sep 2025 20:04:33 +0800 you wrote:
> While most of the statistics functions (emac_get_stats64() and such) are
> called with softirqs enabled, emac_stats_timer() is, as its name
> suggests, also called from a timer, i.e. called in softirq context.
> 
> All of these take stats_lock. Therefore, make stats_lock softirq-safe by
> changing spin_lock() into spin_lock_bh() for the functions that get
> statistics.
> 
> [...]

Here is the summary with links:
  - [net-next] net: spacemit: Make stats_lock softirq-safe
    https://git.kernel.org/netdev/net-next/c/35626012877b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-09-22 19:00 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <CGME20250919120522eucas1p224c42d0cc2a59903d85e6c80dfdfa727@eucas1p2.samsung.com>
2025-09-19 12:04 ` [PATCH net-next] net: spacemit: Make stats_lock softirq-safe Vivian Wang
2025-09-19 12:25   ` Marek Szyprowski
2025-09-22 19:00   ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox