Re: [PATCH net] phy: Don't increment MDIO bus refcount unless it's a different owner

On 12/08/2016 08:27 AM, Johan Hovold wrote:

On Tue, Dec 06, 2016 at 08:54:43PM -0800, Florian Fainelli wrote:

quoted

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.fainelli@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).

Humm, cpsw does call SET_NETDEV_DEV() which should take care of that, is
the call made too late? Do you have an example oops?

I don't mind safeguarding this with a check against dev->dev.parent, but
I would like to fix the drivers where relevant too, since
SET_NETDEV_DEV() should really be called, otherwise a number of things
just don't work

quoted

 	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.

Meh, it's merged now, but thanks, I will fix this once we find out the
proper solution for cpsw.
-- 
Florian