Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit b626ef01 authored by David S. Miller's avatar David S. Miller
Browse files

Merge branch 'phy-mdio-refcnt'



Russell King says:

====================
Phy, mdiobus, and netdev struct device fixes

The third version of this series fixes the build error which David
identified, and drops the broken changes for the Cavium Thunger BGX
ethernet driver as this driver requires some complex changes to
resolve the leakage - and this is best done by people who can test
the driver.

Compared to v2, the only patch which has changed is patch 6
  "net: fix phy refcounting in a bunch of drivers"

I _think_ I've been able to build-test all the drivers touched by
that patch to some degree now, though several of them needed the
Kconfig hacked to allow it (not all had || COMPILE_TEST clause on
their dependencies.)

Previous cover letters below:

This is the second version of the series, with the comments David had
on the first patch fixed up.  Original series description with updated
diffstat below.

While looking at the DSA code, I noticed we have a
of_find_net_device_by_node(), and it looks like users of that are
similarly buggy - it looks like net/dsa/dsa.c is the only user.  Fix
that too.

Hi,

While looking at the phy code, I identified a number of weaknesses
where refcounting on device structures was being leaked, where
modules could be removed while in-use, and where the fixed-phy could
end up having unintended consequences caused by incorrect calls to
fixed_phy_update_state().

This patch series resolves those issues, some of which were discovered
with testing on an Armada 388 board.  Not all patches are fully tested,
particularly the one which touches several network drivers.

When resolving the struct device refcounting problems, several different
solutions were considered before settling on the implementation here -
one of the considerations was to avoid touching many network drivers.
The solution here is:

	phy_attach*() - takes a refcount
	phy_detach*() - drops the phy_attach refcount

Provided drivers always attach and detach their phys, which they should
already be doing, this should change nothing, even if they leak a refcount.

	of_phy_find_device() and of_* functions which use that take
	a refcount.  Arrange for this refcount to be dropped once
	the phy is attached.

This is the reason why the previous change is important - we can't drop
this refcount taken by of_phy_find_device() until something else holds
a reference on the device.  This resolves the leaked refcount caused by
using of_phy_connect() or of_phy_attach().

Even without the above changes, these drivers are leaking by calling
of_phy_find_device().  These drivers are addressed by adding the
appropriate release of that refcount.

The mdiobus code also suffered from the same kind of leak, but thankfully
this only happened in one place - the mdio-mux code.

I also found that the try_module_get() in the phy layer code was utterly
useless: phydev->dev.driver was guaranteed to always be NULL, so
try_module_get() was always being called with a NULL argument.  I proved
this with my SFP code, which declares its own MDIO bus - the module use
count was never incremented irrespective of how I set the MDIO bus up.
This allowed the MDIO bus code to be removed from the kernel while there
were still PHYs attached to it.

One other bug was discovered: while using in-band-status with mvneta, it
was found that if a real phy is attached with in-band-status enabled,
and another ethernet interface is using the fixed-phy infrastructure, the
interface using the fixed-phy infrastructure is configured according to
the other interface using the in-band-status - which is caused by the
fixed-phy code not verifying that the phy_device passed in is actually
a fixed-phy device, rather than a real MDIO phy.

Lastly, having mdio_bus reversing phy_device_register() internals seems
like a layering violation - it's trivial to move that code to the phy
device layer.
====================

