[PATCH net-next 00/10] ds->user_mii_bus cleanup (part 1)

[PATCH net-next 00/10] ds->user_mii_bus cleanup (part 1)

[Vladimir Oltean]

There are some drivers which assign ds->user_mii_bus when they
don't really need its specific functionality, aka non-OF based
dsa_user_phy_connect(). There was some confusion regarding the
fact that yes, this is why ds->user_mii_bus really exists, so
I've started a cleanup series which aims to eliminate the usage
of ds->user_mii_bus from drivers when there is nothing to gain
from it.

Today's drivers are lantiq_gswip, qca8k and bcm_sf2. The work is
not done here, but a "part 2" may or may not come, depending on
other priorities.

All patches were only compile-tested.

Vladimir Oltean (10):
  net: dsa: lantiq_gswip: delete irrelevant use of ds->phys_mii_mask
  net: dsa: lantiq_gswip: use devres for internal MDIO bus, not
    ds->user_mii_bus
  net: dsa: lantiq_gswip: ignore MDIO buses disabled in OF
  net: dsa: qca8k: put MDIO bus OF node on qca8k_mdio_register() failure
  net: dsa: qca8k: skip MDIO bus creation if its OF node has status =
    "disabled"
  net: dsa: qca8k: assign ds->user_mii_bus only for the non-OF case
  net: dsa: qca8k: consolidate calls to a single
    devm_of_mdiobus_register()
  net: dsa: qca8k: use "dev" consistently within qca8k_mdio_register()
  net: dsa: bcm_sf2: stop assigning an OF node to the ds->user_mii_bus
  net: dsa: bcm_sf2: drop priv->master_mii_dn

 drivers/net/dsa/bcm_sf2.c        |  7 ++--
 drivers/net/dsa/bcm_sf2.h        |  1 -
 drivers/net/dsa/lantiq_gswip.c   | 72 ++++++++++++++------------------
 drivers/net/dsa/qca/qca8k-8xxx.c | 47 +++++++++++++--------
 drivers/net/dsa/qca/qca8k-leds.c |  4 +-
 drivers/net/dsa/qca/qca8k.h      |  1 +
 6 files changed, 68 insertions(+), 64 deletions(-)

-- 
2.34.1

[PATCH net-next 01/10] net: dsa: lantiq_gswip: delete irrelevant use of ds->phys_mii_mask

[Vladimir Oltean]

__of_mdiobus_register(), called right next, overwrites the phy_mask
we just configured on the bus, so this is redundant and confusing.
Delete it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/lantiq_gswip.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 05a017c9ef3d..3494ad854cf6 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -521,7 +521,6 @@ static int gswip_mdio(struct gswip_priv *priv, struct device_node *mdio_np)
 	snprintf(ds->user_mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
 		 dev_name(priv->dev));
 	ds->user_mii_bus->parent = priv->dev;
-	ds->user_mii_bus->phy_mask = ~ds->phys_mii_mask;
 
 	err = of_mdiobus_register(ds->user_mii_bus, mdio_np);
 	if (err)
-- 
2.34.1

[PATCH net-next 02/10] net: dsa: lantiq_gswip: use devres for internal MDIO bus, not ds->user_mii_bus

[Vladimir Oltean]

This driver does not need any of the functionalities that make
ds->user_mii_bus special. Those use cases are listed here:
https://lore.kernel.org/netdev/20231221174746.hylsmr3f7g5byrsi@skbuf/

It just makes use of ds->user_mii_bus only as storage for its own MDIO
bus, which otherwise has no connection to the framework. This is because:

- the gswip driver only probes on OF: it fails if of_device_get_match_data()
  returns NULL

- when the child OF node of the MDIO bus is absent, no MDIO bus is
  registered at all, not even by the DSA framework. In order for that to
  have happened, the gswip driver would have needed to provide
  ->phy_read() and ->phy_write() in struct dsa_switch_ops, which it does
  not.

We can break the connection between the gswip driver and the DSA
framework and still preserve the same functionality.

Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
to mdiobus"), MDIO buses take ownership of the OF node handled to them,
and release it on their own. The gswip driver no longer needs to do
this.

Combine that with devres, and we no longer need to keep track of
anything for teardown purposes.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/lantiq_gswip.c | 69 +++++++++++++++-------------------
 1 file changed, 31 insertions(+), 38 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 3494ad854cf6..a514e6c78c38 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -505,26 +505,34 @@ static int gswip_mdio_rd(struct mii_bus *bus, int addr, int reg)
 	return gswip_mdio_r(priv, GSWIP_MDIO_READ);
 }
 
-static int gswip_mdio(struct gswip_priv *priv, struct device_node *mdio_np)
+static int gswip_mdio(struct gswip_priv *priv)
 {
-	struct dsa_switch *ds = priv->ds;
+	struct device_node *mdio_np, *switch_np = priv->dev->of_node;
+	struct device *dev = priv->dev;
+	struct mii_bus *bus;
 	int err;
 
-	ds->user_mii_bus = mdiobus_alloc();
-	if (!ds->user_mii_bus)
-		return -ENOMEM;
+	mdio_np = of_get_compatible_child(switch_np, "lantiq,xrx200-mdio");
+	if (!mdio_np)
+		return 0;
 
-	ds->user_mii_bus->priv = priv;
-	ds->user_mii_bus->read = gswip_mdio_rd;
-	ds->user_mii_bus->write = gswip_mdio_wr;
-	ds->user_mii_bus->name = "lantiq,xrx200-mdio";
-	snprintf(ds->user_mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
-		 dev_name(priv->dev));
-	ds->user_mii_bus->parent = priv->dev;
+	bus = devm_mdiobus_alloc(dev);
+	if (!bus) {
+		err = -ENOMEM;
+		goto out_put_node;
+	}
 
-	err = of_mdiobus_register(ds->user_mii_bus, mdio_np);
-	if (err)
-		mdiobus_free(ds->user_mii_bus);
+	bus->priv = priv;
+	bus->read = gswip_mdio_rd;
+	bus->write = gswip_mdio_wr;
+	bus->name = "lantiq,xrx200-mdio";
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev));
+	bus->parent = priv->dev;
+
+	err = devm_of_mdiobus_register(dev, bus, mdio_np);
+
+out_put_node:
+	of_node_put(mdio_np);
 
 	return err;
 }
