Re: [PATCH net-next v2 6/6] net: mdio: bcm-unimac: implement timestamped MDIO writes

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] unimac_mdio_init_sts() reads the MDC reference rate once at
  probe (`priv->mdio_ref_rate = clk_get_rate(ref_clk)`) and caches it…
- [Medium] net: mdio: bcm-unimac: broken probe deferral in
  unimac_mdio_probe() for GENET
- [Low] The patch gives the mdio node's `clocks` property a new driver-
  side meaning but does not update…
- [Low] On the GENET path, unimac_mdio_probe() gets ref_clk from the mdio
  node with devm_clk_get_optional() but never prepares or enables it.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patch
  makes it visible.

--

net: mdio: bcm-unimac: implement timestamped MDIO writes

This patch adds a write_sts callback to the UniMAC MDIO driver. The callback
takes system timestamps around the MDIO command start and shifts them by
the computed time to the MDC edge that clocks the last data bit. The MDC
reference rate comes from a clock on the mdio node, and write_sts is only
offered when that clock reports a non-zero rate.

> diff --git a/drivers/net/mdio/mdio-bcm-unimac.c b/drivers/net/mdio/mdio-bcm-unimac.c
> index 31e396cc9fbf2..a18e213e3832a 100644
> --- a/drivers/net/mdio/mdio-bcm-unimac.c
> +++ b/drivers/net/mdio/mdio-bcm-unimac.c

[ ... ]

> @@ -234,11 +333,21 @@ static int unimac_mdio_clk_set(struct unimac_mdio_priv *priv)
>  	return ret;
>  }
>  
> +static bool unimac_mdio_init_sts(struct unimac_mdio_priv *priv,
> +				 struct clk *ref_clk)
> +{
> +	/* The reference rate is taken to be fixed, so read it once. */
> +	priv->mdio_ref_rate = clk_get_rate(ref_clk);

[Severity: Medium]
The comment says the reference rate is taken to be fixed. Is anything
actually keeping it fixed? There is no clk_rate_exclusive_get() and no clk
notifier, so the rate cached at probe can go stale if another consumer or a
parent change alters it later.

unimac_mdio_sts_delays() combines the live MDIO_CFG divider with this
cached value:

	*pre_ns = div64_u64((2ULL * bits - 1) * divisor * NSEC_PER_SEC,
			    ref_rate2);

The window between the pre and post timestamps is only about one MDC period
wide (two with MDIO_SUPP_PREAMBLE), but the offsets span about 64 periods.
At 2.5 MHz MDC, a 1% rate change moves the edge by about 256 ns, while the
period is 400 ns.

Would unimac_mdio_write_sts() then give __mdiobus_write_sts() bounds that
silently miss the real MDC edge? The struct mii_bus documentation says
write_sts should be set "only if timestamps can always be provided".

Could the driver take clk_rate_exclusive_get() at probe to enforce this
assumption? Calling clk_get_rate() from write_sts instead runs into the
prepare_lock vs mdio_lock ordering noted in macb_main.c.

> +
> +	return priv->mdio_ref_rate != 0;
> +}

[ ... ]

> @@ -277,11 +386,23 @@ static int unimac_mdio_probe(struct platform_device *pdev)
>  		priv->wait_func_data = pdata->wait_func_data;
>  		bus->phy_mask = ~pdata->phy_mask;
>  		priv->clk = pdata->clk;
> +		/* priv->clk is GENET's main clock, which gates register
> +		 * access. ref_clk is the clock used as the MDC reference,
> +		 * which we get from the MDIO node. The presence of this
> +		 * clock enables write_sts.
> +		 */
> +		ref_clk = devm_clk_get_optional(&pdev->dev, NULL);
> +		if (IS_ERR(ref_clk) && PTR_ERR(ref_clk) != -EPROBE_DEFER) {
> +			dev_warn(&pdev->dev, "no MDC reference clock: %pe\n",
> +				 ref_clk);
> +			ref_clk = NULL;
> +		}

[Severity: Medium]
The commit message says a deferral of the clock lookup is passed on
so the MDIO bus waits for the clock's provider. Does that work on the
GENET path?

There the UniMAC device is not bound from DT. bcmgenet_mii_register()
creates it and calls platform_device_add(ppdev), and the probe result
of unimac_mdio_probe() is not propagated back through device_add().
If the mdio node's clock provider is not ready yet, the device is put
on the deferred probe list and platform_device_add() still returns 0.

The clocks property is on the mdio child node, not on the genet node,
so fw_devlink does not hold off bcmgenet_probe() either. It only adds
a sync-state-only proxy link to the parent.