Tested-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parents 17a10c92 9861f720
Loading
Loading
Loading
Loading
+16 −8
Original line number Diff line number Diff line
@@ -689,9 +689,16 @@ static int xgene_enet_phy_connect(struct net_device *ndev)
			netdev_dbg(ndev, "No phy-handle found in DT\n");
			return -ENODEV;
		}
		pdata->phy_dev = of_phy_find_device(phy_np);

		phy_dev = of_phy_connect(ndev, phy_np, &xgene_enet_adjust_link,
					 0, pdata->phy_mode);
		if (!phy_dev) {
			netdev_err(ndev, "Could not connect to PHY\n");
			return -ENODEV;
		}

		pdata->phy_dev = phy_dev;
	} else {
		phy_dev = pdata->phy_dev;

		if (!phy_dev ||
@@ -700,6 +707,7 @@ static int xgene_enet_phy_connect(struct net_device *ndev)
			netdev_err(ndev, "Could not connect to PHY\n");
			return  -ENODEV;
		}
	}

	pdata->phy_speed = SPEED_UNKNOWN;
	phy_dev->supported &= ~SUPPORTED_10baseT_Half &
+5 −1
Original line number Diff line number Diff line
@@ -1710,8 +1710,10 @@ static void gfar_configure_serdes(struct net_device *dev)
	 * everything for us?  Resetting it takes the link down and requires
	 * several seconds for it to come back.
	 */
	if (phy_read(tbiphy, MII_BMSR) & BMSR_LSTATUS)
	if (phy_read(tbiphy, MII_BMSR) & BMSR_LSTATUS) {
		put_device(&tbiphy->dev);
		return;
	}

	/* Single clk mode, mii mode off(for serdes communication) */
	phy_write(tbiphy, MII_TBICON, TBICON_CLK_SELECT);
@@ -1723,6 +1725,8 @@ static void gfar_configure_serdes(struct net_device *dev)
	phy_write(tbiphy, MII_BMCR,
		  BMCR_ANENABLE | BMCR_ANRESTART | BMCR_FULLDPLX |
		  BMCR_SPEED1000);

	put_device(&tbiphy->dev);
}

static int __gfar_is_rx_idle(struct gfar_private *priv)
+7 −1
Original line number Diff line number Diff line
@@ -1384,6 +1384,8 @@ static int adjust_enet_interface(struct ucc_geth_private *ugeth)
		value = phy_read(tbiphy, ENET_TBI_MII_CR);
		value &= ~0x1000;	/* Turn off autonegotiation */
		phy_write(tbiphy, ENET_TBI_MII_CR, value);

		put_device(&tbiphy->dev);
	}

	init_check_frame_length_mode(ug_info->lengthCheckRx, &ug_regs->maccfg2);
@@ -1702,8 +1704,10 @@ static void uec_configure_serdes(struct net_device *dev)
	 * everything for us?  Resetting it takes the link down and requires
	 * several seconds for it to come back.
	 */
	if (phy_read(tbiphy, ENET_TBI_MII_SR) & TBISR_LSTATUS)
	if (phy_read(tbiphy, ENET_TBI_MII_SR) & TBISR_LSTATUS) {
		put_device(&tbiphy->dev);
		return;
	}

	/* Single clk mode, mii mode off(for serdes communication) */
	phy_write(tbiphy, ENET_TBI_MII_ANA, TBIANA_SETTINGS);
@@ -1711,6 +1715,8 @@ static void uec_configure_serdes(struct net_device *dev)
	phy_write(tbiphy, ENET_TBI_MII_TBICON, TBICON_CLK_SELECT);

	phy_write(tbiphy, ENET_TBI_MII_CR, TBICR_SETTINGS);

	put_device(&tbiphy->dev);
}

/* Configure the PHY for dev.
+2 −0
Original line number Diff line number Diff line
@@ -3175,6 +3175,8 @@ static int mvneta_probe(struct platform_device *pdev)
		struct phy_device *phy = of_phy_find_device(dn);

		mvneta_fixed_link_update(pp, phy);

		put_device(&phy->dev);
	}

	return 0;
+2 −0
Original line number Diff line number Diff line
@@ -828,6 +828,8 @@ static int xemaclite_mdio_setup(struct net_local *lp, struct device *dev)
		if (!phydev)
			dev_info(dev,
				 "MDIO of the phy is not registered yet\n");
		else
			put_device(&phydev->dev);
		return 0;
	}

Loading