@@ -2093,9 +2101,9 @@ static int gswip_gphy_fw_list(struct gswip_priv *priv,
 
 static int gswip_probe(struct platform_device *pdev)
 {
-	struct gswip_priv *priv;
-	struct device_node *np, *mdio_np, *gphy_fw_np;
+	struct device_node *np, *gphy_fw_np;
 	struct device *dev = &pdev->dev;
+	struct gswip_priv *priv;
 	int err;
 	int i;
 	u32 version;
@@ -2162,19 +2170,16 @@ static int gswip_probe(struct platform_device *pdev)
 	}
 
 	/* bring up the mdio bus */
-	mdio_np = of_get_compatible_child(dev->of_node, "lantiq,xrx200-mdio");
-	if (mdio_np) {
-		err = gswip_mdio(priv, mdio_np);
-		if (err) {
-			dev_err(dev, "mdio probe failed\n");
-			goto put_mdio_node;
-		}
+	err = gswip_mdio(priv);
+	if (err) {
+		dev_err(dev, "mdio probe failed\n");
+		goto gphy_fw_remove;
 	}
 
 	err = dsa_register_switch(priv->ds);
 	if (err) {
 		dev_err(dev, "dsa switch register failed: %i\n", err);
-		goto mdio_bus;
+		goto gphy_fw_remove;
 	}
 	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
 		dev_err(dev, "wrong CPU port defined, HW only supports port: %i",
@@ -2193,13 +2198,7 @@ static int gswip_probe(struct platform_device *pdev)
 disable_switch:
 	gswip_mdio_mask(priv, GSWIP_MDIO_GLOB_ENABLE, 0, GSWIP_MDIO_GLOB);
 	dsa_unregister_switch(priv->ds);
-mdio_bus:
-	if (mdio_np) {
-		mdiobus_unregister(priv->ds->user_mii_bus);
-		mdiobus_free(priv->ds->user_mii_bus);
-	}
-put_mdio_node:
-	of_node_put(mdio_np);
+gphy_fw_remove:
 	for (i = 0; i < priv->num_gphy_fw; i++)
 		gswip_gphy_fw_remove(priv, &priv->gphy_fw[i]);
 	return err;
@@ -2218,12 +2217,6 @@ static void gswip_remove(struct platform_device *pdev)
 
 	dsa_unregister_switch(priv->ds);
 
-	if (priv->ds->user_mii_bus) {
-		mdiobus_unregister(priv->ds->user_mii_bus);
-		of_node_put(priv->ds->user_mii_bus->dev.of_node);
-		mdiobus_free(priv->ds->user_mii_bus);
-	}
-
 	for (i = 0; i < priv->num_gphy_fw; i++)
 		gswip_gphy_fw_remove(priv, &priv->gphy_fw[i]);
 }
-- 
2.34.1

[PATCH net-next 03/10] net: dsa: lantiq_gswip: ignore MDIO buses disabled in OF

[Vladimir Oltean]

If the "lantiq,xrx200-mdio" child has status = "disabled", the MDIO bus
creation should be avoided. Use of_device_is_available() to check for
that, and take advantage of 2 facts:

- of_device_is_available(NULL) returns false
- of_node_put(NULL) is a no-op

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/lantiq_gswip.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index a514e6c78c38..de48b194048f 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -510,11 +510,11 @@ static int gswip_mdio(struct gswip_priv *priv)
 	struct device_node *mdio_np, *switch_np = priv->dev->of_node;
 	struct device *dev = priv->dev;
 	struct mii_bus *bus;
-	int err;
+	int err = 0;
 
 	mdio_np = of_get_compatible_child(switch_np, "lantiq,xrx200-mdio");
-	if (!mdio_np)
-		return 0;
+	if (!of_device_is_available(mdio_np))
+		goto out_put_node;
 
 	bus = devm_mdiobus_alloc(dev);
 	if (!bus) {
-- 
2.34.1

[PATCH net-next 04/10] net: dsa: qca8k: put MDIO bus OF node on qca8k_mdio_register() failure

[Vladimir Oltean]

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

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
-- 
2.34.1

[PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

[Vladimir Oltean]

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;
 
 	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

[PATCH net-next 06/10] net: dsa: qca8k: assign ds->user_mii_bus only for the non-OF case

[Vladimir Oltean]

To simplify reasoning about why the DSA framework provides the
ds->user_mii_bus functionality, drivers should only use it if they
need to. The qca8k driver appears to also use it simply as storage
for a pointer, which is not a good enough reason to make the core
much more difficult to follow.

ds->user_mii_bus is useful for only 2 cases:

1. The driver probes on platform_data (no OF)
2. The driver probes on OF, but there is no OF node for the MDIO bus.

It is unclear if case (1) is supported with qca8k. It might not be:
the driver might crash when of_device_get_match_data() returns NULL
and then it dereferences priv->info without NULL checking.

Anyway, let us limit the ds->user_mii_bus usage only to the above cases,
and not assign it when an OF node is present.

The bus->phy_mask assignment follows along with the movement, because
__of_mdiobus_register() overwrites this bus field anyway. The value set
by the driver only matters for the non-OF code path.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 5 +++--
 drivers/net/dsa/qca/qca8k-leds.c | 4 ++--
 drivers/net/dsa/qca/qca8k.h      | 1 +
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 21e36bc3c015..8f69b95c894d 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -961,12 +961,11 @@ qca8k_mdio_register(struct qca8k_priv *priv)
 		goto out_put_node;
 	}
 
+	priv->internal_mdio_bus = bus;
 	bus->priv = (void *)priv;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d.%d",
 		 ds->dst->index, ds->index);
 	bus->parent = ds->dev;
-	bus->phy_mask = ~ds->phys_mii_mask;
-	ds->user_mii_bus = bus;
 
 	/* Check if the devicetree declare the port:phy mapping */
 	if (mdio) {
@@ -980,6 +979,8 @@ qca8k_mdio_register(struct qca8k_priv *priv)
 	/* If a mapping can't be found the legacy mapping is used,
 	 * using the qca8k_port_to_phy function
 	 */
+	ds->user_mii_bus = bus;
+	bus->phy_mask = ~ds->phys_mii_mask;
 	bus->name = "qca8k-legacy user mii";
 	bus->read = qca8k_legacy_mdio_read;
 	bus->write = qca8k_legacy_mdio_write;
diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
index 90e30c2909e4..811ebeeff4ed 100644
--- a/drivers/net/dsa/qca/qca8k-leds.c
+++ b/drivers/net/dsa/qca/qca8k-leds.c
@@ -366,7 +366,6 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p
 {
 	struct fwnode_handle *led = NULL, *leds = NULL;
 	struct led_init_data init_data = { };
-	struct dsa_switch *ds = priv->ds;
 	enum led_default_state state;
 	struct qca8k_led *port_led;
 	int led_num, led_index;
@@ -429,7 +428,8 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p
 		init_data.default_label = ":port";
 		init_data.fwnode = led;
 		init_data.devname_mandatory = true;
-		init_data.devicename = kasprintf(GFP_KERNEL, "%s:0%d", ds->user_mii_bus->id,
+		init_data.devicename = kasprintf(GFP_KERNEL, "%s:0%d",
+						 priv->internal_mdio_bus->id,
 						 port_num);
 		if (!init_data.devicename)
 			return -ENOMEM;
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 2ac7e88f8da5..c8785c36c54e 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -454,6 +454,7 @@ struct qca8k_priv {
 	struct qca8k_ports_config ports_config;
 	struct regmap *regmap;
 	struct mii_bus *bus;
+	struct mii_bus *internal_mdio_bus;
 	struct dsa_switch *ds;
 	struct mutex reg_mutex;
 	struct device *dev;
-- 
2.34.1

[PATCH net-next 07/10] net: dsa: qca8k: consolidate calls to a single devm_of_mdiobus_register()

[Vladimir Oltean]

__of_mdiobus_register() already calls __mdiobus_register() if the
OF node provided as argument is NULL. We can take advantage of that
and simplify the 2 code path, calling devm_of_mdiobus_register() only
once for both cases.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 8f69b95c894d..f12bdb30796f 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -967,25 +967,23 @@ qca8k_mdio_register(struct qca8k_priv *priv)
 		 ds->dst->index, ds->index);
 	bus->parent = ds->dev;
 
-	/* Check if the devicetree declare the port:phy mapping */
 	if (mdio) {
+		/* Check if the device tree declares the port:phy mapping */
 		bus->name = "qca8k user mii";
 		bus->read = qca8k_internal_mdio_read;
 		bus->write = qca8k_internal_mdio_write;
-		err = devm_of_mdiobus_register(priv->dev, bus, mdio);
-		goto out_put_node;
+	} else {
+		/* If a mapping can't be found, the legacy mapping is used,
+		 * using qca8k_port_to_phy()
+		 */
+		ds->user_mii_bus = bus;
+		bus->phy_mask = ~ds->phys_mii_mask;
+		bus->name = "qca8k-legacy user mii";
+		bus->read = qca8k_legacy_mdio_read;
+		bus->write = qca8k_legacy_mdio_write;
 	}
 
-	/* If a mapping can't be found the legacy mapping is used,
-	 * using the qca8k_port_to_phy function
-	 */
-	ds->user_mii_bus = bus;
-	bus->phy_mask = ~ds->phys_mii_mask;
-	bus->name = "qca8k-legacy user mii";
-	bus->read = qca8k_legacy_mdio_read;
-	bus->write = qca8k_legacy_mdio_write;
-
-	err = devm_mdiobus_register(priv->dev, bus);
+	err = devm_of_mdiobus_register(priv->dev, bus, mdio);
 
 out_put_node:
 	of_node_put(mdio);
-- 
2.34.1

[PATCH net-next 08/10] net: dsa: qca8k: use "dev" consistently within qca8k_mdio_register()

[Vladimir Oltean]

Accessed either through priv->dev or ds->dev, it is the same device
structure. Keep a single variable which holds a reference to it, and use
it consistently.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index f12bdb30796f..c51f40960961 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -947,15 +947,16 @@ static int
 qca8k_mdio_register(struct qca8k_priv *priv)
 {
 	struct dsa_switch *ds = priv->ds;
+	struct device *dev = ds->dev;
 	struct device_node *mdio;
 	struct mii_bus *bus;
 	int err = 0;
 
-	mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
+	mdio = of_get_child_by_name(dev->of_node, "mdio");
 	if (mdio && !of_device_is_available(mdio))
 		goto out;
 
-	bus = devm_mdiobus_alloc(ds->dev);
+	bus = devm_mdiobus_alloc(dev);
 	if (!bus) {
 		err = -ENOMEM;
 		goto out_put_node;
@@ -965,7 +966,7 @@ qca8k_mdio_register(struct qca8k_priv *priv)
 	bus->priv = (void *)priv;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d.%d",
 		 ds->dst->index, ds->index);
-	bus->parent = ds->dev;
+	bus->parent = dev;
 
 	if (mdio) {
 		/* Check if the device tree declares the port:phy mapping */
@@ -983,7 +984,7 @@ qca8k_mdio_register(struct qca8k_priv *priv)
 		bus->write = qca8k_legacy_mdio_write;
 	}
 
-	err = devm_of_mdiobus_register(priv->dev, bus, mdio);
+	err = devm_of_mdiobus_register(dev, bus, mdio);
 
 out_put_node:
 	of_node_put(mdio);
-- 
2.34.1

[PATCH net-next 09/10] net: dsa: bcm_sf2: stop assigning an OF node to the ds->user_mii_bus

[Vladimir Oltean]

The bcm_sf2 driver does something strange. Instead of calling
of_mdiobus_register() with an OF node argument, it manually assigns the
bus->dev->of_node and then calls the non-OF mdiobus_register(). This
circumvents some code from __of_mdiobus_register() from running, which
sets the auto-scan mask, parses some device tree properties, etc.

I'm going to go out on a limb and say that the OF node isn't, in fact,
needed at all, and can be removed. The MDIO diversion as initially
implemented in commit 461cd1b03e32 ("net: dsa: bcm_sf2: Register our
slave MDIO bus") looked quite different than it is now, after commit
771089c2a485 ("net: dsa: bcm_sf2: Ensure that MDIO diversion is used").
Initially, it made sense, as bcm_sf2 was registering another set of
driver ops for the "brcm,unimac-mdio" OF node. But now, it deletes all
phandles, which makes "phy-handle"s unable to find PHYs, which means
that it always goes through the OF-unaware dsa_user_phy_connect().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/bcm_sf2.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index cadee5505c29..19b325fa5a27 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -635,7 +635,6 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds)
 	priv->user_mii_bus->write = bcm_sf2_sw_mdio_write;
 	snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "sf2-%d",
 		 index++);
-	priv->user_mii_bus->dev.of_node = dn;
 
 	/* Include the pseudo-PHY address to divert reads towards our
 	 * workaround. This is only required for 7445D0, since 7445E0
-- 
2.34.1

[PATCH net-next 10/10] net: dsa: bcm_sf2: drop priv->master_mii_dn

[Vladimir Oltean]

There used to be a of_node_put(priv->master_mii_dn) call in
bcm_sf2_mdio_unregister(), which was accidentally deleted in commit
6ca80638b90c ("net: dsa: Use conduit and user terms").

But it's not needed - we don't need to hold a reference on the
"brcm,unimac-mdio" OF node for that long, since we don't do anything
with it. We can release it as soon as we finish bcm_sf2_mdio_register().

Also reduce "if (err && dn)" to just "if (err)". We know "dn", aka the
former priv->master_mii_dn, is non-NULL. Otherwise, of_mdio_find_bus(dn)
would not have been able to find the bus behind "brcm,unimac-mdio".

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/bcm_sf2.c | 6 +++---
 drivers/net/dsa/bcm_sf2.h | 1 -
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 19b325fa5a27..4a52ccbe393f 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -621,8 +621,6 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds)
 		goto err_of_node_put;
 	}
 
-	priv->master_mii_dn = dn;
-
 	priv->user_mii_bus = mdiobus_alloc();
 	if (!priv->user_mii_bus) {
 		err = -ENOMEM;
@@ -682,9 +680,11 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds)
 	}
 
 	err = mdiobus_register(priv->user_mii_bus);
-	if (err && dn)
+	if (err)
 		goto err_free_user_mii_bus;
 
+	of_node_put(dn);
+
 	return 0;
 
 err_free_user_mii_bus:
diff --git a/drivers/net/dsa/bcm_sf2.h b/drivers/net/dsa/bcm_sf2.h
index 424f896b5a6f..f95f4880b69e 100644
--- a/drivers/net/dsa/bcm_sf2.h
+++ b/drivers/net/dsa/bcm_sf2.h
@@ -107,7 +107,6 @@ struct bcm_sf2_priv {
 
 	/* Master and slave MDIO bus controller */
 	unsigned int			indir_phy_mask;
-	struct device_node		*master_mii_dn;
 	struct mii_bus			*user_mii_bus;
 	struct mii_bus			*master_mii_bus;
 
-- 
2.34.1

Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

[Alvin Šipraga]

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
>

Re: [PATCH net-next 04/10] net: dsa: qca8k: put MDIO bus OF node on qca8k_mdio_register() failure

[Alvin Šipraga]

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'

Re: [PATCH net-next 06/10] net: dsa: qca8k: assign ds->user_mii_bus only for the non-OF case

[Alvin Šipraga]

On Thu, Jan 04, 2024 at 04:00:33PM +0200, Vladimir Oltean wrote:
> To simplify reasoning about why the DSA framework provides the
> ds->user_mii_bus functionality, drivers should only use it if they
> need to. The qca8k driver appears to also use it simply as storage
> for a pointer, which is not a good enough reason to make the core
> much more difficult to follow.
> 
> ds->user_mii_bus is useful for only 2 cases:
> 
> 1. The driver probes on platform_data (no OF)
> 2. The driver probes on OF, but there is no OF node for the MDIO bus.
> 
> It is unclear if case (1) is supported with qca8k. It might not be:
> the driver might crash when of_device_get_match_data() returns NULL
> and then it dereferences priv->info without NULL checking.
> 
> Anyway, let us limit the ds->user_mii_bus usage only to the above cases,
> and not assign it when an OF node is present.
> 
> The bus->phy_mask assignment follows along with the movement, because
> __of_mdiobus_register() overwrites this bus field anyway. The value set
> by the driver only matters for the non-OF code path.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ---
>  drivers/net/dsa/qca/qca8k-8xxx.c | 5 +++--
>  drivers/net/dsa/qca/qca8k-leds.c | 4 ++--
>  drivers/net/dsa/qca/qca8k.h      | 1 +
>  3 files changed, 6 insertions(+), 4 deletions(-)

Re: [PATCH net-next 07/10] net: dsa: qca8k: consolidate calls to a single devm_of_mdiobus_register()

[Alvin Šipraga]

On Thu, Jan 04, 2024 at 04:00:34PM +0200, Vladimir Oltean wrote:
> __of_mdiobus_register() already calls __mdiobus_register() if the
> OF node provided as argument is NULL. We can take advantage of that
> and simplify the 2 code path, calling devm_of_mdiobus_register() only
> once for both cases.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Much neater!

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ---
>  drivers/net/dsa/qca/qca8k-8xxx.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)

Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

[Vladimir Oltean]

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.

Re: [PATCH net-next 08/10] net: dsa: qca8k: use "dev" consistently within qca8k_mdio_register()

[Alvin Šipraga]

On Thu, Jan 04, 2024 at 04:00:35PM +0200, Vladimir Oltean wrote:
> Accessed either through priv->dev or ds->dev, it is the same device
> structure. Keep a single variable which holds a reference to it, and use
> it consistently.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ---
>  drivers/net/dsa/qca/qca8k-8xxx.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Re: [PATCH net-next 01/10] net: dsa: lantiq_gswip: delete irrelevant use of ds->phys_mii_mask

[Alvin Šipraga]

On Thu, Jan 04, 2024 at 04:00:28PM +0200, Vladimir Oltean wrote:
> __of_mdiobus_register(), called right next, overwrites the phy_mask
> we just configured on the bus, so this is redundant and confusing.
> Delete it.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ---
>  drivers/net/dsa/lantiq_gswip.c | 1 -
>  1 file changed, 1 deletion(-)

Re: [PATCH net-next 02/10] net: dsa: lantiq_gswip: use devres for internal MDIO bus, not ds->user_mii_bus

[Alvin Šipraga]

On Thu, Jan 04, 2024 at 04:00:29PM +0200, Vladimir Oltean wrote:
> This driver does not need any of the functionalities that make
> ds->user_mii_bus special. Those use cases are listed here:
> https://lore.kernel.org/netdev/20231221174746.hylsmr3f7g5byrsi@skbuf/
> 
> It just makes use of ds->user_mii_bus only as storage for its own MDIO
> bus, which otherwise has no connection to the framework. This is because:
> 
> - the gswip driver only probes on OF: it fails if of_device_get_match_data()
>   returns NULL
> 
> - when the child OF node of the MDIO bus is absent, no MDIO bus is
>   registered at all, not even by the DSA framework. In order for that to
>   have happened, the gswip driver would have needed to provide
>   ->phy_read() and ->phy_write() in struct dsa_switch_ops, which it does
>   not.
> 
> We can break the connection between the gswip driver and the DSA
> framework and still preserve the same functionality.
> 
> Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
> to mdiobus"), MDIO buses take ownership of the OF node handled to them,
> and release it on their own. The gswip driver no longer needs to do
> this.
> 
> Combine that with devres, and we no longer need to keep track of
> anything for teardown purposes.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ---
>  drivers/net/dsa/lantiq_gswip.c | 69 +++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 38 deletions(-)

Re: [PATCH net-next 03/10] net: dsa: lantiq_gswip: ignore MDIO buses disabled in OF

[Alvin Šipraga]

On Thu, Jan 04, 2024 at 04:00:30PM +0200, Vladimir Oltean wrote:
> If the "lantiq,xrx200-mdio" child has status = "disabled", the MDIO bus
> creation should be avoided. Use of_device_is_available() to check for
> that, and take advantage of 2 facts:
> 
> - of_device_is_available(NULL) returns false
> - of_node_put(NULL) is a no-op
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ---
>  drivers/net/dsa/lantiq_gswip.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Re: [PATCH net-next 09/10] net: dsa: bcm_sf2: stop assigning an OF node to the ds->user_mii_bus

[Alvin Šipraga]

On Thu, Jan 04, 2024 at 04:00:36PM +0200, Vladimir Oltean wrote:
> The bcm_sf2 driver does something strange. Instead of calling
> of_mdiobus_register() with an OF node argument, it manually assigns the
> bus->dev->of_node and then calls the non-OF mdiobus_register(). This
> circumvents some code from __of_mdiobus_register() from running, which
> sets the auto-scan mask, parses some device tree properties, etc.
> 
> I'm going to go out on a limb and say that the OF node isn't, in fact,
> needed at all, and can be removed. The MDIO diversion as initially
> implemented in commit 461cd1b03e32 ("net: dsa: bcm_sf2: Register our
> slave MDIO bus") looked quite different than it is now, after commit
> 771089c2a485 ("net: dsa: bcm_sf2: Ensure that MDIO diversion is used").
> Initially, it made sense, as bcm_sf2 was registering another set of
> driver ops for the "brcm,unimac-mdio" OF node. But now, it deletes all
> phandles, which makes "phy-handle"s unable to find PHYs, which means
> that it always goes through the OF-unaware dsa_user_phy_connect().
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ---
>  drivers/net/dsa/bcm_sf2.c | 1 -
>  1 file changed, 1 deletion(-)

Re: [PATCH net-next 10/10] net: dsa: bcm_sf2: drop priv->master_mii_dn

[Alvin Šipraga]

On Thu, Jan 04, 2024 at 04:00:37PM +0200, Vladimir Oltean wrote:
> There used to be a of_node_put(priv->master_mii_dn) call in
> bcm_sf2_mdio_unregister(), which was accidentally deleted in commit
> 6ca80638b90c ("net: dsa: Use conduit and user terms").
> 
> But it's not needed - we don't need to hold a reference on the
> "brcm,unimac-mdio" OF node for that long, since we don't do anything
> with it. We can release it as soon as we finish bcm_sf2_mdio_register().
> 
> Also reduce "if (err && dn)" to just "if (err)". We know "dn", aka the
> former priv->master_mii_dn, is non-NULL. Otherwise, of_mdio_find_bus(dn)
> would not have been able to find the bus behind "brcm,unimac-mdio".
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ---
>  drivers/net/dsa/bcm_sf2.c | 6 +++---
>  drivers/net/dsa/bcm_sf2.h | 1 -
>  2 files changed, 3 insertions(+), 4 deletions(-)

Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

[Alvin Šipraga]

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.

Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

[Alvin Šipraga]

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

Re: [PATCH net-next 01/10] net: dsa: lantiq_gswip: delete irrelevant use of ds->phys_mii_mask

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 330 bytes --]

On 1/4/24 06:00, Vladimir Oltean wrote:
> __of_mdiobus_register(), called right next, overwrites the phy_mask
> we just configured on the bus, so this is redundant and confusing.
> Delete it.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 04/10] net: dsa: qca8k: put MDIO bus OF node on qca8k_mdio_register() failure

[Vladimir Oltean]

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

Re: [PATCH net-next 09/10] net: dsa: bcm_sf2: stop assigning an OF node to the ds->user_mii_bus

[Florian Fainelli]

On 1/4/24 06:00, Vladimir Oltean wrote:
> The bcm_sf2 driver does something strange. Instead of calling
> of_mdiobus_register() with an OF node argument, it manually assigns the
> bus->dev->of_node and then calls the non-OF mdiobus_register(). This
> circumvents some code from __of_mdiobus_register() from running, which
> sets the auto-scan mask, parses some device tree properties, etc.
> 
> I'm going to go out on a limb and say that the OF node isn't, in fact,
> needed at all, and can be removed. The MDIO diversion as initially
> implemented in commit 461cd1b03e32 ("net: dsa: bcm_sf2: Register our
> slave MDIO bus") looked quite different than it is now, after commit
> 771089c2a485 ("net: dsa: bcm_sf2: Ensure that MDIO diversion is used").
> Initially, it made sense, as bcm_sf2 was registering another set of
> driver ops for the "brcm,unimac-mdio" OF node. But now, it deletes all
> phandles, which makes "phy-handle"s unable to find PHYs, which means
> that it always goes through the OF-unaware dsa_user_phy_connect().
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net-next 10/10] net: dsa: bcm_sf2: drop priv->master_mii_dn

[Florian Fainelli]

On 1/4/24 06:00, Vladimir Oltean wrote:
> There used to be a of_node_put(priv->master_mii_dn) call in
> bcm_sf2_mdio_unregister(), which was accidentally deleted in commit
> 6ca80638b90c ("net: dsa: Use conduit and user terms").
> 
> But it's not needed - we don't need to hold a reference on the
> "brcm,unimac-mdio" OF node for that long, since we don't do anything
> with it. We can release it as soon as we finish bcm_sf2_mdio_register().
> 
> Also reduce "if (err && dn)" to just "if (err)". We know "dn", aka the
> former priv->master_mii_dn, is non-NULL. Otherwise, of_mdio_find_bus(dn)
> would not have been able to find the bus behind "brcm,unimac-mdio".
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net-next 08/10] net: dsa: qca8k: use "dev" consistently within qca8k_mdio_register()

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

On 1/4/24 06:00, Vladimir Oltean wrote:
> Accessed either through priv->dev or ds->dev, it is the same device
> structure. Keep a single variable which holds a reference to it, and use
> it consistently.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 07/10] net: dsa: qca8k: consolidate calls to a single devm_of_mdiobus_register()

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

On 1/4/24 06:00, Vladimir Oltean wrote:
> __of_mdiobus_register() already calls __mdiobus_register() if the
> OF node provided as argument is NULL. We can take advantage of that
> and simplify the 2 code path, calling devm_of_mdiobus_register() only
> once for both cases.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 06/10] net: dsa: qca8k: assign ds->user_mii_bus only for the non-OF case

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On 1/4/24 06:00, Vladimir Oltean wrote:
> To simplify reasoning about why the DSA framework provides the
> ds->user_mii_bus functionality, drivers should only use it if they
> need to. The qca8k driver appears to also use it simply as storage
> for a pointer, which is not a good enough reason to make the core
> much more difficult to follow.
> 
> ds->user_mii_bus is useful for only 2 cases:
> 
> 1. The driver probes on platform_data (no OF)
> 2. The driver probes on OF, but there is no OF node for the MDIO bus.
> 
> It is unclear if case (1) is supported with qca8k. It might not be:
> the driver might crash when of_device_get_match_data() returns NULL
> and then it dereferences priv->info without NULL checking.
> 
> Anyway, let us limit the ds->user_mii_bus usage only to the above cases,
> and not assign it when an OF node is present.
> 
> The bus->phy_mask assignment follows along with the movement, because
> __of_mdiobus_register() overwrites this bus field anyway. The value set
> by the driver only matters for the non-OF code path.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 02/10] net: dsa: lantiq_gswip: use devres for internal MDIO bus, not ds->user_mii_bus

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

