| Message ID | 20230316074558.15268-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | smsc: fix bugs when interface is not up | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 18 this patch: 18 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 18 this patch: 18 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 64 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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
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
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);
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(-)