[PATCH net-next] net: phy: mdio-boardinfo: Allow recursive mdiobus_register()

[PATCH net-next] net: phy: mdio-boardinfo: Allow recursive mdiobus_register()

[Andrew Lunn]

mdiobus_register will search for any mdiobus board info registered for
the bus being registered. If found, it will probe devices on the bus.
That device, if for example it is an ethernet switch, may then try to
register an mdio bus. Thus we need to allow recursive calls to
mdiobus_register.

Holding the mdio_board_lock will cause a deadlock during this
recursion. Release the lock and use list_for_each_entry_safe.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/mdio-boardinfo.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio-boardinfo.c b/drivers/net/phy/mdio-boardinfo.c
index 1861f387820d..863496fa5d13 100644
--- a/drivers/net/phy/mdio-boardinfo.c
+++ b/drivers/net/phy/mdio-boardinfo.c
@@ -30,17 +30,20 @@ void mdiobus_setup_mdiodev_from_board_info(struct mii_bus *bus,
 					    struct mdio_board_info *bi))
 {
 	struct mdio_board_entry *be;
+	struct mdio_board_entry *tmp;
 	struct mdio_board_info *bi;
 	int ret;
 
 	mutex_lock(&mdio_board_lock);
-	list_for_each_entry(be, &mdio_board_list, list) {
+	list_for_each_entry_safe(be, tmp, &mdio_board_list, list) {
 		bi = &be->board_info;
 
 		if (strcmp(bus->id, bi->bus_id))
 			continue;
 
+		mutex_unlock(&mdio_board_lock);
 		ret = cb(bus, bi);
+		mutex_lock(&mdio_board_lock);
 		if (ret)
 			continue;
 
-- 
2.17.0

Re: [PATCH net-next] net: phy: mdio-boardinfo: Allow recursive mdiobus_register()

[Florian Fainelli]

On 04/18/2018 05:00 PM, Andrew Lunn wrote:
> mdiobus_register will search for any mdiobus board info registered for
> the bus being registered. If found, it will probe devices on the bus.
> That device, if for example it is an ethernet switch, may then try to
> register an mdio bus. Thus we need to allow recursive calls to
> mdiobus_register.
> 
> Holding the mdio_board_lock will cause a deadlock during this
> recursion. Release the lock and use list_for_each_entry_safe.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/phy/mdio-boardinfo.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/mdio-boardinfo.c b/drivers/net/phy/mdio-boardinfo.c
> index 1861f387820d..863496fa5d13 100644
> --- a/drivers/net/phy/mdio-boardinfo.c
> +++ b/drivers/net/phy/mdio-boardinfo.c
> @@ -30,17 +30,20 @@ void mdiobus_setup_mdiodev_from_board_info(struct mii_bus *bus,
>  					    struct mdio_board_info *bi))
>  {
>  	struct mdio_board_entry *be;
> +	struct mdio_board_entry *tmp;
>  	struct mdio_board_info *bi;
>  	int ret;
>  
>  	mutex_lock(&mdio_board_lock);

Don't you need to drop that lock here?

> -	list_for_each_entry(be, &mdio_board_list, list) {
> +	list_for_each_entry_safe(be, tmp, &mdio_board_list, list) {
>  		bi = &be->board_info;
>  
>  		if (strcmp(bus->id, bi->bus_id))
>  			continue;
>  
> +		mutex_unlock(&mdio_board_lock);
>  		ret = cb(bus, bi);
> +		mutex_lock(&mdio_board_lock);
>  		if (ret)
>  			continue;
>  

And conversely drop the unlock from the end of this function?

Also, would you rather target "net" for this change since this appears
to be a bug fix?
-- 
Florian

Re: [PATCH net-next] net: phy: mdio-boardinfo: Allow recursive mdiobus_register()

[Andrew Lunn]

On Wed, Apr 18, 2018 at 05:14:36PM -0700, Florian Fainelli wrote:
> On 04/18/2018 05:00 PM, Andrew Lunn wrote:
> > mdiobus_register will search for any mdiobus board info registered for
> > the bus being registered. If found, it will probe devices on the bus.
> > That device, if for example it is an ethernet switch, may then try to
> > register an mdio bus. Thus we need to allow recursive calls to
> > mdiobus_register.
> > 
> > Holding the mdio_board_lock will cause a deadlock during this
> > recursion. Release the lock and use list_for_each_entry_safe.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  drivers/net/phy/mdio-boardinfo.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/mdio-boardinfo.c b/drivers/net/phy/mdio-boardinfo.c
> > index 1861f387820d..863496fa5d13 100644
> > --- a/drivers/net/phy/mdio-boardinfo.c
> > +++ b/drivers/net/phy/mdio-boardinfo.c
> > @@ -30,17 +30,20 @@ void mdiobus_setup_mdiodev_from_board_info(struct mii_bus *bus,
> >  					    struct mdio_board_info *bi))
> >  {
> >  	struct mdio_board_entry *be;
> > +	struct mdio_board_entry *tmp;
> >  	struct mdio_board_info *bi;
> >  	int ret;
> >  
> >  	mutex_lock(&mdio_board_lock);
> 
> Don't you need to drop that lock here?
> 
> > -	list_for_each_entry(be, &mdio_board_list, list) {
> > +	list_for_each_entry_safe(be, tmp, &mdio_board_list, list) {
> >  		bi = &be->board_info;
> >  
> >  		if (strcmp(bus->id, bi->bus_id))
> >  			continue;
> >  
> > +		mutex_unlock(&mdio_board_lock);
> >  		ret = cb(bus, bi);
> > +		mutex_lock(&mdio_board_lock);
> >  		if (ret)
> >  			continue;
> >  
> 
> And conversely drop the unlock from the end of this function?

No. The recursion happens inside the ret = cb(bus, bi). We need the
lock to be available during that. The lock itself is protecting the
list, so we need to hold the lock while using the list.

> Also, would you rather target "net" for this change since this appears
> to be a bug fix?

As far as i know, there is no in tree driver which can trigger
this. It requires the use of a switch instantiated using mdio_board
info, and the switch then needs to add another mdio bus in its probe
function. The only in tree code doing anything like this is dsa_loop,
and it does not register an mdio bus.

However, later this cycle i plan to add support for Zodiac's SCU3, and
it does trigger this deadlock. So net-next is sufficient for my.

   Andrew

Re: [PATCH net-next] net: phy: mdio-boardinfo: Allow recursive mdiobus_register()

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 19 Apr 2018 02:00:47 +0200

> mdiobus_register will search for any mdiobus board info registered for
> the bus being registered. If found, it will probe devices on the bus.
> That device, if for example it is an ethernet switch, may then try to
> register an mdio bus. Thus we need to allow recursive calls to
> mdiobus_register.
> 
> Holding the mdio_board_lock will cause a deadlock during this
> recursion. Release the lock and use list_for_each_entry_safe.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Applied.

While looking over this code I see that we currently never unregister
mdio boardinfo objects.

If we have drivers that can be unloaded, as it seems the one you plan
to add that needs this change should be, the situation could get more
tricky here.

Re: [PATCH net-next] net: phy: mdio-boardinfo: Allow recursive mdiobus_register()

[Andrew Lunn]

On Fri, Apr 20, 2018 at 10:34:30AM -0400, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Thu, 19 Apr 2018 02:00:47 +0200
> 
> > mdiobus_register will search for any mdiobus board info registered for
> > the bus being registered. If found, it will probe devices on the bus.
> > That device, if for example it is an ethernet switch, may then try to
> > register an mdio bus. Thus we need to allow recursive calls to
> > mdiobus_register.
> > 
> > Holding the mdio_board_lock will cause a deadlock during this
> > recursion. Release the lock and use list_for_each_entry_safe.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> Applied.
> 
> While looking over this code I see that we currently never unregister
> mdio boardinfo objects.
> 
> If we have drivers that can be unloaded, as it seems the one you plan
> to add that needs this change should be, the situation could get more
> tricky here.

Hi David

That is in fact normal, if you look at the i2c and spi versions of
this. This is/was generally used for ARM platforms, from before the
times of DT. There was a board file setting up platform devices for
the different bits of the hardware. The hardware never goes away, so
there is no need to remove the description of the hardware, what
devices are on which bus, etc. The drivers for that hardware can
however be removed.

The platform i'm working on is however x86. So i don't have a board
file as such, just a driver which gets loaded because of matches with
ACPI DMI strings. It populates this mdio boardinfo, as well as i2c
boardinfo, causing other drivers to be loaded. And i don't implement a
remove function, so the driver can never be unloaded. I'm happy with
that.

	Andrew