Re: [PATCH net-next 3/3] net: mdio: realtek-rtl9300: Support non-default clock frequency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Using an uninitialized `bus->dev` pointer for logging before the MDIO bus is registered, resulting in a '(null)' prefix.
- [Low] The `global_freq` variable is overloaded as both an initialization flag and frequency storage, failing if `clock-frequency` is explicitly set to 0.
--

commit 9b57074cdf8cafdf9765568d20f29655308463cf
Author: Markus Stockhausen <markus.stockhausen@gmx.de>

net: mdio: realtek-rtl9300: Support non-default clock frequency

This commit enhances the realtek-rtl9300 MDIO driver to allow setting the
MDC clock frequency via device tree. For the RTL839x series, which only
supports global frequency selection, the driver configures the global
speed based on the first probed bus and issues warnings for conflicts.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
[ ... ]
> @@ -749,6 +766,34 @@ static int otto_emdio_8380_setup_controller(struct otto_emdio_priv *priv)
>  	return regmap_set_bits(priv->regmap, RTL8380_SMI_GLB_CTRL, RTL8380_SMI_PHY_PATCH_DONE);
>  }
>  
> +static int otto_emdio_8390_set_bus_frequency(struct mii_bus *bus, u32 freq)
> +{
> +	u32 val = FIELD_PREP(RTL8390_SMI_FREQ_SEL, min_t(u32, freq / 2500000, 2U));
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
> +
> +	if (priv->global_freq) {

[Severity: Low]
Does this logic correctly handle a scenario where the device tree explicitly
sets the clock-frequency to 0?

Since priv->global_freq is used as both an initialization flag and for
storing the frequency, a frequency of 0 would evaluate to false here.
This could bypass the conflict detection for subsequent buses and silently
overwrite the global hardware frequency.

Would a dedicated initialization flag or sentinel value (like -1) be more
robust to handle all integer inputs?

> +		/* With only one global frequency available skip setup for second bus. */
> +		if (freq != priv->global_freq)
> +			dev_warn(&bus->dev, "Only one frequency for all buses supported");

[Severity: Medium]
Will this dev_warn() print a (null) device name prefix?

Looking at otto_emdio_probe_one(), set_bus_frequency() is called before the
bus is registered via devm_of_mdiobus_register():

	err = priv->info->set_bus_frequency(bus, freq);
	if (err)
		return dev_err_probe(...);

	err = devm_of_mdiobus_register(dev, bus, node);

Because dev_set_name() hasn't been called on bus->dev yet, the logger won't
have a valid name to display. Could this be resolved by logging with
bus->parent instead?

> +		return 0;
> +	}
> +	priv->global_freq = freq;
> +
> +	return regmap_update_bits(priv->regmap, RTL8390_SMI_GLB_CTRL, RTL8390_SMI_FREQ_SEL, val);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903122119.2638354-1-markus.stockhausen@gmx.de?part=3