From: Russell King - ARM Linux <hidden> Date: 2015-09-24 19:18:17
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.
From: Russell King <hidden> Date: 2015-09-24 19:18:48
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 <redacted>
---
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(-)
@@ -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;
@@ -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;}
@@ -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);return0;}
From: Andrew Lunn <andrew@lunn.ch> Date: 2015-09-24 22:05:56
...
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
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.
From: Russell King - ARM Linux <hidden> Date: 2015-09-24 22:16:09
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.
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
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.
From: Andrew Lunn <andrew@lunn.ch> Date: 2015-09-24 22:58:52
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
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.
From: Russell King - ARM Linux <hidden> Date: 2015-09-24 23:33:27
On Fri, Sep 25, 2015 at 12:50:33AM +0200, Andrew Lunn wrote:
quoted
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.
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.)
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(-)
On 24/09/15 12:17, Russell King - ARM Linux wrote:
quoted
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.)