Re: [PATCH net-next v2 3/6] net: mdio: add timestamped write operation

[netdev-bot+sashiko@kernel.org]

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] Claim: the fixed event_delay_ns + 1 ms deadline in
  __mdiobus_write_sts() hard-codes a minimum MDC rate of about 65 kHz.

--

net: mdio: add timestamped write operation

This adds an optional write_sts mii_bus operation that returns system
timestamp bounds for the completion of a clause 22 MDIO write. The new
__mdiobus_write_sts() adds an event delay to the upper bound, then waits
until that upper bound has passed so it can check that the clock was not
stepped. It also adds MDIO_C22_FRAME_BITS and MDIO_PREAMBLE_BITS, which bus
drivers can use to work out when a write completes.

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

[ ... ]

> @@ -145,6 +146,112 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)

[ ... ]

> +int __mdiobus_write_sts(struct mii_bus *bus, int addr, u32 regnum, u16 val,
> +			u32 event_delay_ns, struct ptp_system_timestamp *sts)
> +{

[ ... ]

> +	/* Fail if the clock was stepped. */
> +	if (sts->pre_sts.clock_was_set_seq != sts->post_sts.clock_was_set_seq)
> +		return -EBUSY;
> +
> +	/* Fail if the shift in the upper bound crosses a clock step. Since
> +	 * write_sts() can return before the transfer completes, we need to wait
> +	 * until the shifted upper bound has passed. This wait needs a deadline
> +	 * in case an aux clock is disabled and reenabled, which restarts its
> +	 * raw time. 1 ms is enough for a frame with MDC down to about 65 kHz.
> +	 */
> +	deadline = ktime_add_ns(ktime_get_raw(),
> +				(u64)event_delay_ns + NSEC_PER_MSEC);
> +	for (;;) {
> +		bool late = !ktime_before(ktime_get_raw(), deadline);
> +
> +		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;
> +
> +		if (late)
> +			return -EBUSY;

[Severity: Low]

This isn't a bug, but the 1 ms allowance is counted from the moment
write_sts() returns, which makes it part of the bus op contract.

Suppose a write_sts() implementation returned more than about 1 ms before
its predicted completion. Wouldn't every timestamped write on that bus fail
here with -EBUSY for CLOCK_REALTIME, CLOCK_BOOTTIME and aux clocks, even
though the write itself succeeded?

This looks theoretical for the implementations in this series.
macb_mdio_write_c22_sts() and unimac_mdio_write_sts() both wait for the
controller to go idle before returning. Their upper bound is therefore at
most about one MDC period plus event_delay_ns past the return time.
Synchronous drivers such as bit-banged MDIO also return after the frame.

The code comment above does state the limit. The @write_sts kerneldoc in
include/linux/phy.h, though, only says:

	/**
	 * @write_sts: Perform a C22 write transfer on the bus,
	 * with system timestamps bounding its completion. Set only
	 * if timestamps can always be provided. Must not return
	 * -EOPNOTSUPP.
	 */

Could that kerneldoc also say that write_sts() must not return more than
1 ms before the predicted completion of the transfer?

> +
> +		cpu_relax();
> +	}
> +}
> +EXPORT_SYMBOL_GPL(__mdiobus_write_sts);

[ ... ]

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