Re: [PATCH net-next v3 10/10] net: dsa: microchip: add periodic output support for the KSZ8463

[netdev-bot+sashiko@kernel.org]

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

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

- [High] KSZ8463 advertises 12 periodic-output channels (.n_pins = 12,
  .n_per_out = 12, copied into caps by ksz8463_ptp_set_caps()) while the…
- [Medium] ksz8463_ptp_enable_perout() is not transactional: it calls
  ksz8463_ptp_tou_reset(dev, request->index) (stopping an already…
- [Medium] ksz8463_ptp_configure_perout() dereferences
  dev->ptp_data.pin_config[index]…
- [Low] ksz8463_ptp_set_caps() (drivers/net/dsa/microchip/ksz_ptp.c:1335)
  does not set ptp_data->caps.supported_perout_flags, while the code…

Pre-existing issues:
- [High] The new KSZ8463 settime/adjtime paths funnel into
  ksz_ptp_prepare_restart_perout(), whose catch-up arithmetic can…
- [High] A denormalized periodic-output period is accepted as zero
  nanoseconds and later causes a kernel divide-by-zero or an infinite…

--

net: dsa: microchip: add periodic output support for the KSZ8463

This adds KSZ8463 specific periodic output handling: per-unit trigger
configuration registers, a smaller maximum pulse width, and a new set of
ksz8463_* PTP callbacks installed by ksz8463_ptp_set_caps().  The chip
data now advertises 12 pins and 12 periodic output channels.

A few questions below about the new paths.

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 041bb4f963659..9e234ad71b723 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1161,6 +1161,8 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.supports_mii = {false, false, true},
>  		.supports_rmii = {false, false, true},
>  		.internal_phy = {true, true, false},
> +		.n_pins = 12,
> +		.n_per_out = 12,
>  	},

[Severity: High]
Twelve periodic output channels are advertised here, but struct
ksz_ptp_data still keeps only one set of perout state (a single tou_mode,
one perout_target_time_first, one perout_period), and the restart helper
always rebuilds a request for channel 0:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_ptp_prepare_restart_perout() {
	...
	request->index = 0;
	request->flags = 0;

	return 0;
}

Userspace can map pin 3 to PTP_PF_PEROUT chan 3 (ptp_set_pinfunc() only
checks chan < n_per_out, and ksz_ptp_verify_pin() accepts any PEROUT
pin/chan) and then arm PTP_PEROUT_REQUEST with index 3, which makes
ksz8463_ptp_enable_perout() program trigger unit 3 and set tou_mode =
KSZ_PTP_TOU_PEROUT.

The next clock_settime() then walks:

ksz8463_ptp_settime() -> ksz8463_ptp_restart_perout() ->
  ksz_ptp_prepare_restart_perout() (index = 0) ->
  ksz8463_ptp_enable_perout() -> ksz_ptp_get_pin() ->
  ptp_find_pin(clock, PTP_PF_PEROUT, 0)

With no pin mapped to chan 0 that returns -1 and ksz_ptp_get_pin() returns
-EINVAL.  Does that mean every clock_settime()/clock_adjtime() fails for as
long as a perout is armed on a channel other than 0?

And because the failure takes the goto unlock path after
__ksz_ptp_settime() has already loaded the hardware RTC, is
ptp_data->clock_time (used by ksz_tstamp_reconstruct()) left behind until
ksz_ptp_do_aux_work() happens to run?

If some pin is mapped to chan 0, the restart resets and re-arms unit 0 with
the last stored period and target instead, while the unit that is actually
running keeps its old absolute 32-bit target and is never re-armed.  Is
that intended?

There are two related effects: enabling a second channel overwrites the
stored period/target of the first, and disabling any single channel sets
the global tou_mode to KSZ_PTP_TOU_IDLE, after which clock steps take the
IDLE branch and the units still set in KSZ8463_TOU_EN are never
resynchronised.  Should the perout state become per-channel before
n_per_out = 12 is exposed?

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index 0a77b46cfad25..d33034892a8f6 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c

[ ... ]

