| Message ID | 20240104140037.374166-5-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 68e1010cda7967cfca9c8650ee1f4efcae54ab90 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ds->user_mii_bus cleanup (part 1) | expand |
On Thu, Jan 04, 2024 at 04:00:31PM +0200, Vladimir Oltean wrote: > of_get_child_by_name() gives us an OF node with an elevated refcount, > which should be dropped when we're done with it. This is so that, > if (of_node_check_flag(node, OF_DYNAMIC)) is true, the node's memory can > eventually be freed. > > There are 2 distinct paths to be considered in qca8k_mdio_register(): > > - devm_of_mdiobus_register() succeeds: since commit 3b73a7b8ec38 ("net: > mdio_bus: add refcounting for fwnodes to mdiobus"), the MDIO core > treats this well. > > - devm_of_mdiobus_register() or anything up to that point fails: it is > the duty of the qca8k driver to release the OF node. > > This change addresses the second case by making sure that the OF node > reference is not leaked. > > The "mdio" node may be NULL, but of_node_put(NULL) is safe. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> > --- > drivers/net/dsa/qca/qca8k-8xxx.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c > index ec57d9d52072..5f47a290bd6e 100644 > --- a/drivers/net/dsa/qca/qca8k-8xxx.c > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c > @@ -949,10 +949,15 @@ qca8k_mdio_register(struct qca8k_priv *priv) > struct dsa_switch *ds = priv->ds; > struct device_node *mdio; > struct mii_bus *bus; > + int err; nit: besides qca8k_setup_mdio_bus(), the rest of the driver uses 'int ret'
On Thu, Jan 04, 2024 at 03:46:03PM +0000, Alvin Šipraga wrote: > Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> Thanks for the review, Alvin. > > --- > > drivers/net/dsa/qca/qca8k-8xxx.c | 21 ++++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c > > index ec57d9d52072..5f47a290bd6e 100644 > > --- a/drivers/net/dsa/qca/qca8k-8xxx.c > > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c > > @@ -949,10 +949,15 @@ qca8k_mdio_register(struct qca8k_priv *priv) > > struct dsa_switch *ds = priv->ds; > > struct device_node *mdio; > > struct mii_bus *bus; > > + int err; > > nit: besides qca8k_setup_mdio_bus(), the rest of the driver uses 'int ret' Yeah, good point. It wasn't on my mind as one of the things to check. If this is the only change that ends up being requested, I would prefer dealing with it separately at once in both places, rather than resending a series of 10 patches plus another one to also fix up qca8k_setup_mdio_bus().
On 1/4/24 06:00, Vladimir Oltean wrote: > of_get_child_by_name() gives us an OF node with an elevated refcount, > which should be dropped when we're done with it. This is so that, > if (of_node_check_flag(node, OF_DYNAMIC)) is true, the node's memory can > eventually be freed. > > There are 2 distinct paths to be considered in qca8k_mdio_register(): > > - devm_of_mdiobus_register() succeeds: since commit 3b73a7b8ec38 ("net: > mdio_bus: add refcounting for fwnodes to mdiobus"), the MDIO core > treats this well. > > - devm_of_mdiobus_register() or anything up to that point fails: it is > the duty of the qca8k driver to release the OF node. > > This change addresses the second case by making sure that the OF node > reference is not leaked. > > The "mdio" node may be NULL, but of_node_put(NULL) is safe. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> of_get_child_by_name() gives us an OF node with an elevated refcount, > which should be dropped when we're done with it. This is so that, > if (of_node_check_flag(node, OF_DYNAMIC)) is true, the node's memory can > eventually be freed. > > There are 2 distinct paths to be considered in qca8k_mdio_register(): > > - devm_of_mdiobus_register() succeeds: since commit 3b73a7b8ec38 ("net: > mdio_bus: add refcounting for fwnodes to mdiobus"), the MDIO core > treats this well. > > - devm_of_mdiobus_register() or anything up to that point fails: it is > the duty of the qca8k driver to release the OF node. In both cases, it is qca8k driver duty to put the OF node. 3b73a7b8ec38 just allows you to put it just after mdiobus_registration and not only after mdiobus_unregistration. This patch does put it correctly though. > This change addresses the second case by making sure that the OF node > reference is not leaked. > > The "mdio" node may be NULL, but of_node_put(NULL) is safe. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/dsa/qca/qca8k-8xxx.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c > index ec57d9d52072..5f47a290bd6e 100644 > --- a/drivers/net/dsa/qca/qca8k-8xxx.c > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c > @@ -949,10 +949,15 @@ qca8k_mdio_register(struct qca8k_priv *priv) > struct dsa_switch *ds = priv->ds; > struct device_node *mdio; > struct mii_bus *bus; > + int err; > + > + mdio = of_get_child_by_name(priv->dev->of_node, "mdio"); I couldn't get why you moved this here. It will only be used in of_device_is_available() > > bus = devm_mdiobus_alloc(ds->dev); > - if (!bus) > - return -ENOMEM; > + if (!bus) { > + err = -ENOMEM; > + goto out_put_node; > + } > > bus->priv = (void *)priv; > snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d.%d", > @@ -962,12 +967,12 @@ qca8k_mdio_register(struct qca8k_priv *priv) > ds->user_mii_bus = bus; > > /* Check if the devicetree declare the port:phy mapping */ > - mdio = of_get_child_by_name(priv->dev->of_node, "mdio"); > if (of_device_is_available(mdio)) { This is off-topic for this patch but it sounds strange to have an mdio node marked as disabled. The legacy code seems to cover old device-trees that do not have the mdio node. Supporting an existing but disabled mdio might point in the wrong direction. Anyway, it would be good to have common behavior across drivers. I can update the realtek driver to match whatever is the recommended usage. As a bonus, if a disabled node should return an error and not fallback to the legacy user mii, devm_mdiobus_register could be replaced by devm_of_mdiobus_register() as it handles mdio==NULL. > bus->name = "qca8k user mii"; > bus->read = qca8k_internal_mdio_read; > bus->write = qca8k_internal_mdio_write; > - return devm_of_mdiobus_register(priv->dev, bus, mdio); > + err = devm_of_mdiobus_register(priv->dev, bus, mdio); > + goto out_put_node; > } > > /* If a mapping can't be found the legacy mapping is used, > @@ -976,7 +981,13 @@ qca8k_mdio_register(struct qca8k_priv *priv) > bus->name = "qca8k-legacy user mii"; > bus->read = qca8k_legacy_mdio_read; > bus->write = qca8k_legacy_mdio_write; > - return devm_mdiobus_register(priv->dev, bus); > + > + err = devm_mdiobus_register(priv->dev, bus); > + > +out_put_node: > + of_node_put(mdio); > + > + return err; > } > > static int > -- > 2.34.1 > Regards, Luiz
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c index ec57d9d52072..5f47a290bd6e 100644 --- a/drivers/net/dsa/qca/qca8k-8xxx.c +++ b/drivers/net/dsa/qca/qca8k-8xxx.c @@ -949,10 +949,15 @@ qca8k_mdio_register(struct qca8k_priv *priv) struct dsa_switch *ds = priv->ds; struct device_node *mdio; struct mii_bus *bus; + int err; + + mdio = of_get_child_by_name(priv->dev->of_node, "mdio"); bus = devm_mdiobus_alloc(ds->dev); - if (!bus) - return -ENOMEM; + if (!bus) { + err = -ENOMEM; + goto out_put_node; + } bus->priv = (void *)priv; snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d.%d", @@ -962,12 +967,12 @@ qca8k_mdio_register(struct qca8k_priv *priv) ds->user_mii_bus = bus; /* Check if the devicetree declare the port:phy mapping */ - mdio = of_get_child_by_name(priv->dev->of_node, "mdio"); if (of_device_is_available(mdio)) { bus->name = "qca8k user mii"; bus->read = qca8k_internal_mdio_read; bus->write = qca8k_internal_mdio_write; - return devm_of_mdiobus_register(priv->dev, bus, mdio); + err = devm_of_mdiobus_register(priv->dev, bus, mdio); + goto out_put_node; } /* If a mapping can't be found the legacy mapping is used, @@ -976,7 +981,13 @@ qca8k_mdio_register(struct qca8k_priv *priv) bus->name = "qca8k-legacy user mii"; bus->read = qca8k_legacy_mdio_read; bus->write = qca8k_legacy_mdio_write; - return devm_mdiobus_register(priv->dev, bus); + + err = devm_mdiobus_register(priv->dev, bus); + +out_put_node: + of_node_put(mdio); + + return err; } static int
of_get_child_by_name() gives us an OF node with an elevated refcount, which should be dropped when we're done with it. This is so that, if (of_node_check_flag(node, OF_DYNAMIC)) is true, the node's memory can eventually be freed. There are 2 distinct paths to be considered in qca8k_mdio_register(): - devm_of_mdiobus_register() succeeds: since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes to mdiobus"), the MDIO core treats this well. - devm_of_mdiobus_register() or anything up to that point fails: it is the duty of the qca8k driver to release the OF node. This change addresses the second case by making sure that the OF node reference is not leaked. The "mdio" node may be NULL, but of_node_put(NULL) is safe. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/dsa/qca/qca8k-8xxx.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)