On 1/4/24 06:00, Vladimir Oltean wrote:
> This driver does not need any of the functionalities that make
> ds->user_mii_bus special. Those use cases are listed here:
> https://lore.kernel.org/netdev/20231221174746.hylsmr3f7g5byrsi@skbuf/
> 
> It just makes use of ds->user_mii_bus only as storage for its own MDIO
> bus, which otherwise has no connection to the framework. This is because:
> 
> - the gswip driver only probes on OF: it fails if of_device_get_match_data()
>    returns NULL
> 
> - when the child OF node of the MDIO bus is absent, no MDIO bus is
>    registered at all, not even by the DSA framework. In order for that to
>    have happened, the gswip driver would have needed to provide
>    ->phy_read() and ->phy_write() in struct dsa_switch_ops, which it does
>    not.
> 
> We can break the connection between the gswip driver and the DSA
> framework and still preserve the same functionality.
> 
> Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
> to mdiobus"), MDIO buses take ownership of the OF node handled to them,
> and release it on their own. The gswip driver no longer needs to do
> this.
> 
> Combine that with devres, and we no longer need to keep track of
> anything for teardown purposes.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 03/10] net: dsa: lantiq_gswip: ignore MDIO buses disabled in OF

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 446 bytes --]

On 1/4/24 06:00, Vladimir Oltean wrote:
> If the "lantiq,xrx200-mdio" child has status = "disabled", the MDIO bus
> creation should be avoided. Use of_device_is_available() to check for
> that, and take advantage of 2 facts:
> 
> - of_device_is_available(NULL) returns false
> - of_node_put(NULL) is a no-op
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 04/10] net: dsa: qca8k: put MDIO bus OF node on qca8k_mdio_register() failure

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 963 bytes --]

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>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

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>
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 08/10] net: dsa: qca8k: use "dev" consistently within qca8k_mdio_register()

