[PATCH net v3 0/2] smsc911x: fix issues when interface is not up yet

[PATCH net v3 0/2] smsc911x: fix issues when interface is not up yet

[Wolfram Sang]

Similar to upstream commit 7f5ebf5dae42 ("ravb: avoid PHY being resumed
when interface is not up"), here is the fix for SMSC911x (patch 2).
Also, I saw a splat running 'ifconfig' when interface was not up. Patch
1 fixes it.

Patches are based on v6.2-rc3 and tested on a Renesas APE6-EK.

Changes since v2:
* added tags for patch 1
* patch 2 has been refactored to be less intrusive for easier
  backporting

Wolfram Sang (2):
  smsc911x: only update stats when interface is up
  smsc911x: avoid PHY being resumed when interface is not up

 drivers/net/ethernet/smsc/smsc911x.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.30.2

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

[Wolfram Sang]

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

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;
 }
 
-- 
2.30.2

[PATCH net v3 2/2] smsc911x: avoid PHY being resumed when interface is not up

[Wolfram Sang]

SMSC911x doesn't need mdiobus suspend/resume, that's why it sets
'mac_managed_pm'. However, setting it needs to be moved from init to
probe, so mdiobus PM functions will really never be called (e.g. when
the interface is not up yet during suspend/resume).

Fixes: 3ce9f2bef755 ("net: smsc911x: Stop and start PHY during suspend and resume")
Suggested-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since v2:
* kept the original error handling when a PHY is not present in
  mii_probe() to avoid regressions. Because the patch is simpler now, it
  is also easier to backport.

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

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 67cb5eb9c716..01aac820ac30 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1037,8 +1037,6 @@ static int smsc911x_mii_probe(struct net_device *dev)
 		return ret;
 	}
 
-	/* Indicate that the MAC is responsible for managing PHY PM */
-	phydev->mac_managed_pm = true;
 	phy_attached_info(phydev);
 
 	phy_set_max_speed(phydev, SPEED_100);
@@ -1066,6 +1064,7 @@ static int smsc911x_mii_init(struct platform_device *pdev,
 			     struct net_device *dev)
 {
 	struct smsc911x_data *pdata = netdev_priv(dev);
+	struct phy_device *phydev;
 	int err = -ENXIO;
 
 	pdata->mii_bus = mdiobus_alloc();
@@ -1108,6 +1107,10 @@ static int smsc911x_mii_init(struct platform_device *pdev,
 		goto err_out_free_bus_2;
 	}
 
+	phydev = phy_find_first(pdata->mii_bus);
+	if (phydev)
+		phydev->mac_managed_pm = true;
+
 	return 0;
 
 err_out_free_bus_2:
-- 
2.30.2

Re: [PATCH net v3 2/2] smsc911x: avoid PHY being resumed when interface is not up

[Simon Horman]

On Wed, Mar 22, 2023 at 08:19:59AM +0100, Wolfram Sang wrote:
> SMSC911x doesn't need mdiobus suspend/resume, that's why it sets
> 'mac_managed_pm'. However, setting it needs to be moved from init to
> probe, so mdiobus PM functions will really never be called (e.g. when
> the interface is not up yet during suspend/resume).
> 
> Fixes: 3ce9f2bef755 ("net: smsc911x: Stop and start PHY during suspend and resume")
> Suggested-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

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

[Jakub Kicinski]

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* ?

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

[Wolfram Sang]

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

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]