Re: [PATCH net-next v3 06/10] net: dsa: microchip: extract compute_width

[netdev-bot+sashiko]

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

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

- [Low] The new helper `ksz_ptp_compute_perout_pulse()`
  (drivers/net/dsa/microchip/ksz_ptp.c:227) declares a `struct…

Pre-existing issues:
- [High] A non-normalized but individually non-zero periodic-output
  request is accepted and stored as an effective period of zero, after…
- [Medium] The KSZ PTP driver implements `PTP_PEROUT_DUTY_CYCLE`
  (accepted in `ksz_ptp_get_pin()`, consumed in the newly extracted…

--

net: dsa: microchip: extract compute_width

This moves the periodic-output cycle width and pulse width computations out
of ksz_ptp_enable_perout() into two new helpers,
ksz_ptp_compute_perout_cycle() and ksz_ptp_compute_perout_pulse(), so that
an upcoming ksz8463_ptp_enable_perout() can reuse them.  No functional
change is intended.

A few questions below.

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index f97ea9d69ebc5..2f141df65eb01 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -203,12 +203,49 @@ static int ksz_ptp_get_pin(struct ksz_device *dev,
>  	return pin;
>  }
>  
> +static int ksz_ptp_compute_perout_cycle(struct ksz_device *dev,
> +					struct ptp_perout_request const *request,
> +					u64 *cycle_width_ns)
> +{
> +	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> +
> +	ptp_data->perout_target_time_first.tv_sec  = request->start.sec;
> +	ptp_data->perout_target_time_first.tv_nsec = request->start.nsec;
> +
> +	ptp_data->perout_period.tv_sec = request->period.sec;
> +	ptp_data->perout_period.tv_nsec = request->period.nsec;
> +
> +	*cycle_width_ns = timespec64_to_ns(&ptp_data->perout_period);
> +	if ((*cycle_width_ns & TRIG_CYCLE_WIDTH_M) != *cycle_width_ns) {
> +		*cycle_width_ns = 0;
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
This isn't a bug introduced by this patch, since the code is moved
unchanged, but can a period that sums to zero get past this check and
later divide by zero?

The core does not normalize the request.  In ptp_perout_request() the
enable argument is computed from the raw fields:

drivers/ptp/ptp_chardev.c:ptp_perout_request() {
    ...
	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
		return ops->enable(ops, &req, perout->period.sec || perout->period.nsec);
    ...
}

So a request with period.sec = -1 and period.nsec = 1000000000 arrives
here with on = 1.  ksz_ptp_compute_perout_cycle() stores both raw fields
into ptp_data->perout_period, and timespec64_to_ns() then yields
-1e9 + 1e9 = 0.  The guard passes because 0 & TRIG_CYCLE_WIDTH_M == 0.

ksz_ptp_compute_perout_pulse() returns 0 for the same input, and
ksz_ptp_tou_pulse_verify(0, TRIG_PULSE_WIDTH_M) also returns 0, so the
configuration succeeds and tou_mode becomes KSZ_PTP_TOU_PEROUT.

A later PTP_CLOCK_SETTIME or ADJTIME then reaches ksz_ptp_restart_perout()
with ptp_data->lock held:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_ptp_restart_perout() {
    ...
	period_ns = timespec64_to_ns(&ptp_data->perout_period);

	if (first_ns < now_ns) {
		count = div_u64(now_ns - first_ns, period_ns);
    ...
	/* Ensure 100 ms guard time prior next event */
	while (next_ns < now_ns + 100000000)
		next_ns += period_ns;
    ...
}

With period_ns == 0, is the div_u64() a divide error, and does the guard
time loop spin forever when first_ns >= now_ns?  In both cases
ptp_data->lock is never released, so subsequent PTP operations on the
switch would block.

Since this helper is becoming the shared entry point for the KSZ8463 too,
would it make sense to reject a computed cycle width of zero here?

> +
> +static u64 ksz_ptp_compute_perout_pulse(struct ksz_device *dev,
> +					struct ptp_perout_request const *request,
> +					u64 max_pulse_width)
> +{
> +	u64 req_pulse_width_ns;
> +
> +	if (request->flags & PTP_PEROUT_DUTY_CYCLE)
> +		return request->on.sec * NSEC_PER_SEC +	request->on.nsec;

[Severity: Low]
This isn't a bug, but the dev parameter of
ksz_ptp_compute_perout_pulse() is never used in the function body, which
only reads request->flags, request->on, request->period and
max_pulse_width.  Since the chip specifics are conveyed through
max_pulse_width, could dev be dropped from the signature before the
KSZ8463 caller is added?  The only current caller,
ksz_ptp_enable_perout(), passes it needlessly.

[Severity: Medium]
This is a pre-existing issue, but is this PTP_PEROUT_DUTY_CYCLE branch
reachable at all?

ksz_ptp_set_caps() fills in caps.enable, caps.verify, caps.n_pins and
caps.n_per_out but never sets caps.supported_perout_flags, and the PTP
core has no default for it:

drivers/ptp/ptp_chardev.c:ptp_perout_request() {
    ...
	if (perout->flags & ~ops->supported_perout_flags)
		return -EOPNOTSUPP;
    ...
}

With supported_perout_flags == 0, every PTP_PEROUT_REQUEST2 carrying
PTP_PEROUT_DUTY_CYCLE is rejected before ops->enable() runs, so
ksz_ptp_get_pin() and this branch never see the flag.

Since this branch is now shared with the upcoming KSZ8463 support, should
ksz_ptp_set_caps() also set

	ptp_data->caps.supported_perout_flags = PTP_PEROUT_DUTY_CYCLE;

> +
> +	/* Use a duty cycle of 50%. Maximum pulse width supported by the
> +	 * hardware is a little bit more than 125 ms.
> +	 */
> +	req_pulse_width_ns = (request->period.sec * NSEC_PER_SEC +
> +			      request->period.nsec) / 2;
> +	return min_t(u64, req_pulse_width_ns, max_pulse_width);
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908-ksz-perout-v3-0-6722a3f1ca75%40bootlin.com