[PATCH net 0/2] smsc: fix bugs when interface is not up

[PATCH net 0/2] smsc: fix bugs when interface is not up

[Wolfram Sang]

I found two issues dealing with the SMSC911x on the Renesas APE6-EVM
when the interface is not up yet. Each patch here deals with one of
them. Please read the patch description for details.

Wolfram Sang (2):
  Revert "net: smsc911x: Make Runtime PM handling more fine-grained"
  smsc911x: avoid PHY being resumed when interface is not up

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

-- 
2.30.2

[PATCH 1/2] Revert "net: smsc911x: Make Runtime PM handling more fine-grained"

[Wolfram Sang]

This reverts commit 1e30b8d755b81b0d1585cb22bc753e9f2124fe87. Running
'ifconfig' with the interface down BUGs. This is the culprit:

	smsc911x_get_stats from dev_get_stats+0xe4/0xf4

The above function is called with the clocks off, so register read
fails. Enabling clocks in the above functions does not work, because it
is called in atomic context. So, let's return to the simple and working
PM we had before.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index a2e511912e6a..9d12fd54281a 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -557,7 +557,6 @@ static int smsc911x_mii_read(struct mii_bus *bus, int phyaddr, int regidx)
 	unsigned int addr;
 	int i, reg;
 
-	pm_runtime_get_sync(bus->parent);
 	spin_lock_irqsave(&pdata->mac_lock, flags);
 
 	/* Confirm MII not busy */
@@ -583,7 +582,6 @@ static int smsc911x_mii_read(struct mii_bus *bus, int phyaddr, int regidx)
 
 out:
 	spin_unlock_irqrestore(&pdata->mac_lock, flags);
-	pm_runtime_put(bus->parent);
 	return reg;
 }
 
@@ -596,7 +594,6 @@ static int smsc911x_mii_write(struct mii_bus *bus, int phyaddr, int regidx,
 	unsigned int addr;
 	int i, reg;
 
-	pm_runtime_get_sync(bus->parent);
 	spin_lock_irqsave(&pdata->mac_lock, flags);
 
 	/* Confirm MII not busy */
@@ -626,7 +623,6 @@ static int smsc911x_mii_write(struct mii_bus *bus, int phyaddr, int regidx,
 
 out:
 	spin_unlock_irqrestore(&pdata->mac_lock, flags);
-	pm_runtime_put(bus->parent);
 	return reg;
 }
 
@@ -1595,8 +1591,6 @@ static int smsc911x_open(struct net_device *dev)
 	int retval;
 	int irq_flags;
 
-	pm_runtime_get_sync(dev->dev.parent);
-
 	/* find and start the given phy */
 	if (!dev->phydev) {
 		retval = smsc911x_mii_probe(dev);
@@ -1743,7 +1737,6 @@ static int smsc911x_open(struct net_device *dev)
 	phy_disconnect(dev->phydev);
 	dev->phydev = NULL;
 out:
-	pm_runtime_put(dev->dev.parent);
 	return retval;
 }
 
@@ -1775,7 +1768,6 @@ static int smsc911x_stop(struct net_device *dev)
 		dev->phydev = NULL;
 	}
 	netif_carrier_off(dev);
-	pm_runtime_put(dev->dev.parent);
 
 	SMSC_TRACE(pdata, ifdown, "Interface stopped");
 	return 0;
@@ -2347,6 +2339,7 @@ static int smsc911x_drv_remove(struct platform_device *pdev)
 
 	free_netdev(dev);
 
+	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	return 0;
@@ -2552,7 +2545,6 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 	}
 
 	spin_unlock_irq(&pdata->mac_lock);
-	pm_runtime_put(&pdev->dev);
 
 	netdev_info(dev, "MAC Address: %pM\n", dev->dev_addr);
 
-- 
2.30.2

Re: [PATCH 1/2] Revert "net: smsc911x: Make Runtime PM handling more fine-grained"

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Mar 16, 2023 at 8:46 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> This reverts commit 1e30b8d755b81b0d1585cb22bc753e9f2124fe87. Running
> 'ifconfig' with the interface down BUGs. This is the culprit:
>
>         smsc911x_get_stats from dev_get_stats+0xe4/0xf4
>
> The above function is called with the clocks off, so register read
> fails. Enabling clocks in the above functions does not work, because it
> is called in atomic context. So, let's return to the simple and working
> PM we had before.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

In sh_eth this was fixed differently, by adding a check for
mdp->is_opened to sh_eth_get_stats() [1].
I believe the modern way would be to add a check for netif_running()
instead.

Would adding such a check to smsc911x_get_stats() work for you, too?

[1] 7fa2955ff70ce453 ("sh_eth: Fix sleeping function called from
invalid context")

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] Revert "net: smsc911x: Make Runtime PM handling more fine-grained"

[Wolfram Sang]

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

Hi Geert,

> In sh_eth this was fixed differently, by adding a check for
> mdp->is_opened to sh_eth_get_stats() [1].
> I believe the modern way would be to add a check for netif_running()
> instead.
> 
> Would adding such a check to smsc911x_get_stats() work for you, too?

Yeah, that worked. Thank you! I'll send v2 soon.

Happy hacking,

   Wolfram


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

[PATCH 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). The errno is changed
because ENODEV has a special meaning when returned in probe().

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>
---
 drivers/net/ethernet/smsc/smsc911x.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 9d12fd54281a..4cc5b9833e8f 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1015,12 +1015,7 @@ static int smsc911x_mii_probe(struct net_device *dev)
 	struct phy_device *phydev = NULL;
 	int ret;
 
-	/* find the first phy */
 	phydev = phy_find_first(pdata->mii_bus);
-	if (!phydev) {
-		netdev_err(dev, "no PHY found\n");
-		return -ENODEV;
-	}
 
 	SMSC_TRACE(pdata, probe, "PHY: addr %d, phy_id 0x%08X",
 		   phydev->mdio.addr, phydev->phy_id);
@@ -1033,8 +1028,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);
@@ -1062,6 +1055,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();
@@ -1104,6 +1098,15 @@ static int smsc911x_mii_init(struct platform_device *pdev,
 		goto err_out_free_bus_2;
 	}
 
+	phydev = phy_find_first(pdata->mii_bus);
+	if (!phydev) {
+		netdev_err(dev, "no PHY found\n");
+		err = -ENOENT;
+		goto err_out_free_bus_2;
+	}
+
+	phydev->mac_managed_pm = true;
+
 	return 0;
 
 err_out_free_bus_2:
-- 
2.30.2