[Christian Marangi]

On Thu, Jan 04, 2024 at 04:00:35PM +0200, Vladimir Oltean wrote:
> Accessed either through priv->dev or ds->dev, it is the same device
> structure. Keep a single variable which holds a reference to it, and use
> it consistently.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Christian Marangi <ansuelsmth@gmail.com>

Re: [PATCH net-next 07/10] net: dsa: qca8k: consolidate calls to a single devm_of_mdiobus_register()

[Christian Marangi]

On Thu, Jan 04, 2024 at 04:00:34PM +0200, Vladimir Oltean wrote:
> __of_mdiobus_register() already calls __mdiobus_register() if the
> OF node provided as argument is NULL. We can take advantage of that
> and simplify the 2 code path, calling devm_of_mdiobus_register() only
> once for both cases.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Christian Marangi <ansuelsmth@gmail.com>

-- 
	Ansuel

Re: [PATCH net-next 04/10] net: dsa: qca8k: put MDIO bus OF node on qca8k_mdio_register() failure

[Luiz Angelo Daros de Luca]

> 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

Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

[Luiz Angelo Daros de Luca]

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
>

Re: [PATCH net-next 08/10] net: dsa: qca8k: use "dev" consistently within qca8k_mdio_register()

[Luiz Angelo Daros de Luca]

