Re: [PATCH net-next 1/5] net: mdio: add timestamped write operation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] __mdiobus_write_sts() in drivers/net/phy/mdio_bus.c can return
  -EINVAL or -EBUSY after bus->write_sts() has already succeeded and the…
- [Low] In __mdiobus_write_sts() (drivers/net/phy/mdio_bus.c:223-226),
  the comment says the 1 ms cap 'guards against an aux clock being…

--

net: mdio: add timestamped write operation

This adds an optional write_sts mii_bus operation. It returns PTP system
timestamps that bound the completion of an MDIO write, and comes with the
__mdiobus_write_sts() and __phy_write_sts() helpers. The core checks the
returned timestamps. For clocks that can be stepped, it also busy-waits
until the clock's raw time reaches the upper bound. mdio-mux passes the
operation through to the parent bus.

> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 00d0e4159e9bb..8e734227b2c67 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c

[ ... ]

> @@ -145,6 +146,105 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
>  }
>  EXPORT_SYMBOL(__mdiobus_write);
>  
> +/**
> + * __mdiobus_write_sts - Timestamped version of the __mdiobus_write function

[ ... ]

> + * Return: Zero if successful, negative error code on failure. Returns
> + *	   -EBUSY or -EINVAL if the system timestamps are not valid. If @sts
> + *	   is not NULL, -EOPNOTSUPP is returned only if
> + *	   mdiobus_supports_write_sts() is false.

[ ... ]

> +	if (bus->write_sts)
> +		err = bus->write_sts(bus, addr, regnum, val, sts);
> +	else
> +		err = -EOPNOTSUPP;
> +
> +	trace_mdio_access(bus, 0, addr, regnum, val, err);
> +	mdiobus_stats_acct(&bus->stats[addr], false, err);
> +
> +	if (err)
> +		return err;
> +
> +	if (!sts->pre_sts.valid || !sts->post_sts.valid)
> +		return -EINVAL;
> +
> +	if (sts->clockid == CLOCK_MONOTONIC ||
> +	    sts->clockid == CLOCK_MONOTONIC_RAW)
> +		return 0;
> +
> +	/* Fail if the clock was stepped; callers must retry anyway. */
> +	if (sts->pre_sts.clock_was_set_seq != sts->post_sts.clock_was_set_seq)
> +		return -EBUSY;

[Severity: Low]
These -EINVAL and -EBUSY returns in __mdiobus_write_sts() happen after
bus->write_sts() has already returned 0, so the PHY register has already
changed. The same is true for the !now.valid return, the sequence mismatch
return and the 1 ms deadline return in the loop below.

The same codes can also mean the write never happened. Bus drivers are only
told not to return -EOPNOTSUPP. Also, mdio_mux_write_sts() passes a
switch_fn error straight through without doing the write:

    r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
    ...
    if (r)
        goto out;

For example, the meson g12a mux switch callback returns -EINVAL in its
default case.

Should the kernel-doc say that -EBUSY or -EINVAL from timestamp validation
means the write was performed? The doc says "Returns -EBUSY or -EINVAL if
the system timestamps are not valid", and the comment here says "callers
must retry anyway". Together, they suggest a retry is always safe.

A caller of __phy_write_sts() cannot tell "register not written" apart from
"register written, timestamps unusable". Could a retry of a non-idempotent
write apply it twice? Examples are a time load, a time adjust or a
self-clearing trigger.

The bcm_ptp_framesync_ts() user in bcm-phy-ptp.c handles this correctly. It
always polls for the capture and restores NSE_CTRL, whatever the error. So
nothing misbehaves today. The gap is in the API contract for future users.

> +
> +	/* The shifted upper bound can be later than actual completion. */
> +	deadline = ktime_add_ns(ktime_get_raw(), NSEC_PER_MSEC);
> +	for (;;) {
> +		ktime_get_snapshot_id(sts->clockid, &now);
> +		if (!now.valid)
> +			return -EINVAL;
> +
> +		if (now.clock_was_set_seq != sts->pre_sts.clock_was_set_seq)
> +			return -EBUSY;
> +
> +		if (!ktime_before(now.monoraw, sts->post_sts.monoraw))
> +			return 0;
> +
> +		/* Cap the wait at 1 ms, which is more than any single
> +		 * write's delay. This guards against an aux clock being
> +		 * disabled and reenabled, which restarts its raw time.
> +		 */
> +		if (!ktime_before(ktime_get_raw(), deadline))
> +			return -EBUSY;

[Severity: Low]
Does the 1 ms cap fully guard against an aux clock being disabled and then
re-enabled?

aux_clock_enable() clears the shadow timekeeper:

kernel/time/timekeeping.c:aux_clock_enable() {
    ...
	/* Remove leftovers of a previous registration */
	memset(aux_tks, 0, sizeof(*aux_tks));
    ...
	timekeeping_update_from_shadow(aux_tkd, TK_UPDATE_ALL);
}

This resets clock_was_set_seq, the raw time and offs_aux to 0. Then
TK_UPDATE_ALL increments clock_was_set_seq to 1.

An aux clock that was enabled and never set has seq 1. After a disable and
re-enable it has seq 1 again. The clock_was_set_seq comparisons in
__mdiobus_write_sts() therefore cannot see the reset. The only guard left
is the now.monoraw >= post_sts.monoraw check.

Two cases look like they get through.

In the first case, the aux clock was enabled recently, so post_sts.monoraw
is small. The re-enable happens while the provider sleeps after taking its
snapshots, for example in a wait_event_timeout() based wait such as
bcmgenet_mii_wait() with HZ / 100. The restarted raw time can then pass
post_sts.monoraw before this loop runs. The deadline is only computed after
write_sts() returns, so it does not help here.

In the second case, the disable and re-enable happen on another CPU between
the pre and post snapshots. post_sts then has a small monoraw from the new
epoch, and the loop returns 0 at once. Nothing checks that pre_sts comes
before post_sts.

In both cases gettimex64 would report bounds that cross an aux clock reset
as valid, instead of returning -EBUSY. This needs a privileged user to
toggle the aux clock during the write, so the window is narrow.

Should the comment be reworded to describe what the cap actually covers? Or
should the reset be detected some other way?

[ ... ]

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