* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes
@ 2015-09-24 19:17 Russell King - ARM Linux
2015-09-24 19:18 ` [PATCH 6/9] net: fix phy refcounting in a bunch of drivers Russell King
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Russell King - ARM Linux @ 2015-09-24 19:17 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
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.
drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 24 ++++++----
drivers/net/ethernet/freescale/gianfar.c | 6 ++-
drivers/net/ethernet/freescale/ucc_geth.c | 8 +++-
drivers/net/ethernet/marvell/mvneta.c | 2 +
drivers/net/ethernet/xilinx/xilinx_emaclite.c | 2 +
drivers/net/phy/fixed_phy.c | 2 +-
drivers/net/phy/mdio-mux.c | 19 +++++---
drivers/net/phy/mdio_bus.c | 24 ++++++----
drivers/net/phy/phy_device.c | 62 ++++++++++++++++++++------
drivers/of/of_mdio.c | 27 +++++++++--
include/linux/phy.h | 6 ++-
net/core/net-sysfs.c | 9 ++++
net/dsa/dsa.c | 41 ++++++++++++++---
13 files changed, 181 insertions(+), 51 deletions(-)
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 6/9] net: fix phy refcounting in a bunch of drivers 2015-09-24 19:17 [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes Russell King - ARM Linux @ 2015-09-24 19:18 ` Russell King 2015-09-24 21:57 ` [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes Andrew Lunn 2015-09-25 1:39 ` Florian Fainelli 2 siblings, 0 replies; 12+ messages in thread From: Russell King @ 2015-09-24 19:18 UTC (permalink / raw) To: linux-arm-kernel of_phy_find_device() increments the phy struct device refcount, which we need to properly balance. Add code to network drivers using this function to ensure that the struct device refcount is correctly balanced. For xgene, looking back in the history, we should be able to use of_phy_connect() with a zero flags argument for the DT case as this is how the driver used to operate prior to de7b5b3d790a ("net: eth: xgene: change APM X-Gene SoC platform ethernet to support ACPI"). This leaves the Cavium Thunder BGX unfixed; fixing this driver is a complicated task, one which the maintainers need to be involved with. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 24 ++++++++++++++++-------- drivers/net/ethernet/freescale/gianfar.c | 3 +++ drivers/net/ethernet/freescale/ucc_geth.c | 8 +++++++- drivers/net/ethernet/marvell/mvneta.c | 2 ++ drivers/net/ethernet/xilinx/xilinx_emaclite.c | 2 ++ 5 files changed, 30 insertions(+), 9 deletions(-) diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c index cfa37041ab71..c4bb8027b3fb 100644 --- a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c @@ -689,16 +689,24 @@ 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 = pdata->phy_dev; + 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 || - phy_connect_direct(ndev, phy_dev, &xgene_enet_adjust_link, - pdata->phy_mode)) { - netdev_err(ndev, "Could not connect to PHY\n"); - return -ENODEV; + if (!phy_dev || + phy_connect_direct(ndev, phy_dev, &xgene_enet_adjust_link, + pdata->phy_mode)) { + netdev_err(ndev, "Could not connect to PHY\n"); + return -ENODEV; + } } pdata->phy_speed = SPEED_UNKNOWN; diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 4b69d061d90f..65a16086faec 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -1702,6 +1702,7 @@ static void gfar_configure_serdes(struct net_device *dev) tbiphy = of_phy_find_device(priv->tbi_node); if (!tbiphy) { dev_err(&dev->dev, "error: Could not get TBI device\n"); + put_device(&tbiphy->dev); return; } @@ -1723,6 +1724,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) diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c index 4dd40e057f40..650f7888e32b 100644 --- a/drivers/net/ethernet/freescale/ucc_geth.c +++ b/drivers/net/ethernet/freescale/ucc_geth.c @@ -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. diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index fe2299ac4f5c..0dce037a2682 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -3173,6 +3173,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; diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c b/drivers/net/ethernet/xilinx/xilinx_emaclite.c index 6008eee01a33..cf468c87ce57 100644 --- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c +++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c @@ -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; } -- 2.1.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes 2015-09-24 19:17 [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes Russell King - ARM Linux 2015-09-24 19:18 ` [PATCH 6/9] net: fix phy refcounting in a bunch of drivers Russell King @ 2015-09-24 21:57 ` Andrew Lunn 2015-09-24 22:15 ` Russell King - ARM Linux 2015-09-24 22:15 ` David Miller 2015-09-25 1:39 ` Florian Fainelli 2 siblings, 2 replies; 12+ messages in thread From: Andrew Lunn @ 2015-09-24 21:57 UTC (permalink / raw) To: linux-arm-kernel ... > 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. ... > The mdiobus code also suffered from the same kind of leak, but thankfully > this only happened in one place - the mdio-mux code. Hi Russell I tested both of these with my board. It is a Freescale Vybrid, using the FEC ethernet driver, and i have three switches attached, using mdio-mux to give three mdio busses. No obvious regressions, my board boots, the switches are all present and correct. I built the FEC driver as a module, and it won't unload: kernel:unregister_netdevice: waiting for eth1 to become free. Usage count = 1 unregister_netdevice: waiting for eth1 to become free. Usage count = 1 i assume because DSA holds a reference. I've not tried a fully module build, DSA has issues with that. Tested-by: Andrew Lunn <andrew@lunn.ch> Thanks Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes 2015-09-24 21:57 ` [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes Andrew Lunn @ 2015-09-24 22:15 ` Russell King - ARM Linux 2015-09-24 22:50 ` Andrew Lunn 2015-09-24 22:15 ` David Miller 1 sibling, 1 reply; 12+ messages in thread From: Russell King - ARM Linux @ 2015-09-24 22:15 UTC (permalink / raw) To: linux-arm-kernel On Thu, Sep 24, 2015 at 11:57:31PM +0200, Andrew Lunn wrote: > Hi Russell > > I tested both of these with my board. It is a Freescale Vybrid, using > the FEC ethernet driver, and i have three switches attached, using > mdio-mux to give three mdio busses. > > No obvious regressions, my board boots, the switches are all present > and correct. I built the FEC driver as a module, and it won't unload: > > kernel:unregister_netdevice: waiting for eth1 to become free. Usage count = 1 > unregister_netdevice: waiting for eth1 to become free. Usage count = 1 > > i assume because DSA holds a reference. I've not tried a fully module > build, DSA has issues with that. > > Tested-by: Andrew Lunn <andrew@lunn.ch> Thanks for testing. Please could you confirm whether the same behaviour is observed without the patches, just to make absolutely sure that isn't a regression. However, I think you are correct - I'm unable to locate where in the DSA code: - dst->master_dev's dev_hold() is undone (hence a reference left) - dst is freed - dsa_probe() allocates it using kzalloc(), but dsa_remove() and it's children don't free this structure. There's no notifier which detects whether the underlying device has gone away - it registers a netdev notifier (dsa_slave_netdevice_event) but this only deals with slave devices, not the master device. Thanks. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes 2015-09-24 22:15 ` Russell King - ARM Linux @ 2015-09-24 22:50 ` Andrew Lunn 2015-09-24 23:33 ` Russell King - ARM Linux 0 siblings, 1 reply; 12+ messages in thread From: Andrew Lunn @ 2015-09-24 22:50 UTC (permalink / raw) To: linux-arm-kernel > Thanks for testing. Please could you confirm whether the same behaviour > is observed without the patches, just to make absolutely sure that isn't > a regression. So i tested this now. I have two FEC interfaces. One i my main access interface, and the second is used by DSA to access switches. With your patches, the module Used by count is equal to the number of interfaces which are up. Without your patches, the count is always 0. When i try to remove the fec module, without your patches, but DSA still using the interface, i get the same kernel:unregister_netdevice: waiting for eth1 to become free. Usage count = 1 as with your patch. So this is not a regression. Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes 2015-09-24 22:50 ` Andrew Lunn @ 2015-09-24 23:33 ` Russell King - ARM Linux 0 siblings, 0 replies; 12+ messages in thread From: Russell King - ARM Linux @ 2015-09-24 23:33 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 25, 2015 at 12:50:33AM +0200, Andrew Lunn wrote: > > Thanks for testing. Please could you confirm whether the same behaviour > > is observed without the patches, just to make absolutely sure that isn't > > a regression. > > So i tested this now. > > I have two FEC interfaces. One i my main access interface, and the > second is used by DSA to access switches. With your patches, the > module Used by count is equal to the number of interfaces which are > up. > > Without your patches, the count is always 0. That will be as a result of the MDIO bus module refcounting patch - "phy: fix mdiobus module safety". The code prior to that patch was totally useless and ineffectual - it might as well not even have been present, because it would never have any effect. bus_module would always be NULL in phy_attach_direct(). While my change makes the code start to work as originally intended, it's still unsafe: there's nothing to stop you manually unbinding the driver providing the MDIO bus from the struct device. The driver will then free the resources it claimed in its probe function, which may include the MMIO mapping for the MDIO bus accessor functions. If the accessors are then called, despite keeping the mdio bus, phy, etc data structures properly refcounted, the kernel will oops when the (many) MDIO bus drivers hit the free'd MMIO mapping. This is, unfortunately, just another pre-existing bug in this code. To stop that, we need some way to say "this MDIO bus has been removed, prevent further access" and that needs to be done in a race free way. Right now, that doesn't exist. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes 2015-09-24 21:57 ` [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes Andrew Lunn 2015-09-24 22:15 ` Russell King - ARM Linux @ 2015-09-24 22:15 ` David Miller 2015-09-24 22:26 ` Andrew Lunn 1 sibling, 1 reply; 12+ messages in thread From: David Miller @ 2015-09-24 22:15 UTC (permalink / raw) To: linux-arm-kernel From: Andrew Lunn <andrew@lunn.ch> Date: Thu, 24 Sep 2015 23:57:31 +0200 > I built the FEC driver as a module, and it won't unload: > > kernel:unregister_netdevice: waiting for eth1 to become free. Usage count = 1 > unregister_netdevice: waiting for eth1 to become free. Usage count = 1 > > i assume because DSA holds a reference. I've not tried a fully module > build, DSA has issues with that. > > Tested-by: Andrew Lunn <andrew@lunn.ch> So, is this a regression? Please don't provide a "Tested-by: " tag is you encounter a new problem which could have been introduced by the changes in question. That _REALLY_ screws everything up for me. Thank. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes 2015-09-24 22:15 ` David Miller @ 2015-09-24 22:26 ` Andrew Lunn 2015-09-24 22:51 ` David Miller 0 siblings, 1 reply; 12+ messages in thread From: Andrew Lunn @ 2015-09-24 22:26 UTC (permalink / raw) To: linux-arm-kernel On Thu, Sep 24, 2015 at 03:15:54PM -0700, David Miller wrote: > From: Andrew Lunn <andrew@lunn.ch> > Date: Thu, 24 Sep 2015 23:57:31 +0200 > > > I built the FEC driver as a module, and it won't unload: > > > > kernel:unregister_netdevice: waiting for eth1 to become free. Usage count = 1 > > unregister_netdevice: waiting for eth1 to become free. Usage count = 1 > > > > i assume because DSA holds a reference. I've not tried a fully module > > build, DSA has issues with that. > > > > Tested-by: Andrew Lunn <andrew@lunn.ch> > > So, is this a regression? Sorry, worded that badly. Since DSA is still active, it should not be possible to unload the FEC driver. DSA should have a reference to it, and mdio-mux also should have a reference to the mdio bus of the FEC driver. As Russell requested, i will re-test without his patches, just to make sure. Andrew ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes 2015-09-24 22:26 ` Andrew Lunn @ 2015-09-24 22:51 ` David Miller 2015-09-24 23:12 ` Russell King - ARM Linux 0 siblings, 1 reply; 12+ messages in thread From: David Miller @ 2015-09-24 22:51 UTC (permalink / raw) To: linux-arm-kernel From: Andrew Lunn <andrew@lunn.ch> Date: Fri, 25 Sep 2015 00:26:54 +0200 > On Thu, Sep 24, 2015 at 03:15:54PM -0700, David Miller wrote: >> From: Andrew Lunn <andrew@lunn.ch> >> Date: Thu, 24 Sep 2015 23:57:31 +0200 >> >> > I built the FEC driver as a module, and it won't unload: >> > >> > kernel:unregister_netdevice: waiting for eth1 to become free. Usage count = 1 >> > unregister_netdevice: waiting for eth1 to become free. Usage count = 1 >> > >> > i assume because DSA holds a reference. I've not tried a fully module >> > build, DSA has issues with that. >> > >> > Tested-by: Andrew Lunn <andrew@lunn.ch> >> >> So, is this a regression? > > Sorry, worded that badly. Since DSA is still active, it should not be > possible to unload the FEC driver. DSA should have a reference to it, > and mdio-mux also should have a reference to the mdio bus of the FEC > driver. > > As Russell requested, i will re-test without his patches, just to make > sure. Something needs to hold into the underlying device refcount of a DSA blob so that an unload can't even be attempted in that state. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes 2015-09-24 22:51 ` David Miller @ 2015-09-24 23:12 ` Russell King - ARM Linux 0 siblings, 0 replies; 12+ messages in thread From: Russell King - ARM Linux @ 2015-09-24 23:12 UTC (permalink / raw) To: linux-arm-kernel On Thu, Sep 24, 2015 at 03:51:37PM -0700, David Miller wrote: > From: Andrew Lunn <andrew@lunn.ch> > Date: Fri, 25 Sep 2015 00:26:54 +0200 > > > On Thu, Sep 24, 2015 at 03:15:54PM -0700, David Miller wrote: > >> From: Andrew Lunn <andrew@lunn.ch> > >> Date: Thu, 24 Sep 2015 23:57:31 +0200 > >> > >> > I built the FEC driver as a module, and it won't unload: > >> > > >> > kernel:unregister_netdevice: waiting for eth1 to become free. Usage count = 1 > >> > unregister_netdevice: waiting for eth1 to become free. Usage count = 1 > >> > > >> > i assume because DSA holds a reference. I've not tried a fully module > >> > build, DSA has issues with that. > >> > > >> > Tested-by: Andrew Lunn <andrew@lunn.ch> > >> > >> So, is this a regression? > > > > Sorry, worded that badly. Since DSA is still active, it should not be > > possible to unload the FEC driver. DSA should have a reference to it, > > and mdio-mux also should have a reference to the mdio bus of the FEC > > driver. > > > > As Russell requested, i will re-test without his patches, just to make > > sure. > > Something needs to hold into the underlying device refcount of a DSA > blob so that an unload can't even be attempted in that state. Holding a reference on a struct device does _not_ stop the device being unbound or the module driving it being removed. It merely stops the struct device from being freed before all references have been dropped. Devices are always free to be unbound from their bound drivers irrespective of the struct device refcount. Even taking a reference on the module (via try_module_get()) does not stop this. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes 2015-09-24 19:17 [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes Russell King - ARM Linux 2015-09-24 19:18 ` [PATCH 6/9] net: fix phy refcounting in a bunch of drivers Russell King 2015-09-24 21:57 ` [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes Andrew Lunn @ 2015-09-25 1:39 ` Florian Fainelli 2015-09-25 6:05 ` David Miller 2 siblings, 1 reply; 12+ messages in thread From: Florian Fainelli @ 2015-09-25 1:39 UTC (permalink / raw) To: linux-arm-kernel On 24/09/15 12:17, Russell King - ARM Linux wrote: > Hi, > > 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.) Tested-by: Florian Fainelli <f.fainelli@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Thanks for fixing that. > > 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. > > drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 24 ++++++---- > drivers/net/ethernet/freescale/gianfar.c | 6 ++- > drivers/net/ethernet/freescale/ucc_geth.c | 8 +++- > drivers/net/ethernet/marvell/mvneta.c | 2 + > drivers/net/ethernet/xilinx/xilinx_emaclite.c | 2 + > drivers/net/phy/fixed_phy.c | 2 +- > drivers/net/phy/mdio-mux.c | 19 +++++--- > drivers/net/phy/mdio_bus.c | 24 ++++++---- > drivers/net/phy/phy_device.c | 62 ++++++++++++++++++++------ > drivers/of/of_mdio.c | 27 +++++++++-- > include/linux/phy.h | 6 ++- > net/core/net-sysfs.c | 9 ++++ > net/dsa/dsa.c | 41 ++++++++++++++--- > 13 files changed, 181 insertions(+), 51 deletions(-) > -- Florian ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes 2015-09-25 1:39 ` Florian Fainelli @ 2015-09-25 6:05 ` David Miller 0 siblings, 0 replies; 12+ messages in thread From: David Miller @ 2015-09-25 6:05 UTC (permalink / raw) To: linux-arm-kernel From: Florian Fainelli <f.fainelli@gmail.com> Date: Thu, 24 Sep 2015 18:39:56 -0700 > On 24/09/15 12:17, Russell King - ARM Linux wrote: >> Hi, >> >> 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.) > > Tested-by: Florian Fainelli <f.fainelli@gmail.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > Thanks for fixing that. Series applied, thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-09-25 6:05 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-24 19:17 [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes Russell King - ARM Linux 2015-09-24 19:18 ` [PATCH 6/9] net: fix phy refcounting in a bunch of drivers Russell King 2015-09-24 21:57 ` [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes Andrew Lunn 2015-09-24 22:15 ` Russell King - ARM Linux 2015-09-24 22:50 ` Andrew Lunn 2015-09-24 23:33 ` Russell King - ARM Linux 2015-09-24 22:15 ` David Miller 2015-09-24 22:26 ` Andrew Lunn 2015-09-24 22:51 ` David Miller 2015-09-24 23:12 ` Russell King - ARM Linux 2015-09-25 1:39 ` Florian Fainelli 2015-09-25 6:05 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).