| Message ID | 20200922072931.2148-1-geert+renesas@glider.be |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] Revert "ravb: Fixed to be able to unload modules" | expand |
On 22.09.2020 10:29, Geert Uytterhoeven wrote: > This reverts commit 1838d6c62f57836639bd3d83e7855e0ee4f6defc. > > This commit moved the ravb_mdio_init() call (and thus the > of_mdiobus_register() call) from the ravb_probe() to the ravb_open() > call. This causes a regression during system resume (s2idle/s2ram), as > new PHY devices cannot be bound while suspended. > > During boot, the Micrel PHY is detected like this: > > Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=228) > ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off > > During system suspend, (A) defer_all_probes is set to true, and (B) > usermodehelper_disabled is set to UMH_DISABLED, to avoid drivers being > probed while suspended. > > A. If CONFIG_MODULES=n, phy_device_register() calling device_add() > merely adds the device, but does not probe it yet, as > really_probe() returns early due to defer_all_probes being set: > > dpm_resume+0x128/0x4f8 > device_resume+0xcc/0x1b0 > dpm_run_callback+0x74/0x340 > ravb_resume+0x190/0x1b8 > ravb_open+0x84/0x770 > of_mdiobus_register+0x1e0/0x468 > of_mdiobus_register_phy+0x1b8/0x250 > of_mdiobus_phy_device_register+0x178/0x1e8 > phy_device_register+0x114/0x1b8 > device_add+0x3d4/0x798 > bus_probe_device+0x98/0xa0 > device_initial_probe+0x10/0x18 > __device_attach+0xe4/0x140 > bus_for_each_drv+0x64/0xc8 > __device_attach_driver+0xb8/0xe0 > driver_probe_device.part.11+0xc4/0xd8 > really_probe+0x32c/0x3b8 > > Later, phy_attach_direct() notices no PHY driver has been bound, > and falls back to the Generic PHY, leading to degraded operation: > > Generic PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=POLL) > ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off > > B. If CONFIG_MODULES=y, request_module() returns early with -EBUSY due > to UMH_DISABLED, and MDIO initialization fails completely: > > mdio_bus e6800000.ethernet-ffffffff:00: error -16 loading PHY driver module for ID 0x00221622 > ravb e6800000.ethernet eth0: failed to initialize MDIO > PM: dpm_run_callback(): ravb_resume+0x0/0x1b8 returns -16 > PM: Device e6800000.ethernet failed to resume: error -16 > > Ignoring -EBUSY in phy_request_driver_module(), like was done for > -ENOENT in commit 21e194425abd65b5 ("net: phy: fix issue with loading > PHY driver w/o initramfs"), would makes it fall back to the Generic > PHY, like in the CONFIG_MODULES=n case. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: stable@vger.kernel.org Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com> > --- > Commit 1838d6c62f578366 ("ravb: Fixed to be able to unload modules") was > already backported to stable v4.4, v4.9, v4.14, v4.19, v5.4, and v5.8), > and thus needs to be reverted there, too. Ugh! Sorry for not noticing the issue with the original patch... :-/ MBR, Sergei
From: Geert Uytterhoeven <geert+renesas@glider.be> Date: Tue, 22 Sep 2020 09:29:31 +0200 > This reverts commit 1838d6c62f57836639bd3d83e7855e0ee4f6defc. > > This commit moved the ravb_mdio_init() call (and thus the > of_mdiobus_register() call) from the ravb_probe() to the ravb_open() > call. This causes a regression during system resume (s2idle/s2ram), as > new PHY devices cannot be bound while suspended. ... > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: stable@vger.kernel.org I noticed this too late, but please don't CC: stable on networking patches. We have our own workflow as per the netdev FAQ. I've applied this but the inability to remove a module is an extremely serious bug and should be fixed properly.
Hi David, On Thu, Sep 24, 2020 at 2:40 AM David Miller <davem@davemloft.net> wrote: > From: Geert Uytterhoeven <geert+renesas@glider.be> > Date: Tue, 22 Sep 2020 09:29:31 +0200 > > > This reverts commit 1838d6c62f57836639bd3d83e7855e0ee4f6defc. > > > > This commit moved the ravb_mdio_init() call (and thus the > > of_mdiobus_register() call) from the ravb_probe() to the ravb_open() > > call. This causes a regression during system resume (s2idle/s2ram), as > > new PHY devices cannot be bound while suspended. > ... > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Cc: stable@vger.kernel.org > > I noticed this too late, but please don't CC: stable on networking > patches. We have our own workflow as per the netdev FAQ. OK, will try to remember. I wanted to give a heads-up to stable that they've backported early a patch which turned out to have issues. > I've applied this but the inability to remove a module is an > extremely serious bug and should be fixed properly. Sure. As you stated in https://lore.kernel.org/linux-renesas-soc/20200820.165244.540878641387937530.davem@davemloft.net/ that will need some rework in the MDIO subsystem... Thanks! Gr{oetje,eeting}s, Geert
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 5082c16bf9c060b2..9c4df4ede0111eae 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1339,51 +1339,6 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler, return error; } -/* MDIO bus init function */ -static int ravb_mdio_init(struct ravb_private *priv) -{ - struct platform_device *pdev = priv->pdev; - struct device *dev = &pdev->dev; - int error; - - /* Bitbang init */ - priv->mdiobb.ops = &bb_ops; - - /* MII controller setting */ - priv->mii_bus = alloc_mdio_bitbang(&priv->mdiobb); - if (!priv->mii_bus) - return -ENOMEM; - - /* Hook up MII support for ethtool */ - priv->mii_bus->name = "ravb_mii"; - priv->mii_bus->parent = dev; - snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", - pdev->name, pdev->id); - - /* Register MDIO bus */ - error = of_mdiobus_register(priv->mii_bus, dev->of_node); - if (error) - goto out_free_bus; - - return 0; - -out_free_bus: - free_mdio_bitbang(priv->mii_bus); - return error; -} - -/* MDIO bus release function */ -static int ravb_mdio_release(struct ravb_private *priv) -{ - /* Unregister mdio bus */ - mdiobus_unregister(priv->mii_bus); - - /* Free bitbang info */ - free_mdio_bitbang(priv->mii_bus); - - return 0; -} - /* Network device open function for Ethernet AVB */ static int ravb_open(struct net_device *ndev) { @@ -1392,13 +1347,6 @@ static int ravb_open(struct net_device *ndev) struct device *dev = &pdev->dev; int error; - /* MDIO bus init */ - error = ravb_mdio_init(priv); - if (error) { - netdev_err(ndev, "failed to initialize MDIO\n"); - return error; - } - napi_enable(&priv->napi[RAVB_BE]); napi_enable(&priv->napi[RAVB_NC]); @@ -1476,7 +1424,6 @@ static int ravb_open(struct net_device *ndev) out_napi_off: napi_disable(&priv->napi[RAVB_NC]); napi_disable(&priv->napi[RAVB_BE]); - ravb_mdio_release(priv); return error; } @@ -1786,8 +1733,6 @@ static int ravb_close(struct net_device *ndev) ravb_ring_free(ndev, RAVB_BE); ravb_ring_free(ndev, RAVB_NC); - ravb_mdio_release(priv); - return 0; } @@ -1939,6 +1884,51 @@ static const struct net_device_ops ravb_netdev_ops = { .ndo_set_features = ravb_set_features, }; +/* MDIO bus init function */ +static int ravb_mdio_init(struct ravb_private *priv) +{ + struct platform_device *pdev = priv->pdev; + struct device *dev = &pdev->dev; + int error; + + /* Bitbang init */ + priv->mdiobb.ops = &bb_ops; + + /* MII controller setting */ + priv->mii_bus = alloc_mdio_bitbang(&priv->mdiobb); + if (!priv->mii_bus) + return -ENOMEM; + + /* Hook up MII support for ethtool */ + priv->mii_bus->name = "ravb_mii"; + priv->mii_bus->parent = dev; + snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", + pdev->name, pdev->id); + + /* Register MDIO bus */ + error = of_mdiobus_register(priv->mii_bus, dev->of_node); + if (error) + goto out_free_bus; + + return 0; + +out_free_bus: + free_mdio_bitbang(priv->mii_bus); + return error; +} + +/* MDIO bus release function */ +static int ravb_mdio_release(struct ravb_private *priv) +{ + /* Unregister mdio bus */ + mdiobus_unregister(priv->mii_bus); + + /* Free bitbang info */ + free_mdio_bitbang(priv->mii_bus); + + return 0; +} + static const struct of_device_id ravb_match_table[] = { { .compatible = "renesas,etheravb-r8a7790", .data = (void *)RCAR_GEN2 }, { .compatible = "renesas,etheravb-r8a7794", .data = (void *)RCAR_GEN2 }, @@ -2213,6 +2203,13 @@ static int ravb_probe(struct platform_device *pdev) eth_hw_addr_random(ndev); } + /* MDIO bus init */ + error = ravb_mdio_init(priv); + if (error) { + dev_err(&pdev->dev, "failed to initialize MDIO\n"); + goto out_dma_free; + } + netif_napi_add(ndev, &priv->napi[RAVB_BE], ravb_poll, 64); netif_napi_add(ndev, &priv->napi[RAVB_NC], ravb_poll, 64); @@ -2234,6 +2231,8 @@ static int ravb_probe(struct platform_device *pdev) out_napi_del: netif_napi_del(&priv->napi[RAVB_NC]); netif_napi_del(&priv->napi[RAVB_BE]); + ravb_mdio_release(priv); +out_dma_free: dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat, priv->desc_bat_dma); @@ -2265,6 +2264,7 @@ static int ravb_remove(struct platform_device *pdev) unregister_netdev(ndev); netif_napi_del(&priv->napi[RAVB_NC]); netif_napi_del(&priv->napi[RAVB_BE]); + ravb_mdio_release(priv); pm_runtime_disable(&pdev->dev); free_netdev(ndev); platform_set_drvdata(pdev, NULL);
This reverts commit 1838d6c62f57836639bd3d83e7855e0ee4f6defc. This commit moved the ravb_mdio_init() call (and thus the of_mdiobus_register() call) from the ravb_probe() to the ravb_open() call. This causes a regression during system resume (s2idle/s2ram), as new PHY devices cannot be bound while suspended. During boot, the Micrel PHY is detected like this: Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=228) ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off During system suspend, (A) defer_all_probes is set to true, and (B) usermodehelper_disabled is set to UMH_DISABLED, to avoid drivers being probed while suspended. A. If CONFIG_MODULES=n, phy_device_register() calling device_add() merely adds the device, but does not probe it yet, as really_probe() returns early due to defer_all_probes being set: dpm_resume+0x128/0x4f8 device_resume+0xcc/0x1b0 dpm_run_callback+0x74/0x340 ravb_resume+0x190/0x1b8 ravb_open+0x84/0x770 of_mdiobus_register+0x1e0/0x468 of_mdiobus_register_phy+0x1b8/0x250 of_mdiobus_phy_device_register+0x178/0x1e8 phy_device_register+0x114/0x1b8 device_add+0x3d4/0x798 bus_probe_device+0x98/0xa0 device_initial_probe+0x10/0x18 __device_attach+0xe4/0x140 bus_for_each_drv+0x64/0xc8 __device_attach_driver+0xb8/0xe0 driver_probe_device.part.11+0xc4/0xd8 really_probe+0x32c/0x3b8 Later, phy_attach_direct() notices no PHY driver has been bound, and falls back to the Generic PHY, leading to degraded operation: Generic PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=POLL) ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off B. If CONFIG_MODULES=y, request_module() returns early with -EBUSY due to UMH_DISABLED, and MDIO initialization fails completely: mdio_bus e6800000.ethernet-ffffffff:00: error -16 loading PHY driver module for ID 0x00221622 ravb e6800000.ethernet eth0: failed to initialize MDIO PM: dpm_run_callback(): ravb_resume+0x0/0x1b8 returns -16 PM: Device e6800000.ethernet failed to resume: error -16 Ignoring -EBUSY in phy_request_driver_module(), like was done for -ENOENT in commit 21e194425abd65b5 ("net: phy: fix issue with loading PHY driver w/o initramfs"), would makes it fall back to the Generic PHY, like in the CONFIG_MODULES=n case. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Cc: stable@vger.kernel.org --- Commit 1838d6c62f578366 ("ravb: Fixed to be able to unload modules") was already backported to stable v4.4, v4.9, v4.14, v4.19, v5.4, and v5.8), and thus needs to be reverted there, too. --- drivers/net/ethernet/renesas/ravb_main.c | 110 +++++++++++------------ 1 file changed, 55 insertions(+), 55 deletions(-)