On Tue, Dec 06, 2016 at 08:54:43PM -0800, Florian Fainelli wrote:
> Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way we
> dealt with MDIO bus module reference count, but sort of introduced a
> regression in that, if an Ethernet driver registers its own MDIO bus
> driver, as is common, we will end up with the Ethernet driver's
> module->refnct set to 1, thus preventing this driver from any removal.
>
> Fix this by comparing the network device's device driver owner against
> the MDIO bus driver owner, and only if they are different, increment the
> MDIO bus module refcount.
>
> Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
> ---
> Russell,
>
> I verified this against the ethoc driver primarily (on a TS7300 board)
> and bcmgenet.
>
> Thanks!
>
> drivers/net/phy/phy_device.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 1a4bf8acad78..c4ceb082e970 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -857,11 +857,17 @@ EXPORT_SYMBOL(phy_attached_print);
> int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
> u32 flags, phy_interface_t interface)
> {
> + struct module *ndev_owner = dev->dev.parent->driver->owner;
Is this really safe? A driver does not need to set a parent device, and
in that case you get a NULL-deref here (I tried using cpsw).
> struct mii_bus *bus = phydev->mdio.bus;
> struct device *d = &phydev->mdio.dev;
> int err;
>
> - if (!try_module_get(bus->owner)) {
> + /* For Ethernet device drivers that register their own MDIO bus, we
> + * will have bus->owner match ndev_mod, so we do not want to increment
You also wanted s/ndev_mod/ndev_owner/ here.
> + * our own module->refcnt here, otherwise we would not be able to
> + * unload later on.
> + */
> + if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
> dev_err(&dev->dev, "failed to get the bus module\n");
> return -EIO;
Johan
