Re: [PATCH net-next v4 08/11] net: dsa: microchip: extract prepare reset

[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] The commit subject "net: dsa: microchip: extract prepare reset"
  does not describe the change.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  moves it into the new shared helper and later patches keep using it.

--

net: dsa: microchip: extract prepare reset

This commit moves the request setup in ksz_ptp_restart_perout() into a
new helper, ksz_ptp_prepare_restart_perout(). The helper fills a
caller-supplied struct ptp_perout_request. A later KSZ8463-specific
restart path will reuse it.

> net: dsa: microchip: extract prepare reset

[Severity: Low]
This isn't a bug, but should the subject say "restart" instead of "reset"?

The patch adds no reset logic. It only splits out the setup of the
periodic output restart request into ksz_ptp_prepare_restart_perout().

This file also has ksz_ptp_tou_reset(), which ksz_ptp_enable_perout()
calls. That makes "reset" in the subject easy to confuse with a TOU
reset change.

> Extract the inialization into a dedicated function so it can be used
> later by the KSZ8463 support.

There is also a typo in the body: "inialization" should be
"initialization".

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index 4b57cf076bb4f..b627062dbaad7 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -793,11 +793,11 @@ static int ksz_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
>  	return ret;
>  }
>  
> -static int ksz_ptp_restart_perout(struct ksz_device *dev)
> +static int ksz_ptp_prepare_restart_perout(struct ksz_device *dev,
> +					  struct ptp_perout_request *request)
>  {
>  	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
>  	s64 now_ns, first_ns, period_ns, next_ns;
> -	struct ptp_perout_request request;
>  	struct timespec64 next;
>  	struct timespec64 now;
>  	struct timespec64 tmp;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch.
However, the code now sits in the new shared helper, which the later
KSZ8463 restart path is meant to use.

Can the unsigned int count in ksz_ptp_prepare_restart_perout() truncate
the 64-bit div_u64() quotient?

drivers/net/dsa/microchip/ksz_ptp.c:ksz_ptp_prepare_restart_perout() {
    ...
	unsigned int count;
    ...
	if (first_ns < now_ns) {
		count = div_u64(now_ns - first_ns, period_ns);
		next_ns = first_ns + count * period_ns;
	} else {
		next_ns = first_ns;
	}

	/* Ensure 100 ms guard time prior next event */
	while (next_ns < now_ns + 100000000)
		next_ns += period_ns;
    ...
}

Once the PHC is more than 2^32 periods past the stored perout start,
count loses its high bits and next_ns lands far behind now_ns. The guard
loop then has to make up the lost cycles one period_ns at a time. It has
no resched point, and ptp_data->lock is held.

For example, take a 1 us period started at 110 s, followed by a settime
to 5000 s. The quotient is 4,890,000,000, which truncates to
595,032,704. That leaves about 4.29e9 loop iterations.

Suppose instead a perout is started with the PHC near 0 and the clock
is then stepped to current TAI. With a 1 us period that is about 1.79e15
iterations, and ksz_ptp_tou_pulse_verify() accepts periods as short as
8 ns.

With a 1 ms period, the same thing happens on any settime or adjtime
more than about 49.7 days after the perout start time.

The paths that reach this whenever tou_mode == KSZ_PTP_TOU_PEROUT are:

  clock_settime() -> ksz_ptp_settime() -> ksz_ptp_restart_perout()
      -> ksz_ptp_prepare_restart_perout()

  clock_adjtime(ADJ_SETOFFSET) -> ksz_ptp_adjtime()
      -> ksz_ptp_restart_perout() -> ksz_ptp_prepare_restart_perout()

Would declaring count as u64 (or s64) avoid this?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-ksz-perout-v4-0-d675bd21ce49%40bootlin.com