| Message ID | 20240104140037.374166-6-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e66bf63a7f670a600338faf889bed71f90c183bf |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ds->user_mii_bus cleanup (part 1) | expand |
On Thu, Jan 04, 2024 at 04:00:32PM +0200, Vladimir Oltean wrote: > Currently the driver calls the non-OF devm_mdiobus_register() rather > than devm_of_mdiobus_register() for this case, but it seems to rather > be a confusing coincidence, and not a real use case that needs to be > supported. I am not really sure about the use case, but I always thought that status = "disabled" sort of functions the same as if the node were simply never specified. But with your change, there is a behavioural difference between these two cases: (a) mdio unspecified => register "qca8k-legacy user mii" (b) mdio specified, but status = "disabled" => don't register anything Was this your intention? > > If the device tree says status = "disabled" for the MDIO bus, we > shouldn't need an MDIO bus at all. Instead, just exit as early as > possible and do not call any MDIO API. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/dsa/qca/qca8k-8xxx.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c > index 5f47a290bd6e..21e36bc3c015 100644 > --- a/drivers/net/dsa/qca/qca8k-8xxx.c > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c > @@ -949,9 +949,11 @@ qca8k_mdio_register(struct qca8k_priv *priv) > struct dsa_switch *ds = priv->ds; > struct device_node *mdio; > struct mii_bus *bus; > - int err; > + int err = 0; > > mdio = of_get_child_by_name(priv->dev->of_node, "mdio"); > + if (mdio && !of_device_is_available(mdio)) > + goto out; > > bus = devm_mdiobus_alloc(ds->dev); > if (!bus) { > @@ -967,7 +969,7 @@ qca8k_mdio_register(struct qca8k_priv *priv) > ds->user_mii_bus = bus; > > /* Check if the devicetree declare the port:phy mapping */ > - if (of_device_is_available(mdio)) { > + if (mdio) { > bus->name = "qca8k user mii"; > bus->read = qca8k_internal_mdio_read; > bus->write = qca8k_internal_mdio_write; > @@ -986,7 +988,7 @@ qca8k_mdio_register(struct qca8k_priv *priv) > > out_put_node: > of_node_put(mdio); > - > +out: > return err; > } > > -- > 2.34.1 >
On Thu, Jan 04, 2024 at 03:44:48PM +0000, Alvin Šipraga wrote: > On Thu, Jan 04, 2024 at 04:00:32PM +0200, Vladimir Oltean wrote: > > Currently the driver calls the non-OF devm_mdiobus_register() rather > > than devm_of_mdiobus_register() for this case, but it seems to rather > > be a confusing coincidence, and not a real use case that needs to be > > supported. > > I am not really sure about the use case, but I always thought that > status = "disabled" sort of functions the same as if the node were > simply never specified. But with your change, there is a behavioural > difference between these two cases: > > (a) mdio unspecified => register "qca8k-legacy user mii" > (b) mdio specified, but status = "disabled" => don't register anything > > Was this your intention? Yeah, it was my intention. I'm not sure if I agree with your equivalence. For example, PCI devices probe through enumeration. Their OF node is optional, aka when absent, they still probe. But when an associated OF node exists and has status = "disabled", they don't probe.
On Thu, Jan 04, 2024 at 05:49:27PM +0200, Vladimir Oltean wrote: > On Thu, Jan 04, 2024 at 03:44:48PM +0000, Alvin Šipraga wrote: > > On Thu, Jan 04, 2024 at 04:00:32PM +0200, Vladimir Oltean wrote: > > > Currently the driver calls the non-OF devm_mdiobus_register() rather > > > than devm_of_mdiobus_register() for this case, but it seems to rather > > > be a confusing coincidence, and not a real use case that needs to be > > > supported. > > > > I am not really sure about the use case, but I always thought that > > status = "disabled" sort of functions the same as if the node were > > simply never specified. But with your change, there is a behavioural > > difference between these two cases: > > > > (a) mdio unspecified => register "qca8k-legacy user mii" > > (b) mdio specified, but status = "disabled" => don't register anything > > > > Was this your intention? > > Yeah, it was my intention. I'm not sure if I agree with your equivalence. > For example, PCI devices probe through enumeration. Their OF node is > optional, aka when absent, they still probe. But when an associated OF > node exists and has status = "disabled", they don't probe. Yes, good example with PCIe. The change makes sense then. Thanks for clarifying.
On Thu, Jan 04, 2024 at 04:00:32PM +0200, Vladimir Oltean wrote: > Currently the driver calls the non-OF devm_mdiobus_register() rather > than devm_of_mdiobus_register() for this case, but it seems to rather > be a confusing coincidence, and not a real use case that needs to be > supported. > > If the device tree says status = "disabled" for the MDIO bus, we > shouldn't need an MDIO bus at all. Instead, just exit as early as > possible and do not call any MDIO API. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> > --- > drivers/net/dsa/qca/qca8k-8xxx.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-)
On 1/4/24 06:00, Vladimir Oltean wrote: > Currently the driver calls the non-OF devm_mdiobus_register() rather > than devm_of_mdiobus_register() for this case, but it seems to rather > be a confusing coincidence, and not a real use case that needs to be > supported. > > If the device tree says status = "disabled" for the MDIO bus, we > shouldn't need an MDIO bus at all. Instead, just exit as early as > possible and do not call any MDIO API. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Em qui., 4 de jan. de 2024 às 11:01, Vladimir Oltean <vladimir.oltean@nxp.com> escreveu: > > Currently the driver calls the non-OF devm_mdiobus_register() rather > than devm_of_mdiobus_register() for this case, but it seems to rather > be a confusing coincidence, and not a real use case that needs to be > supported. > > If the device tree says status = "disabled" for the MDIO bus, we > shouldn't need an MDIO bus at all. Instead, just exit as early as > possible and do not call any MDIO API. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/dsa/qca/qca8k-8xxx.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c > index 5f47a290bd6e..21e36bc3c015 100644 > --- a/drivers/net/dsa/qca/qca8k-8xxx.c > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c > @@ -949,9 +949,11 @@ qca8k_mdio_register(struct qca8k_priv *priv) > struct dsa_switch *ds = priv->ds; > struct device_node *mdio; > struct mii_bus *bus; > - int err; > + int err = 0; > > mdio = of_get_child_by_name(priv->dev->of_node, "mdio"); > + if (mdio && !of_device_is_available(mdio)) > + goto out; Hum.. that's why you moved this call here in the previous patch. Don't you still need to put the node that is not available? Just put it unconditionally whenever you exit this function after you get it. of_node_put() can handle even NULL. I'm not sure if this and other simple switches can be useful without a valid MDIO. Anyway, wouldn't it be equivalent to having an empty mdio node? It looks like it would work as well but without a specific code path. > bus = devm_mdiobus_alloc(ds->dev); > if (!bus) { > @@ -967,7 +969,7 @@ qca8k_mdio_register(struct qca8k_priv *priv) > ds->user_mii_bus = bus; > > /* Check if the devicetree declare the port:phy mapping */ > - if (of_device_is_available(mdio)) { > + if (mdio) { > bus->name = "qca8k user mii"; > bus->read = qca8k_internal_mdio_read; > bus->write = qca8k_internal_mdio_write; > @@ -986,7 +988,7 @@ qca8k_mdio_register(struct qca8k_priv *priv) > > out_put_node: > of_node_put(mdio); > - > +out: > return err; > } > > -- > 2.34.1 >
On Thu, Jan 04, 2024 at 11:19:20PM -0300, Luiz Angelo Daros de Luca wrote: > Don't you still need to put the node that is not available? Just put > it unconditionally whenever you exit this function after you get it. > of_node_put() can handle even NULL. You're right. I've prepared a patch to handle this case correctly. I don't think it's worth sending to 'net' now that 'net-next' has closed, because as you say below, it's quite possible that the !of_device_is_available() code path is never exercised by existing device trees. So the bug is like the tree that falls in the forest but nobody hears it. I will submit the correction when net-next reopens, together with Alvin's suggested "err" -> "ret" renaming. > I'm not sure if this and other simple switches can be useful without a > valid MDIO. Will that always be the case? As implausible as this may sound, I've received DSA questions from people using the sja1105 as a two-port adapter between MII on the CPU side and RMII on the PHY side. It was the cheapest way of adapting their SoC to RMII, using a switch as not even a port multiplier. I see that AR8237 has 1x RGMII and 1x SerDes, so maybe somebody would want to use it that way, and sidestep the internal PHYs? I don't know. > Anyway, wouldn't it be equivalent to having an empty mdio > node? It looks like it would work as well but without a specific code > path. I guess you could also express this that way too. Although, in case it matters, an 'empty node' has to pass schema validation (has to have all required properties), and a disabled one doesn't. The idea with this patch was to deliberately change the status = "disabled" handling that the driver already had, to make things more consistent across the board. Each driver binding has its own unique interpretation of an absent MDIO OF node already. Some consider the OF node optional and thus register it anyway, some say that absent means it's not needed. But I think status = "disabled" should be an unambiguous way to specify through DT that the hardware component is disabled. This is not how qca8k was interpreting it prior to this change.
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c index 5f47a290bd6e..21e36bc3c015 100644 --- a/drivers/net/dsa/qca/qca8k-8xxx.c +++ b/drivers/net/dsa/qca/qca8k-8xxx.c @@ -949,9 +949,11 @@ qca8k_mdio_register(struct qca8k_priv *priv) struct dsa_switch *ds = priv->ds; struct device_node *mdio; struct mii_bus *bus; - int err; + int err = 0; mdio = of_get_child_by_name(priv->dev->of_node, "mdio"); + if (mdio && !of_device_is_available(mdio)) + goto out; bus = devm_mdiobus_alloc(ds->dev); if (!bus) { @@ -967,7 +969,7 @@ qca8k_mdio_register(struct qca8k_priv *priv) ds->user_mii_bus = bus; /* Check if the devicetree declare the port:phy mapping */ - if (of_device_is_available(mdio)) { + if (mdio) { bus->name = "qca8k user mii"; bus->read = qca8k_internal_mdio_read; bus->write = qca8k_internal_mdio_write; @@ -986,7 +988,7 @@ qca8k_mdio_register(struct qca8k_priv *priv) out_put_node: of_node_put(mdio); - +out: return err; }
Currently the driver calls the non-OF devm_mdiobus_register() rather than devm_of_mdiobus_register() for this case, but it seems to rather be a confusing coincidence, and not a real use case that needs to be supported. If the device tree says status = "disabled" for the MDIO bus, we shouldn't need an MDIO bus at all. Instead, just exit as early as possible and do not call any MDIO API. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/dsa/qca/qca8k-8xxx.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)