| Message ID | 1406074570-13246-3-git-send-email-ezequiel.garcia@free-electrons.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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 >
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.
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.
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.
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);
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(-)