Re: [net-next,v15,07/13] net: mdio: realtek-rtl9300: Configure hardware polling during probing

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] Asymmetric polling control: otto_emdio_init_polling()
  (drivers/net/mdio/mdio-realtek-rtl9300.c:786) clears the chip-wide…
- [Low] No rollback of the chip-wide polling state on probe failure and
  no teardown counterpart.
- [Low] Concern that otto_emdio_set_port_polling() writes the SMI
  polling-control register without holding priv->lock, allowing polling…

--

net: mdio: realtek-rtl9300: Configure hardware polling during probing

This adds a poll_ctrl register offset per chip variant and a helper to
toggle the per-port hardware polling bit. Polling is disabled for all
ports at probe time, re-enabled from a new notify_phy_attach callback and
disabled again from notify_phy_detach.

References given in the commit message:
[1] https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/files-6.18/drivers/net/mdio/mdio-realtek-otto.c#L818
[2] https://lore.kernel.org/netdev/680696024a8648535ce6dee771fe4de67802e0e8.1769053496.git.daniel@makrotopia.org/

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index 7de8adec243ba..a925640290cb0 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c

[ ... ]

> @@ -253,6 +256,12 @@ static struct otto_emdio_priv *otto_emdio_bus_to_priv(struct mii_bus *bus)
>  	return chan->priv;
>  }
>  
> +static int otto_emdio_set_port_polling(struct otto_emdio_priv *priv, int port, bool active)
> +{
> +	return regmap_assign_bits(priv->regmap, priv->info->poll_ctrl + (port / 32) * 4,
> +				  BIT(port % 32), active);
> +}

[Severity: Low]
A question about the claim in the commit message that "Enabling/disabling
hardware polling is a simple regmap bit access. For this it is enough to
rely on the regmap internal lock." otto_emdio_set_port_polling() runs
without priv->lock, so a polling toggle for one port can land while an
indirect command sequence for another port is in flight.

Reading the file again this looks fine, since priv->lock only covers the
indirect command register sequence:

    otto_emdio_read_cmd()/otto_emdio_write_cmd() {
        lockdep_assert_held(&priv->lock);

and poll_ctrl (0xca90 / 0x0ccc) is not one of the command registers
(c22_data/c45_data/io_data/port_mask_low/port_mask_high/broadcast/ext_page),
while regmap_assign_bits() does the read-modify-write under the regmap
lock. Also, in steady state the polling bits of all attached ports are
already set while commands run, so taking priv->lock around the toggle
would not change the interference behaviour.

Is that reasoning correct, i.e. is the intended invariant "priv->lock
covers the command sequence registers only" rather than "all hardware
access"?

[ ... ]

> @@ -596,6 +605,36 @@ static int otto_emdio_9310_setup_controller(struct otto_emdio_priv *priv)
>  	return 0;
>  }
>  
> +static int otto_emdio_notify_phy_attach(struct phy_device *phydev)
> +{
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(phydev->mdio.bus);
> +	int port = otto_emdio_phy_to_port(phydev->mdio.bus, phydev->mdio.addr);
> +
> +	if (port < 0) {
> +		/* All subsequent bus operations will fail */
> +		phydev_err(phydev, "PHY is not mapped to a valid switch port\n");
> +		return port;
> +	}
> +
> +	return otto_emdio_set_port_polling(priv, port, true);
> +}

[ ... ]

> @@ -741,6 +783,19 @@ static int otto_emdio_map_ports(struct device *dev)
>  	return err;
>  }
>  
> +static int otto_emdio_init_polling(struct otto_emdio_priv *priv)
> +{
> +	int err;
> +
> +	for (int port = 0; port < priv->info->num_ports; port++) {
> +		err = otto_emdio_set_port_polling(priv, port, false);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Is the enable/disable pair symmetric here?

otto_emdio_init_polling() clears the polling bit for every port from 0 to
info->num_ports - 1, but the only place a bit is ever set again is
otto_emdio_notify_phy_attach(), which is reached only from
phy_attach_direct() and returns early when otto_emdio_phy_to_port() cannot
map the address:

    if (port < 0) {
        /* All subsequent bus operations will fail */
        phydev_err(phydev, "PHY is not mapped to a valid switch port\n");
        return port;
    }

priv->valid_ports is filled only from ethernet-ports children that carry a
phy-handle:

    otto_emdio_map_ports() {
        ...
        phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
        /* skip ports without phys */
        if (!phy_dn)
            continue;

So can a port that never gets a phydev attached ever have its polling bit
restored? cameo-rtl9302c-2x-rtl8224-2xge.dts has port@24 and port@25 with
phy-mode = "10gbase-r" and no phy-handle, which look like concrete
instances of that.

The file header states the constraint this affects:

 * - Without the polling and a proper MAC status, some of the link handling features do not work.
 *   Especially an unpopulated MAC_LINK_STS register cancels operations to other MAC registers.

The commit message says "Disable polling for all ports when the MDIO driver
starts" and "it brings the hardware polling into a consistent operation mode
for devices where U-Boot does not take care", but does not mention that
ports without a kernel-managed PHY are never re-enabled and that the
bootloader mask is overwritten without any save/restore. Could that be
spelled out, or the loop restricted to priv->valid_ports?

Note there is no in-tree MAC or DSA driver for these SoCs today, so on a
pure mainline kernel notify_phy_attach() never runs and the net effect of
loading this driver is that polling stays off for all ports. That makes the
functional impact only demonstrable with an out-of-tree or future MAC
driver, but the asymmetry in the code is visible as is.

>  static int otto_emdio_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -760,6 +815,10 @@ static int otto_emdio_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->regmap))
>  		return PTR_ERR(priv->regmap);
>  
> +	err = otto_emdio_init_polling(priv);
> +	if (err)
> +		return err;
> +
>  	platform_set_drvdata(pdev, priv);
>  
>  	err = otto_emdio_map_ports(dev);

[Severity: Low]
Should the polling mask be restored when probe fails?

otto_emdio_init_polling() runs right after the syscon regmap is obtained,
before platform_set_drvdata() and before otto_emdio_map_ports(),
otto_emdio_setup_topology(), info->setup_controller() and
otto_emdio_probe_one()/devm_of_mdiobus_register(), each of which does a
plain "return err".

The bits live in the shared switch syscon regmap rather than in
driver-private memory, so devres unwinding does not undo them, and the
driver has neither a .remove callback nor a devm_add_action() for this.
Does a non-deferred probe failure (missing ethernet-ports node, illegal or
duplicate port number, devm_mdiobus_alloc_size() returning -ENOMEM,
devm_of_mdiobus_register() failing) leave the switch with polling disabled
for all ports with nothing to restore it?

The same applies mid-loop in otto_emdio_init_polling():

    err = otto_emdio_set_port_polling(priv, port, false);
    if (err)
        return err;

which returns with a partially modified mask and no saved copy of the
previous register value.

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