diff mbox series

[net,v3,1/2] smsc911x: only update stats when interface is up

Message ID 20230322071959.9101-2-wsa+renesas@sang-engineering.com
State New
Headers
Series smsc911x: fix issues when interface is not up yet |

Commit Message

Wolfram Sang March 22, 2023, 7:19 a.m. UTC 
  Otherwise the clocks are not enabled and reading registers will BUG.

Fixes: 1e30b8d755b8 ("net: smsc911x: Make Runtime PM handling more fine-grained")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes since v2:
* added tags

 drivers/net/ethernet/smsc/smsc911x.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski March 23, 2023, 6:39 p.m. UTC | #1 
On Wed, 22 Mar 2023 08:19:58 +0100 Wolfram Sang wrote:
> -	smsc911x_tx_update_txcounters(dev);
> -	dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
> +
> +	if (netif_running(dev)) {
> +		smsc911x_tx_update_txcounters(dev);
> +		dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
> +	}

Same problem as on the renesas patch, netif_running() can return true
before ndo->open() is called. And stats can be read with just the RCU
lock (via procfs).

Maybe we should add a false-negative version of netif_running() ?
__LINK_STATE_START*ED* ?
Wolfram Sang March 24, 2023, 7:33 a.m. UTC | #2 
Hi Jakub,

> Maybe we should add a false-negative version of netif_running() ?
> __LINK_STATE_START*ED* ?

Sounds very reasonable, yet I am missing subsystem details to really
judge it. I just hacked a quick coccinelle script and 14 more drivers
could be happy about such a change:

 drivers/net/ethernet/3com/3c515.c            | 2 +-
 drivers/net/ethernet/8390/axnet_cs.c         | 2 +-
 drivers/net/ethernet/8390/lib8390.c          | 2 +-
 drivers/net/ethernet/dec/tulip/de2104x.c     | 2 +-
 drivers/net/ethernet/dec/tulip/tulip_core.c  | 2 +-
 drivers/net/ethernet/dec/tulip/winbond-840.c | 2 +-
 drivers/net/ethernet/fealnx.c                | 2 +-
 drivers/net/ethernet/natsemi/natsemi.c       | 2 +-
 drivers/net/ethernet/realtek/8139cp.c        | 2 +-
 drivers/net/ethernet/silan/sc92031.c         | 2 +-
 drivers/net/ethernet/smsc/epic100.c          | 2 +-
 drivers/net/ethernet/sun/sungem.c            | 2 +-
 drivers/net/ethernet/toshiba/tc35815.c       | 2 +-
 drivers/net/ethernet/via/via-velocity.c      | 2 +-
 14 files changed, 14 insertions(+), 14 deletions(-)

The script:

@@
identifier name, args;
@@

	static struct net_device_stats *name(struct net_device *args)
	{
		...
-		netif_running
+		netif_opened
		...
	}


Thanks and happy hacking,

   Wolfram
diff mbox series

Patch

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index a2e511912e6a..67cb5eb9c716 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1838,8 +1838,12 @@  smsc911x_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 static struct net_device_stats *smsc911x_get_stats(struct net_device *dev)
 {
 	struct smsc911x_data *pdata = netdev_priv(dev);
-	smsc911x_tx_update_txcounters(dev);
-	dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
+
+	if (netif_running(dev)) {
+		smsc911x_tx_update_txcounters(dev);
+		dev->stats.rx_dropped += smsc911x_reg_read(pdata, RX_DROP);
+	}
+
 	return &dev->stats;
 }