Re: [PATCH net-next v14 10/13] net: mdio: realtek-rtl9300: Open up C22 and C45 space in parallel

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

Pre-existing issues:
- [High] The MDIO read/write functions return `-ENOENT` for unmapped PHY addresses and `-ENXIO` on hardware command failure, which improperly aborts the entire MDIO bus registration.
--

--- Patch [10]: [PATCH net-next v14 10/13] net: mdio: realtek-rtl9300: Open up C22 and C45 space in parallel ---
commit c559542aa88ff474f3367ec89a5df414f866718f
Author: Markus Stockhausen <[email protected]>

net: mdio: realtek-rtl9300: Open up C22 and C45 space in parallel

This commit enables parallel C22 and C45 access for the Realtek RTL9300 MDIO
driver. It does this by unconditionally assigning both the C22 and C45 read
and write bus operations during probe.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index f3777c61705aa..c417a2e3d613d 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
[ ... ]
> @@ -683,13 +683,10 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
>  		return -ENOMEM;
>  
>  	bus->name = "Realtek Switch MDIO Bus";
> -	if (priv->smi_bus_is_c45[mdio_bus]) {
> -		bus->read_c45 = otto_emdio_read_c45;
> -		bus->write_c45 = otto_emdio_write_c45;
> -	} else {
> -		bus->read = otto_emdio_read_c22;
> -		bus->write = otto_emdio_write_c22;
> -	}
> +	bus->read_c45 = otto_emdio_read_c45;
> +	bus->write_c45 = otto_emdio_write_c45;
> +	bus->read = otto_emdio_read_c22;
> +	bus->write = otto_emdio_write_c22;

[Severity: High]
This is a pre-existing issue, but does the MDIO read/write implementation
improperly abort the entire MDIO bus registration when encountering unmapped
PHY addresses or hardware command failures?

Looking at otto_emdio_phy_to_port(), if a PHY address isn't mapped, it returns
-ENOENT:

drivers/net/mdio/mdio-realtek-rtl9300.c:otto_emdio_phy_to_port() {
    ...
    for_each_set_bit(i, priv->valid_ports, priv->info->num_ports)
        if (priv->smi_bus[i] == chan->mdio_bus &&
            priv->smi_addr[i] == phy_id)
            return i;

    return -ENOENT;
}

Similarly, if a hardware timeout occurs, otto_emdio_run_cmd() returns -ENXIO:

drivers/net/mdio/mdio-realtek-rtl9300.c:otto_emdio_run_cmd() {
    ...
    return cmdstate & info->cmd_fail ? -ENXIO : 0;
}

When the generic PHY subsystem (like get_phy_c22_id) encounters these
non-standard errors during mdiobus_read(), it transforms them into a fatal
-EIO. Because -EIO is not -ENODEV, __of_mdiobus_register() will immediately
unregister the bus and exit, preventing the switch and all its network ports
from initializing.

Could returning -ENODEV instead be safer so that missing PHYs are simply
skipped instead of failing the entire bus?

>  	bus->parent = dev;
>  	bus->notify_phy_attach = otto_emdio_notify_phy_attach;
>  	bus->notify_phy_detach = otto_emdio_notify_phy_detach;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10