> Accessed either through priv->dev or ds->dev, it is the same device
> structure. Keep a single variable which holds a reference to it, and use
> it consistently.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Re: [PATCH net-next 07/10] net: dsa: qca8k: consolidate calls to a single devm_of_mdiobus_register()

[Luiz Angelo Daros de Luca]

> __of_mdiobus_register() already calls __mdiobus_register() if the
> OF node provided as argument is NULL. We can take advantage of that
> and simplify the 2 code path, calling devm_of_mdiobus_register() only
> once for both cases.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

I should have read all the series before... :-)

Reviewed-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Re: [PATCH net-next 06/10] net: dsa: qca8k: assign ds->user_mii_bus only for the non-OF case

[Luiz Angelo Daros de Luca]

> To simplify reasoning about why the DSA framework provides the
> ds->user_mii_bus functionality, drivers should only use it if they
> need to. The qca8k driver appears to also use it simply as storage
> for a pointer, which is not a good enough reason to make the core
> much more difficult to follow.
>
> ds->user_mii_bus is useful for only 2 cases:
>
> 1. The driver probes on platform_data (no OF)
> 2. The driver probes on OF, but there is no OF node for the MDIO bus.
>
> It is unclear if case (1) is supported with qca8k. It might not be:
> the driver might crash when of_device_get_match_data() returns NULL
> and then it dereferences priv->info without NULL checking.

