Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[David Miller]

From: Florian Fainelli 
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 
> Reviewed-by: Florian Fainelli 
> 
> Thanks for fixing that.

Series applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[David Miller]

From: Florian Fainelli 
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 
> Reviewed-by: Florian Fainelli 
> 
> Thanks for fixing that.

Series applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Florian Fainelli]

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 
Reviewed-by: Florian Fainelli 

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(-)
> 

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Russell King - ARM Linux]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Russell King - ARM Linux]

On Thu, Sep 24, 2015 at 03:51:37PM -0700, David Miller wrote:
> From: Andrew Lunn 
> 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 
> >> 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 
> >> 
> >> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Andrew Lunn]

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[David Miller]

From: Andrew Lunn 
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 
>> 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 
>> 
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Andrew Lunn]

On Thu, Sep 24, 2015 at 03:15:54PM -0700, David Miller wrote:
> From: Andrew Lunn 
> 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 
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[David Miller]

From: Andrew Lunn 
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 

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Russell King - ARM Linux]

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 

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Andrew Lunn]

...

> 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 

Thanks
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[David Miller]

From: Andrew Lunn 
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 
>> 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 
>> 
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Russell King - ARM Linux]

On Thu, Sep 24, 2015 at 03:51:37PM -0700, David Miller wrote:
> From: Andrew Lunn 
> 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 
> >> 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 
> >> 
> >> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Andrew Lunn]

...

> 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 

Thanks
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[David Miller]

From: Andrew Lunn 
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 

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Russell King - ARM Linux]

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 

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Andrew Lunn]

On Thu, Sep 24, 2015 at 03:15:54PM -0700, David Miller wrote:
> From: Andrew Lunn 
> 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 
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Andrew Lunn]

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Russell King - ARM Linux]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/9] Phy, mdiobus, and netdev struct device fixes

[Florian Fainelli]

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 
Reviewed-by: Florian Fainelli 

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