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

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

[Ezequiel Garcia]

The motivation of this small series is 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 problem by adding a pair of module_{get,put},
so the module reference count is increased when the PHY device is attached
and is decreased when the PHY is detached.

Tested with mvneta and mv643xx_eth drivers, which depend on the mvmdio
driver to handle MDIO. This series applies on both net and net-next branches.

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

 drivers/net/phy/mdio_bus.c   |  1 +
 drivers/net/phy/phy_device.c | 23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

-- 
2.0.1

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

[PATCH 2/2] net: phy: 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 a MDIO driver is removed, but the phydev
that relies on it is attached and running.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/net/phy/phy_device.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 35d753d..a10dc47 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -575,6 +575,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		      u32 flags, phy_interface_t interface)
 {
 	struct device *d = &phydev->dev;
+	struct module *bus_module;
 	int err;
 
 	/* Assume that if there is no driver, that it doesn't
@@ -599,6 +600,14 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		return -EBUSY;
 	}
 
+	/* Increment the bus module reference count */
+	bus_module = phydev->bus->dev.driver ?
+		     phydev->bus->dev.driver->owner : NULL;
+	if (!try_module_get(bus_module)) {
+		dev_err(&dev->dev, "failed to get the bus module\n");
+		return -EIO;
+	}
+
 	phydev->attached_dev = dev;
 	dev->phydev = phydev;
 
@@ -614,10 +623,14 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	 */
 	err = phy_init_hw(phydev);
 	if (err)
-		phy_detach(phydev);
-	else
-		phy_resume(phydev);
+		goto err_module_put;
 
+	phy_resume(phydev);
+	return err;
+
+err_module_put:
+	module_put(bus_module);
+	phy_detach(phydev);
 	return err;
 }
 EXPORT_SYMBOL(phy_attach_direct);
@@ -664,6 +677,10 @@ EXPORT_SYMBOL(phy_attach);
 void phy_detach(struct phy_device *phydev)
 {
 	int i;
+
+	if (phydev->bus->dev.driver)
+		module_put(phydev->bus->dev.driver->owner);
+
 	phydev->attached_dev->phydev = NULL;
 	phydev->attached_dev = NULL;
 	phy_suspend(phydev);
-- 
2.0.1

Re: [PATCH 2/2] net: phy: Ensure the MDIO bus module is held

[Florian Fainelli]

2014-07-22 17:16 GMT-07:00 Ezequiel Garcia <ezequiel.garcia@free-electrons.com>:
> 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 a MDIO driver is removed, but the phydev
> that relies on it is attached and running.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/net/phy/phy_device.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 35d753d..a10dc47 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -575,6 +575,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>                       u32 flags, phy_interface_t interface)
>  {
>         struct device *d = &phydev->dev;
> +       struct module *bus_module;
>         int err;
>
>         /* Assume that if there is no driver, that it doesn't
> @@ -599,6 +600,14 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>                 return -EBUSY;
>         }
>
> +       /* Increment the bus module reference count */
> +       bus_module = phydev->bus->dev.driver ?
> +                    phydev->bus->dev.driver->owner : NULL;
> +       if (!try_module_get(bus_module)) {
> +               dev_err(&dev->dev, "failed to get the bus module\n");
> +               return -EIO;
> +       }
> +
>         phydev->attached_dev = dev;
>         dev->phydev = phydev;
>
> @@ -614,10 +623,14 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>          */
>         err = phy_init_hw(phydev);
>         if (err)
> -               phy_detach(phydev);
> -       else
> -               phy_resume(phydev);
> +               goto err_module_put;
>
> +       phy_resume(phydev);
> +       return err;
> +
> +err_module_put:
> +       module_put(bus_module);
> +       phy_detach(phydev);

Since we are calling phy_detach() which is also attempting to do a
module_put(), does not that result in one too many calls to
module_put() on error path?

>         return err;
>  }
>  EXPORT_SYMBOL(phy_attach_direct);
> @@ -664,6 +677,10 @@ EXPORT_SYMBOL(phy_attach);
>  void phy_detach(struct phy_device *phydev)
>  {
>         int i;
> +
> +       if (phydev->bus->dev.driver)
> +               module_put(phydev->bus->dev.driver->owner);
> +
>         phydev->attached_dev->phydev = NULL;
>         phydev->attached_dev = NULL;
>         phy_suspend(phydev);
> --
> 2.0.1
>



-- 
Florian

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

[Florian Fainelli]

2014-07-22 17:16 GMT-07:00 Ezequiel Garcia <ezequiel.garcia@free-electrons.com>:
> 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.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
>



-- 
Florian

Re: [PATCH 2/2] net: phy: Ensure the MDIO bus module is held

[Ezequiel Garcia]

On 23 Jul 11:22 AM, Florian Fainelli wrote:
> 2014-07-22 17:16 GMT-07:00 Ezequiel Garcia <ezequiel.garcia@free-electrons.com>:
> > @@ -614,10 +623,14 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
> >          */
> >         err = phy_init_hw(phydev);
> >         if (err)
> > -               phy_detach(phydev);
> > -       else
> > -               phy_resume(phydev);
> > +               goto err_module_put;
> >
> > +       phy_resume(phydev);
> > +       return err;
> > +
> > +err_module_put:
> > +       module_put(bus_module);
> > +       phy_detach(phydev);
> 
> Since we are calling phy_detach() which is also attempting to do a
> module_put(), does not that result in one too many calls to
> module_put() on error path?
> 

As far as I can see all that module_get() and module_put() do
is increment and decrement refcounters. Therefore, I think you can
have as many module_{get,put}() calls as you want on a path.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 2/2] net: phy: Ensure the MDIO bus module is held