There are plenty of places that will not work without OF. For example,
qca8k_setup_mdio_bus will return -EINVAL without ports or
ethernet-ports in the device-tree.
That error will abort qca8k_setup. I think it is safe to assume case (2).

> Anyway, let us limit the ds->user_mii_bus usage only to the above cases,
> and not assign it when an OF node is present.
>
> The bus->phy_mask assignment follows along with the movement, because
> __of_mdiobus_register() overwrites this bus field anyway. The value set
> by the driver only matters for the non-OF code path.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Re: [PATCH net-next 02/10] net: dsa: lantiq_gswip: use devres for internal MDIO bus, not ds->user_mii_bus

[Luiz Angelo Daros de Luca]

> This driver does not need any of the functionalities that make
> ds->user_mii_bus special. Those use cases are listed here:
> https://lore.kernel.org/netdev/20231221174746.hylsmr3f7g5byrsi@skbuf/
>
> It just makes use of ds->user_mii_bus only as storage for its own MDIO
> bus, which otherwise has no connection to the framework. This is because:
>
> - the gswip driver only probes on OF: it fails if of_device_get_match_data()
>   returns NULL
>
> - when the child OF node of the MDIO bus is absent, no MDIO bus is
>   registered at all, not even by the DSA framework. In order for that to
>   have happened, the gswip driver would have needed to provide
>   ->phy_read() and ->phy_write() in struct dsa_switch_ops, which it does
>   not.
>
> We can break the connection between the gswip driver and the DSA
> framework and still preserve the same functionality.
>
> Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
> to mdiobus"), MDIO buses take ownership of the OF node handled to them,
> and release it on their own. The gswip driver no longer needs to do
> this.
>
> Combine that with devres, and we no longer need to keep track of
> anything for teardown purposes.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Re: [PATCH net-next 10/10] net: dsa: bcm_sf2: drop priv->master_mii_dn

