Re: [PATCH net-next v2 2/3] net: mdio: realtek-rtl9300: Convert "fwnode" left-overs to "of"

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Non-equivalent API conversion in otto_emdio_probe(): replacing
  `device_for_each_child_node_scoped(dev, child)` with…

--

net: mdio: realtek-rtl9300: Convert "fwnode" left-overs to "of"

This converts the remaining fwnode based property/child-node accesses in
the rtl9300 MDIO driver over to the OF API, and re-indents the
continuation line of otto_emdio_probe_one().

[Severity: High]

Is this iterator swap intended to be behaviour preserving?  The two
iterators differ in whether they honour the DT status property.

The old one dispatched into the OF fwnode ops:

drivers/of/property.c:of_fwnode_get_next_child_node() {
	return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode),
							    to_of_node(child)));
}

The new one does not filter:

include/linux/of.h
#define for_each_child_of_node_scoped(parent, child) \
	for (struct device_node *child __free(device_node) =		\
	     of_get_next_child(parent, NULL);				\
	     child != NULL;						\
	     child = of_get_next_child(parent, child))

So nodes with status = "disabled" now reach otto_emdio_probe_one().

Does that turn a disabled bus child into a probe failure for the whole
controller?  A disabled mdio-bus@N node still has a valid reg, so it
passes the reg and num_buses checks and ends up in:

	err = devm_of_mdiobus_register(dev, bus, node);
	if (err)
		return dev_err_probe(dev, err, "cannot register MDIO bus\n");

which reaches:

drivers/net/mdio/of_mdio.c:__of_mdiobus_register() {
	/* Do not continue if the node is disabled */
	if (!of_device_is_available(np))
		return -ENODEV;

and otto_emdio_probe() propagates that error immediately:

	for_each_child_of_node_scoped(dev->of_node, child) {
		err = otto_emdio_probe_one(dev, priv, child);
		if (err)
			return err;
	}

Previously the disabled child was skipped and the enabled buses still
came up.

This looks reachable with in-tree DT: arch/mips/boot/dts/realtek/rtl930x.dtsi
ships all four mdio-bus@0..3 children with status = "disabled", so boards
that enable the controller plus only the buses they use would now fail
probe entirely.

There is also a CONFIG_OF_MDIO=n variant of the same concern: the stub
devm_of_mdiobus_register() in include/linux/of_mdio.h forwards to
devm_mdiobus_register() with no availability check, so a DT-disabled bus
would be registered and address-scanned instead.

Finally, the sibling OF walk in the same driver still filters on
availability:

	for_each_available_child_of_node_scoped(ports_dn, port_dn) {

so otto_emdio_map_ports() and the bus enumeration would no longer agree on
which nodes are usable.  Would for_each_available_child_of_node_scoped(dev->of_node, child)
be the equivalent conversion here?

Note this is still present at the end of the series, in otto_emdio_probe()
at e4ad7bb3a35d ("net: mdio: realtek-rtl9300: Support non-default clock
frequency").

 		err = otto_emdio_probe_one(dev, priv, child);
 		if (err)
 			return err;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906081946.3499898-1-markus.stockhausen%40gmx.de