diff mbox series

[v3] ravb: Fixed to be able to unload modules

Message ID 20200820094307.3977-1-ashiduka@fujitsu.com
State New
Headers show
Series [v3] ravb: Fixed to be able to unload modules | expand

Commit Message

ashiduka@fujitsu.com Aug. 20, 2020, 9:43 a.m. UTC
When this driver is built as a module, I cannot rmmod it after insmoding
it.
This is because that this driver calls ravb_mdio_init() at the time of
probe, and module->refcnt is incremented by alloc_mdio_bitbang() called
after that.
Therefore, even if ifup is not performed, the driver is in use and rmmod
cannot be performed.

$ lsmod
Module                  Size  Used by
ravb                   40960  1
$ rmmod ravb
rmmod: ERROR: Module ravb is in use

Call ravb_mdio_init() at open and free_mdio_bitbang() at close, thereby
rmmod is possible in the ifdown state.

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
Signed-off-by: Yuusuke Ashizuka <ashiduka@fujitsu.com>
Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com>
---
Changes in v3:
 - Update the commit subject and description.
 - Add Fixes c156633f1353.
 - Add Reviewed-by Sergei.
 https://patchwork.kernel.org/patch/11692719/

Changes in v2:
 - Fix build error

 drivers/net/ethernet/renesas/ravb_main.c | 110 +++++++++++++++----------------
 1 file changed, 55 insertions(+), 55 deletions(-)

Comments

Sergei Shtylyov Aug. 20, 2020, 12:29 p.m. UTC | #1
Also, s/Fixed/fix/ in the subject. Nearly missed it. :-)
Geert Uytterhoeven Sept. 16, 2020, 9:31 a.m. UTC | #2
Hi Ashizuka-san,

On Thu, Aug 20, 2020 at 2:55 PM Yuusuke Ashizuka <ashiduka@fujitsu.com> wrote:
> When this driver is built as a module, I cannot rmmod it after insmoding

> it.

> This is because that this driver calls ravb_mdio_init() at the time of

> probe, and module->refcnt is incremented by alloc_mdio_bitbang() called

> after that.

> Therefore, even if ifup is not performed, the driver is in use and rmmod

> cannot be performed.

>

> $ lsmod

> Module                  Size  Used by

> ravb                   40960  1

> $ rmmod ravb

> rmmod: ERROR: Module ravb is in use

>

> Call ravb_mdio_init() at open and free_mdio_bitbang() at close, thereby

> rmmod is possible in the ifdown state.

>

> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

> Signed-off-by: Yuusuke Ashizuka <ashiduka@fujitsu.com>

> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com>


Thanks for your patch, which is now commit 1838d6c62f578366 ("ravb:
Fixed to be able to unload modules") in v5.9-rc4 (backported to stable
v4.4, v4.9, v4.14, v4.19, v5.4, and v5.8).

This is causing a regression during resume from s2idle/s2ram on (at
least) Salvator-X(S) and Ebisu.  Reverting that commit fixes this.

During boot, the Micrel PHY is detected correctly:

    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 resume, if CONFIG_MODULES=n, it falls back to the Generic PHY
(case A):

    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

and Ethernet still works (degraded, due to polling).

During resume, if CONFIG_MODULES=y, MDIO initialization fails (case B):

    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

and Ethernet no longer works.

Case B happens because usermodehelper_disabled is set to UMH_DISABLED
during system suspend, causing request_module() to return -EBUSY.
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"), makes it fall back to the Generic PHY, cfr.
case A.

For case A, I haven't found out yet why it falls back to the Generic PHY.

Thanks for your comments!

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
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 99f7aae..df89d09 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1342,6 +1342,51 @@  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)
 {
@@ -1350,6 +1395,13 @@  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]);
 
@@ -1427,6 +1479,7 @@  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;
 }
 
@@ -1736,6 +1789,8 @@  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;
 }
 
@@ -1887,51 +1942,6 @@  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 },
@@ -2174,13 +2184,6 @@  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);
 
@@ -2202,8 +2205,6 @@  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);
 
@@ -2235,7 +2236,6 @@  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);