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

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

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

- [High] KSZ8463 periodic output is advertised as 12 independent channels
  (.n_pins = 12, .n_per_out = 12 in ksz_switch_chips[KSZ8463]) but the…
- [High] KSZ8463 periodic output (TOU) is not properly disabled in
  hardware (leaves EN bit).
- [Medium] Build failure when CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=n:
  ksz8463_setup() in drivers/net/dsa/microchip/ksz8.c now calls…
- [Medium] The newly added ksz8463_ptp_configure_perout() dereferences
  dev->ptp_data.pin_config[index] (reading pin->chan to select the…
- [Medium] KSZ8463 TOU enable bit is left set in the hardware if the
  activation verification check fails.

Pre-existing issues:
- [High] A non-normalized periodic-output period whose nanosecond value
  converts to zero is accepted, stored as the global restart period, and…
- [Low] The KSZ periodic-output implementation accepts and processes
  PTP_PEROUT_DUTY_CYCLE (ksz_ptp_get_pin() returns -EOPNOTSUPP only for…
- [Low] ksz_ptp_clock_unregister() only calls ptp_clock_unregister(); it
  never disables the trigger units that the perout path enabled.

--

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

This adds KSZ8463 specific periodic output handling, since the chip has one
configuration register block per trigger unit, a smaller maximum pulse
width and 12 outputs instead of 2.  It wires new ksz8463_ptp_* callbacks
into a dedicated ksz8463_ptp_set_caps() and advertises 12 pins / 12
periodic output channels for the KSZ8463.

A few questions on the new code below.

[Severity: Medium]
Does this still build with CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=n?

This call site is not inside any #ifdef (it is only guarded by
"if (dev->irq > 0)"), and ksz_ptp.h declares the new symbol only in the
PTP-enabled branch:

drivers/net/dsa/microchip/ksz_ptp.h
  #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
  ...
  void ksz_ptp_set_caps(struct dsa_switch *ds);
  void ksz8463_ptp_set_caps(struct dsa_switch *ds);

The #else branch stubs every other PTP entry point that ksz8.c uses
(ksz_ptp_set_caps(), ksz_ptp_clock_register(), ksz_ptp_clock_unregister(),
ksz8463_ptp_irq_setup(), ksz8463_ptp_irq_free()) but has no stub for
ksz8463_ptp_set_caps():

  static inline void ksz_ptp_set_caps(struct dsa_switch *ds) { }

ksz_ptp.o is also not linked in that configuration, since the Makefile
guards it with "ifdef CONFIG_NET_DSA_MICROCHIP_KSZ_PTP".  Previously this
line called ksz_ptp_set_caps(), which does have a stub.  Would adding
"static inline void ksz8463_ptp_set_caps(struct dsa_switch *ds) { }" to the
#else branch be the intent?

[ ... ]

[Severity: High]
Does the software reset actually stop the pulse train, or does
KSZ8463_TOU_EN have to be cleared as well?

ksz8463_ptp_tou_reset() only pulses BIT(unit) in KSZ8463_TOU_SW_RST
and leaves KSZ8463_TOU_EN untouched, but that is the register
ksz8463_ptp_tou_start() writes to arm the unit.  Compare the generic
path: ksz_ptp_tou_reset() explicitly drops the enable bit as its last
step,

	/* Clear reset and set GPIO direction */
	return ksz_rmw32(dev, REG_PTP_CTRL_STAT__4, (TRIG_RESET | TRIG_ENABLE),
			 0);

while the KSZ8463 version has no equivalent for BIT(unit) in
KSZ8463_TOU_EN.

That matters for the disable path in ksz8463_ptp_enable_perout(),
which handles "on == 0" purely by calling ksz8463_ptp_tou_reset() and
then setting ptp_data->tou_mode to KSZ_PTP_TOU_IDLE.  If the enable
bit survives the reset, a PTP_PEROUT_REQUEST with a zero period keeps
the output toggling on the pin while software believes the unit is
idle, so settime/adjtime will no longer even realign it.  Reachable
from plain userspace: enable a perout on /dev/ptpN, then disable it.

Could you confirm against the datasheet whether writing SW_RST
self-clears TOU_EN?  If it does not, should the !on branch (or
ksz8463_ptp_tou_reset() itself, mirroring ksz_ptp_tou_reset()) also
clear BIT(unit) in KSZ8463_TOU_EN?

[Severity: Low]
This isn't an issue introduced by this patch, but once a unit has been
armed here via KSZ8463_TOU_EN, is anything expected to turn it off at
teardown?  ksz_ptp_clock_unregister() only does:

	ptp_data = &dev->ptp_data;

	if (ptp_data->clock)
		ptp_clock_unregister(ptp_data->clock);

so after unbinding the driver the enabled trigger units keep toggling the
pins with no owning software state.  The same gap already exists for the
ksz9477/lan937x trigger units, so this is only an observation about the new
users of that path.

[Severity: Medium]
Should this error path undo the KSZ8463_TOU_EN write that precedes
it?

ksz8463_ptp_tou_start() sets BIT(unit) in KSZ8463_TOU_EN and then
returns -EIO if the unit does not show up in KSZ8463_TOU_ACTIVE,
leaving the enable bit set in hardware.  The caller,
ksz8463_ptp_enable_perout(), propagates the error without setting
ptp_data->tou_mode, so the unit is left armed with no software state
claiming it.

The generic ksz_ptp_tou_start() can get away with a bare return
because of its "Unit will be reset on next access" assumption:
ksz_ptp_tou_reset() clears TRIG_ENABLE at the start of the next
enable.  As noted on ksz8463_ptp_tou_reset(), the KSZ8463 reset only
toggles KSZ8463_TOU_SW_RST and never touches KSZ8463_TOU_EN, so that
assumption does not carry over here and the stale enable bit is not
cleaned up on the next request either.

Would clearing BIT(unit) in KSZ8463_TOU_EN before returning -EIO be
the right thing, or is it preferable to make ksz8463_ptp_tou_reset()
clear the enable bit so the existing "reset on next access" pattern
holds for this chip too?

[Severity: Medium]
Which lock protects pin_config[] here?

pin_config is owned by the PTP core and documented as protected by
ptp_clock::pincfg_mux in drivers/ptp/ptp_private.h:

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

The core takes that mutex around the enable() callback, but the new restart
path reaches this function from clock_settime()/clock_adjtime() with only
ptp_data->lock held:

ksz8463_ptp_settime()/ksz8463_ptp_adjtime()
  -> ksz8463_ptp_restart_perout()
    -> ksz8463_ptp_enable_perout()
      -> ksz_ptp_get_pin()            /* calls ptp_find_pin() */
      -> ksz8463_ptp_configure_perout()

Meanwhile PTP_PIN_SETFUNC writes pin->func and pin->chan in
ptp_set_pinfunc() under pincfg_mux only.  If chan changes while this runs,
cfg_base can select a different trigger unit's register block than the one
ksz8463_ptp_tou_reset()/ksz8463_ptp_tou_start() operate on
(request->index), so one unit gets programmed while another gets reset and
enabled.

Note that simply switching to ptp_find_pin_unlocked() would invert the lock
order against the enable() path, which already holds pincfg_mux before
ptp_data->lock.  Would snapshotting pin index and chan under ptp_data->lock
at enable() time work better?

The bare ptp_find_pin() call from settime/adjtime predates this patch for
ksz9477/lan937x; the new part is deriving a register block address from
pin->chan on that path.

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

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but the new KSZ8463 path
inherits it, so it may be worth a look: can a period of 0 ns be stored and
later used as a divisor?

ksz_ptp_compute_perout_cycle() saves the request into ptp_data before
validating it, and only checks that the converted value fits the cycle
width mask:

	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) {

A non-normalized request with period.sec = -1 and period.nsec = 1000000000
is treated as "enable" by the core (perout->period.sec || perout->period.nsec
is true) and converts to 0 ns, which passes the mask check.  The default
pulse width then also computes to 0 and passes ksz_ptp_tou_pulse_verify().

No error path in ksz8463_ptp_enable_perout() resets tou_mode, so after a
previous successful enable the state stays KSZ_PTP_TOU_PEROUT with
perout_period converting to 0.  The next clock_settime()/clock_adjtime()
then runs ksz_ptp_prepare_restart_perout() with period_ns == 0:

	count = div_u64(now_ns - first_ns, period_ns);
	...
	while (next_ns < now_ns + 100000000)
		next_ns += period_ns;

which is a divide-by-zero on x86-64, and on arm64 (where UDIV by zero
returns 0) the loop never terminates while ptp_data->lock is held.  Would
rejecting a period whose nanosecond conversion is zero, and only committing
perout_period after validation, be appropriate here?

[Severity: High]
Can a periodic output on a channel other than 0 be restarted after a clock
step?  ksz_ptp_prepare_restart_perout() hardcodes the channel:

	request->index = 0;
	request->flags = 0;

So with an output running on, say, channel 5:

ksz8463_ptp_settime()
  -> ksz8463_ptp_restart_perout()
    -> ksz_ptp_prepare_restart_perout()   /* request->index = 0 */
      -> ksz8463_ptp_enable_perout()
        -> ksz_ptp_get_pin()
          -> ptp_find_pin(clock, PTP_PF_PEROUT, 0)   /* no match, -1 */

ksz_ptp_get_pin() then returns -EINVAL, which is propagated out of
ksz8463_ptp_settime()/ksz8463_ptp_adjtime() after
__ksz_ptp_settime()/__ksz_ptp_adjtime() already reprogrammed the hardware
RTC.  The output is never realigned, the ioctl reports failure although the
clock did change, and the "goto unlock" also skips the ptp_data->clock_time
update used by ksz_tstamp_reconstruct() (that mirror is otherwise only
re-synced from ksz_ptp_do_aux_work(), which runs only while a port has
hardware timestamping enabled).

The reachable sequence is a plain /dev/ptpN one: PTP_PIN_SETFUNC assigns
pin 5 to PTP_PF_PEROUT chan 5 (ksz_ptp_verify_pin() accepts PTP_PF_PEROUT
for any pin/chan and ptp_set_pinfunc() only requires chan < n_per_out = 12),
then PTP_PEROUT_REQUEST index 5 succeeds and sets tou_mode =
KSZ_PTP_TOU_PEROUT, then clock_settime() or clock_adjtime().

Two related points on the same singleton state, now that .n_per_out = 12 is
advertised for the KSZ8463:

If some pin does happen to be mapped to chan 0, the restart reprograms and
starts trigger unit 0 using the globally shared perout_period and
perout_target_time_first of whichever channel was configured last, so the
wrong unit is armed while the running unit keeps a stale target time.

Also, the !on branch of ksz8463_ptp_enable_perout() sets the single
tou_mode to KSZ_PTP_TOU_IDLE no matter which of the 12 units was disabled:

	if (!on) {
		ptp_data->tou_mode = KSZ_PTP_TOU_IDLE;
		return 0;
	}

after which settime/adjtime take the IDLE case and any still-enabled unit
is not restarted at all.

Should struct ksz_ptp_data keep tou_mode, perout_period and
perout_target_time_first per unit, with the channel index preserved in the
restart request?

[ ... ]

[Severity: Low]
This is a pre-existing omission rather than something this patch
introduces, but should supported_perout_flags be set here too?

The driver handles PTP_PEROUT_DUTY_CYCLE (ksz_ptp_get_pin() only rejects
"request->flags & ~PTP_PEROUT_DUTY_CYCLE", and
ksz_ptp_compute_perout_pulse() consumes request->on for it), yet
caps.supported_perout_flags is left at zero, and ptp_perout_request() does:

	if (perout->flags & ~ops->supported_perout_flags)
		return -EOPNOTSUPP;

so the duty-cycle branch cannot be reached from the UAPI.  For the KSZ8463
in particular the maximum pulse width is 500 us, so the default 50 % duty
cycle is always clamped and the pulse width cannot be chosen at all.  The
existing ksz_ptp_set_caps() has the same omission.

 void ksz_ptp_set_caps(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902-ksz-perout-v2-0-6f277fcc9e68%40bootlin.com