> @@ -120,6 +132,28 @@ static int ksz_ptp_tou_target_time_set(struct ksz_device *dev,
>  	return 0;
>  }
>  
> +static int ksz8463_ptp_tou_start(struct ksz_device *dev, u8 unit)
> +{
> +	u16 data;
> +	int ret;
> +
> +	ret = ksz_rmw16(dev, KSZ8463_TOU_EN, BIT(unit), BIT(unit));
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz_read16(dev, KSZ8463_TOU_ACTIVE, &data);
> +	if (ret)
> +		return ret;
> +
> +	if (!(data & BIT(unit))) {
> +		dev_err(dev->dev, "%s: Trigger unit%d error!\n", __func__,
> +			unit);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -147,6 +181,56 @@ static int ksz_ptp_tou_start(struct ksz_device *dev, u8 unit)
>  	return 0;
>  }
>  
> +static int ksz8463_ptp_configure_perout(struct ksz_device *dev,
> +					u32 cycle_width_ns, u32 pulse_width_ns,
> +					struct timespec64 const *target_time,
> +					u8 index)
> +{
> +	struct ptp_pin_desc *pin = &dev->ptp_data.pin_config[index];
> +	u16 cfg_base = KSZ8463_TRIG1_CFG + KSZ8463_TRIGN_CFG_SIZE * pin->chan;

[Severity: Medium]
Is pin_config safe to read here with only ptp_data->lock held?  pin->chan
selects which trigger unit's register block gets rewritten, and
drivers/ptp/ptp_private.h documents the lock for that array as:

	struct mutex pincfg_mux; /* protect concurrent info->pin_config access */

The core holds pincfg_mux around ->enable() only, not around
->settime64()/->adjtime().  So on the new paths:

CPU0: clock_settime() -> ksz8463_ptp_settime() (takes ptp_data->lock only)
        -> ksz8463_ptp_restart_perout() -> ksz8463_ptp_enable_perout()
        -> ksz_ptp_get_pin() -> ptp_find_pin() -> ksz8463_ptp_configure_perout()
           reads pin_config[index].chan / .index

CPU1: PTP_PIN_SETFUNC -> ptp_set_pinfunc() (holds pincfg_mux) writes

	if (pin1) {
		ptp_disable_pinfunc(info, func, chan);
		pin1->func = PTP_PF_NONE;
		pin1->chan = 0;
	}
	...
	pin2->func = func;
	pin2->chan = chan;

Those stores happen after ptp_disable_pinfunc() has already returned from
ksz8463_ptp_enable() and released ptp_data->lock, and for func ==
PTP_PF_NONE no ->enable() call happens at all, so ptp_data->lock does not
close the window.  Can cfg_base then point at a different, possibly
active, trigger unit than the one reset and enabled via request->index and
silently rewrite its configuration?

Note that taking pincfg_mux inside ptp_data->lock would invert the order
the core establishes in ptp_set_pinfunc() -> ptp_disable_pinfunc() ->
ksz8463_ptp_enable(); caching the pin/chan under ptp_data->lock at
enable() time looks like it would avoid that.

> +	u16 data;
> +	int ret;
> +
> +	/* Hardware has only 32 bit */
> +	if ((target_time->tv_sec & 0xffffffff) != target_time->tv_sec)
> +		return -EINVAL;
> +
> +	data = KSZ8463_NOTIFY_BIT |
> +	       FIELD_PREP(KSZ8463_PATTERN_M, TRIG_POS_PERIOD) |
> +	       pin->index;
> +	ret = ksz_write16(dev, cfg_base + KSZ8463_PATTERN_OFF, data);
> +	if (ret)
> +		return ret;

[ ... ]

> +	return 0;
> +}
> +

[ ... ]

> @@ -241,6 +325,58 @@ static u64 ksz_ptp_compute_perout_pulse(struct ksz_device *dev,
>  	return min_t(u64, req_pulse_width_ns, max_pulse_width);
>  }
>  
> +static int ksz8463_ptp_enable_perout(struct ksz_device *dev,
> +				     struct ptp_perout_request const *request,
> +				     int on)
> +{
> +	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> +	u64 cycle_width_ns;
> +	u64 pulse_width_ns;
> +	int pin;
> +	int ret;
> +
> +	pin = ksz_ptp_get_pin(dev, request);
> +	if (pin < 0)
> +		return pin;
> +
> +	ret = ksz8463_ptp_tou_reset(dev, request->index);
> +	if (ret)
> +		return ret;
> +
> +	if (!on) {
> +		ptp_data->tou_mode = KSZ_PTP_TOU_IDLE;
> +		return 0;
> +	}
> +	ret = ksz_ptp_compute_perout_cycle(dev, request, &cycle_width_ns);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Is there a way back out of this sequence when a request is rejected?  The
running unit is reset before the request is validated, and
ksz_ptp_compute_perout_cycle() overwrites the saved schedule before it can
reject the period:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_ptp_compute_perout_cycle() {
	ptp_data->perout_target_time_first.tv_sec  = request->start.sec;
	...
	*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;
	}
}

So with a perout already running, a PTP_PEROUT_REQUEST carrying a period
whose ns value does not fit 32 bits (period.sec >= 5) resets trigger unit
N, stores the rejected start/period, and returns -EINVAL, while tou_mode
stays KSZ_PTP_TOU_PEROUT from the previous successful enable.  Does every
later clock_settime()/clock_adjtime() then take the PEROUT branch, call
ksz8463_ptp_restart_perout() with the rejected parameters, fail again, and
jump to unlock after the RTC has already been written but before
ptp_data->clock_time is updated?

The same applies to the -EINVAL/-ERANGE returns from
ksz_ptp_tou_pulse_verify(), the 32-bit target check in
ksz8463_ptp_configure_perout(), any regmap error, and the -EIO from
ksz8463_ptp_tou_start().

Related: ksz8463_ptp_tou_start() sets KSZ8463_TOU_EN before reading
KSZ8463_TOU_ACTIVE and does not clear the enable bit when the read fails
or ACTIVE is not set, so does an operation reported as -EIO leave the unit
enabled in hardware?

> +	pulse_width_ns = ksz_ptp_compute_perout_pulse(dev, request,
> +						      KSZ8463_MAX_PULSE_WIDTH);
> +
> +	ret = ksz_ptp_tou_pulse_verify(pulse_width_ns,
> +				       KSZ8463_TRIG_PULSE_WIDTH_M);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz8463_ptp_configure_perout(dev, cycle_width_ns, pulse_width_ns,
> +					   &ptp_data->perout_target_time_first,
> +					   pin);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz_ptp_tou_gpio(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz8463_ptp_tou_start(dev, request->index);
> +	if (ret)
> +		return ret;
> +
> +	ptp_data->tou_mode = KSZ_PTP_TOU_PEROUT;
> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -845,6 +981,18 @@ static int ksz_ptp_restart_perout(struct ksz_device *dev)
>  	return ksz_ptp_enable_perout(dev, &request, 1);
>  }
>  
> +static int ksz8463_ptp_restart_perout(struct ksz_device *dev)
> +{
> +	struct ptp_perout_request request;
> +	int ret;
> +
> +	ret = ksz_ptp_prepare_restart_perout(dev, &request);
> +	if (ret)
> +		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but the helper reached from here
truncates the catch-up quotient to 32 bits, and this adds a second entry
point into it:

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;
}

count is only unsigned int, so once the true number of elapsed periods
passes 2^32, next_ns is left roughly 2^32 * period_ns in the past and the
guard loop advances one period per iteration, with ptp_data->lock held, in
ioctl context, and without cond_resched().

Small periods are accepted (TRIG_CYCLE_WIDTH_M is GENMASK(31, 0), and a
1 us period yields a 500 ns pulse that passes ksz_ptp_tou_pulse_verify()),
so a perout with a 1 us period armed a day earlier gives a true quotient
around 8.6e10.  Can the next clock_settime() ->
ksz8463_ptp_settime() -> ksz8463_ptp_restart_perout() then spin for that
many iterations under the PTP mutex, producing RCU stalls or soft lockups
and blocking every other PTP operation on the device?

Write access to /dev/ptpN is enough to reach both steps, since posix
dynamic clocks only require FMODE_WRITE.  Would widening count, or
bounding the accepted period, be worth doing here?

[Severity: High]
This is a pre-existing issue in the same shared helper, but the new
KSZ8463 clock update paths make it reachable for this chip too: a
denormalized period is accepted as zero nanoseconds.

PTP_PEROUT_REQUEST does not check that struct ptp_clock_time is
normalized, and the core enables the output because the fields are
nonzero:

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

So period.sec = -1 with period.nsec = 1000000000 reaches
ksz8463_ptp_enable_perout() with on = 1, ksz_ptp_compute_perout_cycle()
stores those raw fields into ptp_data->perout_period, timespec64_to_ns()
is 0, and 0 passes both the TRIG_CYCLE_WIDTH_M test and
ksz_ptp_tou_pulse_verify().

With a stored period of 0 and tou_mode still KSZ_PTP_TOU_PEROUT from an
earlier successful enable, the next clock_settime()/clock_adjtime() reaches
ksz_ptp_prepare_restart_perout():

	if (first_ns < now_ns) {
		count = div_u64(now_ns - first_ns, period_ns);

Does this divide by zero and Oops with ptp_data->lock held?  And in the
other branch, does

	while (next_ns < now_ns + 100000000)
		next_ns += period_ns;

add zero forever?  Would a normalized, strictly positive period check in
ksz_ptp_compute_perout_cycle() cover both cases?

> +
> +	return ksz8463_ptp_enable_perout(dev, &request, 1);
> +}
> +

[ ... ]

> @@ -871,6 +1019,41 @@ static int __ksz_ptp_settime(struct ksz_device *dev,
>  	return 0;
>  }
>  
> +static int ksz8463_ptp_settime(struct ptp_clock_info *ptp,
> +			       const struct timespec64 *ts)
> +{
> +	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> +	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> +	int ret;
> +
> +	mutex_lock(&ptp_data->lock);
> +
> +	ret = __ksz_ptp_settime(dev, ts);
> +	if (ret)
> +		goto unlock;
> +
> +	switch (ptp_data->tou_mode) {
> +	case KSZ_PTP_TOU_IDLE:
> +		break;
> +
> +	case KSZ_PTP_TOU_PEROUT:
> +		ret = ksz8463_ptp_restart_perout(dev);
> +		if (ret)
> +			goto unlock;
> +
> +		break;
> +	}
> +
> +	spin_lock_bh(&ptp_data->clock_lock);
> +	ptp_data->clock_time = *ts;
> +	spin_unlock_bh(&ptp_data->clock_lock);
> +
> +unlock:
> +	mutex_unlock(&ptp_data->lock);
> +
> +	return ret;
> +}
> +

[ ... ]

> @@ -1095,6 +1332,27 @@ static int ksz_ptp_start_clock(struct ksz_device *dev)
>  	return 0;
>  }
>  
> +void ksz8463_ptp_set_caps(struct dsa_switch *ds)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_ptp_data *ptp_data;
> +
> +	ptp_data = &dev->ptp_data;
> +
> +	ptp_data->caps.owner		= THIS_MODULE;
> +	snprintf(ptp_data->caps.name, 16, "Microchip Clock");
> +	ptp_data->caps.max_adj		= KSZ_MAX_DRIFT_CORR;
> +	ptp_data->caps.gettime64	= ksz_ptp_gettime;
> +	ptp_data->caps.settime64	= ksz8463_ptp_settime;
> +	ptp_data->caps.adjfine		= ksz_ptp_adjfine;
> +	ptp_data->caps.adjtime		= ksz8463_ptp_adjtime;
> +	ptp_data->caps.do_aux_work	= ksz_ptp_do_aux_work;
> +	ptp_data->caps.enable		= ksz8463_ptp_enable;
> +	ptp_data->caps.verify		= ksz_ptp_verify_pin;
> +	ptp_data->caps.n_pins		= dev->info->n_pins;
> +	ptp_data->caps.n_per_out	= dev->info->n_per_out;
> +}
> +

[Severity: Low]
supported_perout_flags is not set here, while the path installed by this
function implements PTP_PEROUT_DUTY_CYCLE: ksz_ptp_get_pin() only rejects
flags outside PTP_PEROUT_DUTY_CYCLE, and
ksz_ptp_compute_perout_pulse() consumes request->on:

	if (request->flags & PTP_PEROUT_DUTY_CYCLE)
		return request->on.sec * NSEC_PER_SEC +	request->on.nsec;

The core rejects the ioctl before ->enable() runs:

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

So does every PTP_PEROUT_REQUEST2 with PTP_PEROUT_DUTY_CYCLE fail with
-EOPNOTSUPP, leaving the duty cycle handling unreachable?  The same
omission exists in ksz_ptp_set_caps(), so the KSZ8463 is no worse than the
other chips, but should the new initializer either set
supported_perout_flags = PTP_PEROUT_DUTY_CYCLE or drop the duty cycle
handling?

[ ... ]

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