[RFC 0/2] net: phy: Prevent an MDIO bus from being unloaded while in use

[RFC 0/2] net: phy: Prevent an MDIO bus from being unloaded while in use

[Ezequiel Garcia]

Hi all,

This small patchset is a first proposal to fix the current lack of relationship
between an ethernet driver and the MDIO bus behind the PHY device.

In such cases, we would find no visible link between the drivers:

  $ lsmod
  Module                  Size  Used by    Not tainted
  mvmdio                  2941  0 
  mvneta                 22069  0 

Which means nothing prevents the MDIO driver from being removed:

  $ modprobe -r mvmdio

  # Unable to handle kernel NULL pointer dereference at virtual address 00000060
  pgd = c0004000
  [00000060] *pgd=00000000
  Internal error: Oops: 17 [#1] SMP ARM
  Modules linked in: mvneta [last unloaded: mvmdio]
  CPU: 0 PID: 22 Comm: kworker/0:1 Not tainted 3.16.0-rc5-01127-g62c0816-dirty #608
  Workqueue: events_power_efficient phy_state_machine
  task: df5ec840 ti: df67a000 task.ti: df67a000
  PC is at phy_state_machine+0x1c/0x468
  LR is at phy_state_machine+0x18/0x468
  [snip]

This patchset fixes this by calling module_{get,put} in the ethernet driver.
After this patch, the mvmdio driver is no longer removable:

  $ modprobe -r mvmdio
  modprobe: can't unload module mvmdio: Resource temporarily unavailable

Of course, we can expect a few drivers to have the same need and therefore
we could argue about moving this to be part of the phydev connection.

Ezequiel Garcia (2):
  net: phy: Set the driver when registering an MDIO bus device
  net: mvneta: Ensure the MDIO bus module is held

 drivers/net/ethernet/marvell/mvneta.c | 12 ++++++++++++
 drivers/net/phy/mdio_bus.c            |  1 +
 2 files changed, 13 insertions(+)

-- 
2.0.1

[RFC 1/2] net: phy: Set the driver when registering an MDIO bus device

[Ezequiel Garcia]

mdiobus_register() registers a device which is already bound to a driver.
Hence, the driver pointer should be set properly in order to track down
the driver associated to the MDIO bus.

This will be used to allow ethernet driver to pin down a MDIO bus driver,
preventing it from being unloaded while the PHY device is running.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/net/phy/mdio_bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 4eaadcf..203651e 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -255,6 +255,7 @@ int mdiobus_register(struct mii_bus *bus)
 
 	bus->dev.parent = bus->parent;
 	bus->dev.class = &mdio_bus_class;
+	bus->dev.driver = bus->parent->driver;
 	bus->dev.groups = NULL;
 	dev_set_name(&bus->dev, "%s", bus->id);
 
-- 
2.0.1

[RFC 2/2] net: mvneta: Ensure the MDIO bus module is held

[Ezequiel Garcia]

This commit adds proper module_{get,put} to prevent the MDIO bus module
from being unloaded while the phydev is connected. By doing so, we fix
a kernel panic produced when the mvmdio driver is removed, and one of
the ethernet interfaces using it is "up".

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index dadd9a5..98d0cd1 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -297,6 +297,7 @@ struct mvneta_port {
 	struct mvneta_pcpu_stats *stats;
 
 	struct mii_bus *mii_bus;
+	struct module *phy_module;
 	struct phy_device *phy_dev;
 	phy_interface_t phy_interface;
 	struct device_node *phy_node;
@@ -2575,6 +2576,15 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
 		return -ENODEV;
 	}
 
+	/* Get the MDIO driver so it can't be removed beneath our feet */
+	pp->phy_module = phy_dev->bus->dev.driver ?
+			 phy_dev->bus->dev.driver->owner : NULL;
+	if (!try_module_get(pp->phy_module)) {
+		netdev_err(pp->dev, "could not get the MDIO module\n");
+		phy_disconnect(phy_dev);
+		return -EIO;
+	}
+
 	phy_dev->supported &= PHY_GBIT_FEATURES;
 	phy_dev->advertising = phy_dev->supported;
 
@@ -2590,6 +2600,8 @@ static void mvneta_mdio_remove(struct mvneta_port *pp)
 {
 	phy_disconnect(pp->phy_dev);
 	pp->phy_dev = NULL;
+	module_put(pp->phy_module);
+	pp->phy_module = NULL;
 }
 
 static int mvneta_open(struct net_device *dev)
-- 
2.0.1

Re: [RFC 0/2] net: phy: Prevent an MDIO bus from being unloaded while in use

[Florian Fainelli]

Hi Ezequiel,

2014-07-21 11:26 GMT-07:00 Ezequiel Garcia <ezequiel.garcia@free-electrons.com>:
> Hi all,
>
> This small patchset is a first proposal to fix the current lack of relationship
> between an ethernet driver and the MDIO bus behind the PHY device.
>
> In such cases, we would find no visible link between the drivers:
>
>   $ lsmod
>   Module                  Size  Used by    Not tainted
>   mvmdio                  2941  0
>   mvneta                 22069  0
>
> Which means nothing prevents the MDIO driver from being removed:
>
>   $ modprobe -r mvmdio
>
>   # Unable to handle kernel NULL pointer dereference at virtual address 00000060
>   pgd = c0004000
>   [00000060] *pgd=00000000
>   Internal error: Oops: 17 [#1] SMP ARM
>   Modules linked in: mvneta [last unloaded: mvmdio]
>   CPU: 0 PID: 22 Comm: kworker/0:1 Not tainted 3.16.0-rc5-01127-g62c0816-dirty #608
>   Workqueue: events_power_efficient phy_state_machine
>   task: df5ec840 ti: df67a000 task.ti: df67a000
>   PC is at phy_state_machine+0x1c/0x468
>   LR is at phy_state_machine+0x18/0x468
>   [snip]
>
> This patchset fixes this by calling module_{get,put} in the ethernet driver.
> After this patch, the mvmdio driver is no longer removable:
>
>   $ modprobe -r mvmdio
>   modprobe: can't unload module mvmdio: Resource temporarily unavailable
>
> Of course, we can expect a few drivers to have the same need and therefore
> we could argue about moving this to be part of the phydev connection.

Right, that's exactly what jumps to mind when I take a look at your
patch set. As long as a PHY device connected to a particular MDIO bus
is in used, we should make sure its refcount does not hit zero.

BTW, neither mvmdio nor mvneta seems to set their 'owner' field, is
that intentional?

>
> Ezequiel Garcia (2):
>   net: phy: Set the driver when registering an MDIO bus device
>   net: mvneta: Ensure the MDIO bus module is held
>
>  drivers/net/ethernet/marvell/mvneta.c | 12 ++++++++++++
>  drivers/net/phy/mdio_bus.c            |  1 +
>  2 files changed, 13 insertions(+)
>
> --
> 2.0.1
>



-- 
Florian

Re: [RFC 0/2] net: phy: Prevent an MDIO bus from being unloaded while in use

[Ezequiel Garcia]

Hi Florian,

Thanks for the quick reply.

On 21 Jul 11:42 AM, Florian Fainelli wrote:
[..]
> >
> > This patchset fixes this by calling module_{get,put} in the ethernet driver.
> > After this patch, the mvmdio driver is no longer removable:
> >
> >   $ modprobe -r mvmdio
> >   modprobe: can't unload module mvmdio: Resource temporarily unavailable
> >
> > Of course, we can expect a few drivers to have the same need and therefore
> > we could argue about moving this to be part of the phydev connection.
> 
> Right, that's exactly what jumps to mind when I take a look at your
> patch set. As long as a PHY device connected to a particular MDIO bus
> is in used, we should make sure its refcount does not hit zero.
> 

Any suggestions about where to do it? I think the right place is
after the calls to bus_find_device() in of_phy_find_device() and
phy_connect().

> BTW, neither mvmdio nor mvneta seems to set their 'owner' field, is
> that intentional?
> 

Actually, I had a patch that did so and then dropped it. The owner
is set to unconditionally for plaftorm drivers, so setting it has no
effect. See include/linux/platform_device.h:

/*
 * use a macro to avoid include chaining to get THIS_MODULE
 */
#define platform_driver_register(drv) \
        __platform_driver_register(drv, THIS_MODULE)

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [RFC 0/2] net: phy: Prevent an MDIO bus from being unloaded while in use

[Florian Fainelli]

2014-07-21 11:55 GMT-07:00 Ezequiel Garcia <ezequiel.garcia@free-electrons.com>:
> Hi Florian,
>
> Thanks for the quick reply.
>
> On 21 Jul 11:42 AM, Florian Fainelli wrote:
> [..]
>> >
>> > This patchset fixes this by calling module_{get,put} in the ethernet driver.
>> > After this patch, the mvmdio driver is no longer removable:
>> >
>> >   $ modprobe -r mvmdio
>> >   modprobe: can't unload module mvmdio: Resource temporarily unavailable
>> >
>> > Of course, we can expect a few drivers to have the same need and therefore
>> > we could argue about moving this to be part of the phydev connection.
>>
>> Right, that's exactly what jumps to mind when I take a look at your
>> patch set. As long as a PHY device connected to a particular MDIO bus
>> is in used, we should make sure its refcount does not hit zero.
>>
>
> Any suggestions about where to do it? I think the right place is
> after the calls to bus_find_device() in of_phy_find_device() and
> phy_connect().

A common place that is guaranteed to be called by all
phy_{connect,attach}{,_direct} is phy_attach_direct(), presumably
right after the call to device_bind_driver for instance?

>
>> BTW, neither mvmdio nor mvneta seems to set their 'owner' field, is
>> that intentional?
>>
>
> Actually, I had a patch that did so and then dropped it. The owner
> is set to unconditionally for plaftorm drivers, so setting it has no
> effect. See include/linux/platform_device.h:

Ah, indeed, I guess a coccinelle spatch could clean up this redundant
assignment for a bunch of drivers then.

>
> /*
>  * use a macro to avoid include chaining to get THIS_MODULE
>  */
> #define platform_driver_register(drv) \
>         __platform_driver_register(drv, THIS_MODULE)
>
> --
> Ezequiel García, Free Electrons
> Embedded Linux, Kernel and Android Engineering
> http://free-electrons.com



-- 
Florian