[Florian Fainelli]

2014-07-23 11:40 GMT-07:00 Ezequiel Garcia <ezequiel.garcia@free-electrons.com>:
> On 23 Jul 11:22 AM, Florian Fainelli wrote:
>> 2014-07-22 17:16 GMT-07:00 Ezequiel Garcia <ezequiel.garcia@free-electrons.com>:
>> > @@ -614,10 +623,14 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>> >          */
>> >         err = phy_init_hw(phydev);
>> >         if (err)
>> > -               phy_detach(phydev);
>> > -       else
>> > -               phy_resume(phydev);
>> > +               goto err_module_put;
>> >
>> > +       phy_resume(phydev);
>> > +       return err;
>> > +
>> > +err_module_put:
>> > +       module_put(bus_module);
>> > +       phy_detach(phydev);
>>
>> Since we are calling phy_detach() which is also attempting to do a
>> module_put(), does not that result in one too many calls to
>> module_put() on error path?
>>
>
> As far as I can see all that module_get() and module_put() do
> is increment and decrement refcounters. Therefore, I think you can
> have as many module_{get,put}() calls as you want on a path.

Ok, I was concerned that in case that specific PHY device cannot be
registered, we might be decrementing the reference count twice, hence
making it 0. There might be another PHY device sitting on this same
MDIO bus, and since the reference count for the MDIO driver is now 0,
we could end up unloading that module.

Exercising that specific case is not probably easy to trigger.
-- 
Florian

Re: [PATCH 2/2] net: phy: Ensure the MDIO bus module is held

[Ezequiel Garcia]

On 23 Jul 03:40 PM, Ezequiel Garcia wrote:
> On 23 Jul 11:22 AM, Florian Fainelli wrote:
> > 2014-07-22 17:16 GMT-07:00 Ezequiel Garcia <ezequiel.garcia@free-electrons.com>:
> > > @@ -614,10 +623,14 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
> > >          */
> > >         err = phy_init_hw(phydev);
> > >         if (err)
> > > -               phy_detach(phydev);
> > > -       else
> > > -               phy_resume(phydev);
> > > +               goto err_module_put;
> > >
> > > +       phy_resume(phydev);
> > > +       return err;
> > > +
> > > +err_module_put:
> > > +       module_put(bus_module);
> > > +       phy_detach(phydev);
> > 
> > Since we are calling phy_detach() which is also attempting to do a
> > module_put(), does not that result in one too many calls to
> > module_put() on error path?
> > 
> 
> As far as I can see all that module_get() and module_put() do
> is increment and decrement refcounters. Therefore, I think you can
> have as many module_{get,put}() calls as you want on a path.

After reading this again, I know see what your pointing. The call to
phy_detach already puts the module, so we don't need to do it on the
error path above.

I'll cook up v2.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com