diff mbox series

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

Message ID 20200820094307.3977-1-ashiduka@fujitsu.com (mailing list archive)
State Accepted
Commit 1838d6c62f57836639bd3d83e7855e0ee4f6defc
Delegated to: Geert Uytterhoeven
Headers show
Series [v3] ravb: Fixed to be able to unload modules | expand

Commit Message

Yuusuke Ashizuka 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

David Miller Aug. 20, 2020, 11:52 p.m. UTC | #1
From: Yuusuke Ashizuka <ashiduka@fujitsu.com>
Date: Thu, 20 Aug 2020 18:43:07 +0900

> 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

This is OK as a short term bug fix for sure.

But longer term, we should always be able to rmmod a networking
device driver even if it is UP.

Supplementary references from MDIO make this not possible.

What would need to happen is that MDIO would stop taking references to
the network device driver module, and it would register a netdevice
notifier that would do the necessary detachmentand resource release
during an unregister attempt.

That's how ipv4, ipv6, etc. deal with references to the device via
routes, assigned protocol addresses, etc.
David Miller Aug. 24, 2020, 10:38 p.m. UTC | #2
From: Yuusuke Ashizuka <ashiduka@fujitsu.com>
Date: Thu, 20 Aug 2020 18:43:07 +0900

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

Applied and queued up for -stable, thanks.
Geert Uytterhoeven Sept. 16, 2020, 9:31 a.m. UTC | #3
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 Sept. 21, 2020, 1:16 p.m. UTC | #4
On Wed, Sep 16, 2020 at 11:31 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> 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

phy_device_register() calls device_add(), which is immediately bound to
the micrel driver.

> 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.

During system suspend, defer_all_probes is set to true, to avoid drivers
being probed while suspended.  Hence phy_device_register() calling
device_add() merely adds the device, but does not probe it yet
(really_probe() returns early)"

   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


Hence registering PHY devices from a net_device's ndo_open() call back
must not be done.

I will send a formal revert later today.

Gr{oetje,eeting}s,

                        Geert
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);