[Luiz Angelo Daros de Luca]

> There used to be a of_node_put(priv->master_mii_dn) call in
> bcm_sf2_mdio_unregister(), which was accidentally deleted in commit
> 6ca80638b90c ("net: dsa: Use conduit and user terms").
>
> But it's not needed - we don't need to hold a reference on the
> "brcm,unimac-mdio" OF node for that long, since we don't do anything
> with it. We can release it as soon as we finish bcm_sf2_mdio_register().
>
> Also reduce "if (err && dn)" to just "if (err)". We know "dn", aka the
> former priv->master_mii_dn, is non-NULL. Otherwise, of_mdio_find_bus(dn)
> would not have been able to find the bus behind "brcm,unimac-mdio".
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Re: [PATCH net-next 00/10] ds->user_mii_bus cleanup (part 1)

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu,  4 Jan 2024 16:00:27 +0200 you wrote:
> There are some drivers which assign ds->user_mii_bus when they
> don't really need its specific functionality, aka non-OF based
> dsa_user_phy_connect(). There was some confusion regarding the
> fact that yes, this is why ds->user_mii_bus really exists, so
> I've started a cleanup series which aims to eliminate the usage
> of ds->user_mii_bus from drivers when there is nothing to gain
> from it.
> 
> [...]

Here is the summary with links:
  - [net-next,01/10] net: dsa: lantiq_gswip: delete irrelevant use of ds->phys_mii_mask
    https://git.kernel.org/netdev/net-next/c/fc74b32b4032
  - [net-next,02/10] net: dsa: lantiq_gswip: use devres for internal MDIO bus, not ds->user_mii_bus
    https://git.kernel.org/netdev/net-next/c/cd4ba3ecced9
  - [net-next,03/10] net: dsa: lantiq_gswip: ignore MDIO buses disabled in OF
    https://git.kernel.org/netdev/net-next/c/7a898539391d
  - [net-next,04/10] net: dsa: qca8k: put MDIO bus OF node on qca8k_mdio_register() failure
    https://git.kernel.org/netdev/net-next/c/68e1010cda79
  - [net-next,05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"
    https://git.kernel.org/netdev/net-next/c/e66bf63a7f67
  - [net-next,06/10] net: dsa: qca8k: assign ds->user_mii_bus only for the non-OF case
    https://git.kernel.org/netdev/net-next/c/525366b81f33
  - [net-next,07/10] net: dsa: qca8k: consolidate calls to a single devm_of_mdiobus_register()
    https://git.kernel.org/netdev/net-next/c/5c5d6b34b683
  - [net-next,08/10] net: dsa: qca8k: use "dev" consistently within qca8k_mdio_register()
    https://git.kernel.org/netdev/net-next/c/c4a1cefdf3bc
  - [net-next,09/10] net: dsa: bcm_sf2: stop assigning an OF node to the ds->user_mii_bus
    https://git.kernel.org/netdev/net-next/c/04a4bc9dddc7
  - [net-next,10/10] net: dsa: bcm_sf2: drop priv->master_mii_dn
    https://git.kernel.org/netdev/net-next/c/45f62ca5cc48

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

[Vladimir Oltean]

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.