Re: [RFC PATCH 12/16] dsa: Make mdio bus optional

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  writes:

> On Fri, May 27, 2016 at 10:55:45AM -0400, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn  writes:
>> 
>> > -  mdiobus_unregister(ds->slave_mii_bus);
>> > +  if (ds->slave_mii_bus && ds->drv->phy_read)
>> > +  mdiobus_unregister(ds->slave_mii_bus);
>> 
>> So if a driver registered the slave MII bus itself, it may have
>> unregistered it itself as well, so checking ds->slave_mii_bus is OK
>> (assuming the driver correctly zero'ed it).
>> 
>> But is it necessary to check ds->drv->phy_read?
>
> It makes the code symmetrical to the register code, which makes the
> same check. My experience is that keeping the code symmetrical results
> in less surprises late on.
>
> One such surprise could be a driver that does not zero
> ds->slave_mii_bus. In fact, mv88e6xxx does not zero it. So we would
> get a double unregister happening. We also don't want the core
> unregistering it, since we have other cleanup work to do, we have an
> of_node_put() to call.

OK good for me, as long as it is intuitive for the driver
implementation.

Thanks,

Vivien

Re: [RFC PATCH 12/16] dsa: Make mdio bus optional

[Andrew Lunn]

On Fri, May 27, 2016 at 10:55:45AM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn  writes:
> 
> > -   mdiobus_unregister(ds->slave_mii_bus);
> > +   if (ds->slave_mii_bus && ds->drv->phy_read)
> > +   mdiobus_unregister(ds->slave_mii_bus);
> 
> So if a driver registered the slave MII bus itself, it may have
> unregistered it itself as well, so checking ds->slave_mii_bus is OK
> (assuming the driver correctly zero'ed it).
> 
> But is it necessary to check ds->drv->phy_read?

It makes the code symmetrical to the register code, which makes the
same check. My experience is that keeping the code symmetrical results
in less surprises late on.

One such surprise could be a driver that does not zero
ds->slave_mii_bus. In fact, mv88e6xxx does not zero it. So we would
get a double unregister happening. We also don't want the core
unregistering it, since we have other cleanup work to do, we have an
of_node_put() to call.

Andrew

Re: [RFC PATCH 12/16] dsa: Make mdio bus optional

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  writes:

> - mdiobus_unregister(ds->slave_mii_bus);
> + if (ds->slave_mii_bus && ds->drv->phy_read)
> + mdiobus_unregister(ds->slave_mii_bus);

So if a driver registered the slave MII bus itself, it may have
unregistered it itself as well, so checking ds->slave_mii_bus is OK
(assuming the driver correctly zero'ed it).

But is it necessary to check ds->drv->phy_read?

Thanks,

Vivien

[RFC PATCH 12/16] dsa: Make mdio bus optional

[Andrew Lunn]

The switch may want to instantiate its own MDIO bus. Only do it
centrally if the switch has not already created one, and the read op
is implemented.

Signed-off-by: Andrew Lunn 
---
 net/dsa/dsa.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 49cdf143c822..b1787e2f4bb3 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -340,17 +340,18 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, 
struct device *parent)
if (ret < 0)
goto out;
 
-   ds->slave_mii_bus = devm_mdiobus_alloc(parent);
-   if (ds->slave_mii_bus == NULL) {
-   ret = -ENOMEM;
-   goto out;
-   }
-   dsa_slave_mii_bus_init(ds);
-
-   ret = mdiobus_register(ds->slave_mii_bus);
-   if (ret < 0)
-   goto out;
+   if (!ds->slave_mii_bus && drv->phy_read) {
+   ds->slave_mii_bus = devm_mdiobus_alloc(parent);
+   if (!ds->slave_mii_bus) {
+   ret = -ENOMEM;
+   goto out;
+   }
+   dsa_slave_mii_bus_init(ds);
 
+   ret = mdiobus_register(ds->slave_mii_bus);
+   if (ret < 0)
+   goto out;
+   }
 
/*
 * Create network devices for physical switch ports.
@@ -493,7 +494,8 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
dsa_cpu_dsa_destroy(ds->ports[port].dn);
}
 
-   mdiobus_unregister(ds->slave_mii_bus);
+   if (ds->slave_mii_bus && ds->drv->phy_read)
+   mdiobus_unregister(ds->slave_mii_bus);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.8.1