bcmgenet_probe() then carries on as if the bus existed.
bcmgenet_mii_of_init() cannot find MoCA PHYs with
of_phy_find_device(), and register_netdev() runs with no MDIO bus or
PHY registered. Bringing the interface up before the deferred probe
completes would fail with "could not attach to PHY".

When the deferred unimac_mdio_probe() does run, could it race with
bcmgenet_open()? bcmgenet_open() resets the UMAC through
bcmgenet_umac_reset() and init_umac(), and that could happen while
of_mdiobus_register() is scanning the bus.

No in-tree DT gives a GENET mdio node a clock today, so this becomes
reachable only once the BCM2711 GISB clock is wired up. That is the
case this patch is preparing for.

write_sts is optional, so would it be simpler to treat -EPROBE_DEFER
like any other error on the pdata path and not offer write_sts? The
cost is that write_sts would then depend on probe order.

Alternatively, bcmgenet could look up the MDC reference clock itself
and pass it in struct unimac_mdio_pdata. A deferral would then happen
in bcmgenet_probe(), where it is actually honoured.

[Severity: Low]
On the GENET path, ref_clk is obtained here but never prepared or enabled.
Its only use is the clk_get_rate() call in unimac_mdio_init_sts(), and
unimac_mdio_write_sts() enables only priv->clk. That is GENET's main clock,
not the clock MDC is divided from.

The standalone path reads the same clocks property and calls
clk_prepare_enable() on that clock around every transfer.

The GENET path therefore relies on the provider marking the clock critical,
as the commit message says the BCM2711 GISB clock will. Neither the driver
nor the binding requires that.

If a DT gives a GENET mdio node a gateable, non-critical clock, can
clk_disable_unused gate it and stop MDIO? Would
devm_clk_get_optional_enabled() remove that dependency?

A smaller point: clk.h documents clk_get_rate() as "only valid once the
clock source has been enabled". unimac_mdio_init_sts() reads the rate before
any enable on both paths. Under the common clock framework the returned rate
does not depend on the enable state, so this is only a mismatch with the
documented contract.

[Severity: Medium]
This isn't a bug introduced by this patch, but the new comment makes it
more visible. unimac_mdio_clk_set() still computes the MDC divider from
priv->clk, falling back to 250 MHz:

	rate = clk_get_rate(priv->clk);
	if (!rate)
		rate = 250000000;

	div = (rate / (2 * priv->clk_freq)) - 1;

On the GENET path, priv->clk is pdata->clk. bcmgenet_mii_register() fills
that in with GENET's own clock (ppd.clk = priv->clk), which the comment
above says is not the MDC reference.

Now that ref_clk is available, should unimac_mdio_clk_set() use its rate
for the divider too?

For example, the BCM2711 genet node has no clocks, so the divider uses the
250 MHz fallback. A clock-frequency property on the mdio node would then
produce an MDC of about 200/250 of the requested frequency.

The new write_sts bounds are not affected, because
unimac_mdio_sts_delays() reads the divider back from MDIO_CFG.

>  	} else {
>  		bus->name = "unimac MII bus";
>  		priv->wait_func_data = priv;
>  		priv->wait_func = unimac_mdio_poll;
>  		priv->clk = devm_clk_get_optional(&pdev->dev, NULL);
> +		ref_clk = priv->clk;
>  	}

[ ... ]

> @@ -289,9 +410,16 @@ static int unimac_mdio_probe(struct platform_device *pdev)

[ ... ]

>  	bus->parent = &pdev->dev;
>  	bus->read = unimac_mdio_read;
>  	bus->write = unimac_mdio_write;
> +	if (unimac_mdio_init_sts(priv, ref_clk))
> +		bus->write_sts = unimac_mdio_write_sts;

[Severity: Low]
On the GENET compatibles, the mdio node's clocks property now has a new
meaning. The binding in
Documentation/devicetree/bindings/net/brcm,unimac-mdio.yaml still only
describes it like this:

  clocks:
    description: A reference to the clock supplying the MDIO bus controller

Should the binding say that the clock's rate must be the MDC reference
rate, that the clock enables timestamped writes, and that the driver does
not enable it on the GENET path?

The current text would also allow a DT to point a GENET mdio node's clocks
at another clock that supplies the controller, such as GENET's main clock.
unimac_mdio_init_sts() accepts any non-zero rate. Would a consumer such as
bcm_phy_write_exp_sts() then get bounds that do not contain the real MDC
edge?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018100257.566267-1-jjc%40jclark.com