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

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

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

- [High] KSZ8463 advertises 12 periodic-output channels (.n_pins = 12 /
  .n_per_out = 12) but the driver keeps only one set of software state…
- [Medium] Nothing in the new KSZ8463 path ever clears the per-unit
  enable bit in KSZ8463_TOU_EN (0x206).
- [Medium] Build break with CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=n:
  ksz8463_setup() (drivers/net/dsa/microchip/ksz8.c:2481) now calls…
- [Medium] ksz8463_ptp_enable_perout() performs destructive actions
  before validating the request and never reconciles state on failure.
- [Medium] ksz8463_ptp_configure_perout() (ksz_ptp.c:184) directly
  dereferences dev->ptp_data.pin_config[index], reading pin->chan to…
- [Low] Stale comment in the shared helper
  ksz_ptp_compute_perout_pulse(): 'Use a duty cycle of 50%.
- [Low] ksz8463_ptp_set_caps() (ksz_ptp.c:1335) does not set
  caps.supported_perout_flags, yet the KSZ8463 perout implementation…

Pre-existing issues:
- [High] A non-normalized periodic-output period is accepted and stored
  as zero nanoseconds, and the restart path then divides by it.
- [Medium] ksz_ptp_prepare_restart_perout() stores the 64-bit quotient
  div_u64(now_ns - first_ns, period_ns) in an 'unsigned int count'…

--

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

This adds KSZ8463-specific periodic output support to the microchip DSA
driver, with per-unit trigger configuration register blocks, a smaller
maximum pulse width, and 12 advertised outputs instead of 2.

A few questions on the new code below.

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

ksz8.o is built unconditionally, while the Makefile only adds ksz_ptp.o
inside "ifdef CONFIG_NET_DSA_MICROCHIP_KSZ_PTP".  In ksz_ptp.h, the new
declaration lives in the enabled branch:

  #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
  ...
  void ksz8463_ptp_set_caps(struct dsa_switch *ds);

and the #else branch only provides the old stub:

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

Every other KSZ8463-specific PTP entry point (ksz8463_ptp_irq_setup,
ksz8463_ptp_irq_free, ksz8463_get_ts_info, ksz8463_hwtstamp_set) does have
a stub or #define in the #else branch.  Should ksz8463_ptp_set_caps() get
one too, otherwise ksz8463_setup() is compiled with no declaration?

[ ... ]

[Severity: Medium]
Should this also clear BIT(unit) in KSZ8463_TOU_EN?

The generic counterpart just below deliberately ends by clearing
TRIG_ENABLE (and write-1-clears the per-unit status bits):

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

Here only KSZ8463_TOU_SW_RST is pulsed, and the disable path in
ksz8463_ptp_enable_perout() returns right after it:

  ret = ksz8463_ptp_tou_reset(dev, request->index);
  if (ret)
          return ret;

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

If asserting the software reset bit does not itself clear TOU_EN, does the
unit keep driving the pin after userspace disabled it while the driver
reports KSZ_PTP_TOU_IDLE?

There is a second consequence on the re-arm path, see the note on
ksz8463_ptp_tou_start() below.

 static int ksz_ptp_tou_reset(struct ksz_device *dev, u8 unit)
 {
 	u32 data;

[ ... ]

[Severity: Medium]
Continuing the note above: if BIT(unit) is still set in KSZ8463_TOU_EN when
the restart path reaches here, does ksz_rmw16() actually emit a write?

The KSZ8463 regmap config uses .cache_type = REGCACHE_NONE with no
reg_update_bits, and _regmap_update_bits() only writes when the value
changes:

  drivers/base/regmap/regmap.c:_regmap_update_bits() {
          ...
          if (force_write || (tmp != orig) || map->force_write_field) {
                  ret = _regmap_write(map, reg, tmp);
          ...
  }

So no 0->1 edge would be generated to latch the freshly written
TARGET_SEC/TARGET_NSEC, and the KSZ8463_TOU_ACTIVE check right after would
pass on the stale ACTIVE bit, making
ksz8463_ptp_settime()/ksz8463_ptp_adjtime() return success while the unit
still runs against the pre-step target time.

Also on the -EIO exit here, BIT(unit) is left set in KSZ8463_TOU_EN while
tou_mode is never advanced to KSZ_PTP_TOU_PEROUT.  Should the enable bit be
cleared before returning the error?

[Severity: Medium]
Is pin_config[] safe to dereference here on the settime/adjtime path?

pin_config[] is owned by the PTP core and protected by
ptp_clock::pincfg_mux, which the core holds only around ->enable():

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

The new path ksz8463_ptp_settime()/ksz8463_ptp_adjtime() ->
ksz8463_ptp_restart_perout() -> ksz8463_ptp_enable_perout() ->
ksz8463_ptp_configure_perout() only holds the driver-private
ptp_data->lock.  Can a concurrent PTP_PIN_SETFUNC change pin->chan
underneath, so that cfg_base points at a different trigger unit than
BIT(request->index) which ksz8463_ptp_tou_start() enables?

Note that simply taking pincfg_mux here would invert the order used by the
->enable() path (pincfg_mux then ptp_data->lock); caching pin/chan under
pincfg_mux at enable() time 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;
+}
+
 static int ksz_ptp_configure_perout(struct ksz_device *dev,
 				    u32 cycle_width_ns, u32 pulse_width_ns,
 				    struct timespec64 const *target_time,

[ ... ]

[Severity: Medium]
Can a rejected request leave the driver and hardware out of sync here?

The unit is reset before any of the timing values are validated, and
ksz_ptp_compute_perout_cycle() stores the requested period before deciding
it is out of range:

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

So with a perout already active (tou_mode == KSZ_PTP_TOU_PEROUT), a
PTP_PEROUT_REQUEST with an out-of-range period stops the running unit,
overwrites perout_period with the rejected value, and returns while
tou_mode still says PEROUT.  Every later
ksz8463_ptp_settime()/ksz8463_ptp_adjtime() then rebuilds the request from
that stored period and fails again, after the RTC has already been
written.  An SPI error in one of the six writes in
ksz8463_ptp_configure_perout() gives the same half-programmed state.
Should validation happen before the unit reset and before the state is
committed?

+	pulse_width_ns = ksz_ptp_compute_perout_pulse(dev, request,
+						      KSZ8463_MAX_PULSE_WIDTH);

[Severity: Low]
This isn't a bug, but the comment in ksz_ptp_compute_perout_pulse() is now
stale for this caller:

  /* Use a duty cycle of 50%. Maximum pulse width supported by the
   * hardware is a little bit more than 125 ms.
   */

The maximum is a parameter now, and this call passes
KSZ8463_MAX_PULSE_WIDTH = 500000LL, i.e. 0.5 ms.  Could the comment be
made generic or moved to the callers?

+
+	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;
+}
+
 static int ksz_ptp_enable_perout(struct ksz_device *dev,
 				 struct ptp_perout_request const *request,
 				 int on)

[ ... ]

[Severity: High]
Does this work for channels other than 0?

ksz_ptp_prepare_restart_perout() hardcodes the channel and drops the flags:

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

but this chip now advertises .n_pins = 12 and .n_per_out = 12, and
struct ksz_ptp_data keeps a single tou_mode / perout_target_time_first /
perout_period for all of them.

With a perout armed on, say, channel 3 (PTP_PIN_SETFUNC accepts it,
ksz_ptp_verify_pin() only filters on func and the core only checks
chan < n_per_out), the restart path becomes:

  ksz8463_ptp_adjtime()
    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)   /* no such pin */

so -EINVAL is returned, and this happens after __ksz_ptp_adjtime() has
already stepped the RTC:

  ret = __ksz_ptp_adjtime(dev, delta);
  if (ret)
          goto unlock;
  ...
  case KSZ_PTP_TOU_PEROUT:
          ret = ksz8463_ptp_restart_perout(dev);
          if (ret)
                  goto unlock;

The "goto unlock" also skips the ptp_data->clock_time update.  Since
ptp4l/phc2sys call adjtime/adjfine continuously, would every PHC adjust
fail for 11 of the 12 channels the commit advertises?  And if some pin does
happen to be mapped to channel 0, unit 0 gets reprogrammed with whatever
timing is stored in the single set of fields while the genuinely active
unit keeps its pre-step target.

Related to the single set of state fields: units 1..11 are never re-armed,
"request->flags = 0" discards PTP_PEROUT_DUTY_CYCLE on every restart,
enabling a second channel overwrites the first channel's stored timing, and
disabling any one channel sets the global tou_mode to KSZ_PTP_TOU_IDLE
while the other units stay enabled in hardware.  Should the per-unit state
be tracked per channel, and should the restart carry the real index?

[Severity: High]
This isn't a bug introduced by this patch, but the new caller of
ksz_ptp_prepare_restart_perout() adds a second path to it, so it is worth
raising: can a stored period of zero nanoseconds divide by zero here?

struct ptp_clock_time has a signed sec and an unsigned nsec, and
ptp_perout_request() validates flags, reserved fields, duty
cycle/phase and "index < n_per_out", but never normalization or
positivity; it decides "on" from "period.sec || period.nsec".  A period of
{sec = -1, nsec = 1000000000} is therefore treated as an enable, while
timespec64_to_ns() evaluates it to 0, which passes both checks:

  *cycle_width_ns = timespec64_to_ns(&ptp_data->perout_period);
  if ((*cycle_width_ns & TRIG_CYCLE_WIDTH_M) != *cycle_width_ns) {

and ksz_ptp_tou_pulse_verify(0) passes too.  A later settime/adjtime then
reaches:

  period_ns = timespec64_to_ns(&ptp_data->perout_period);

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

which is either a divide-error with a zero divisor, or a loop that never
advances, both with ptp_data->lock held.  Should non-normalized times be
rejected and the converted period required to be strictly positive before
it is stored?

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but this new
function adds another caller: ksz_ptp_prepare_restart_perout() stores the
64-bit quotient in an unsigned int:

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

Once more than UINT_MAX periods have elapsed (about 72 minutes with a 1 us
period, about 50 days with 1 ms), count truncates and the guard-time loop
has to catch up one period at a time, up to roughly 4.29e9 iterations with
ptp_data->lock held.  Would a u64 count avoid that stall?

 static int __ksz_ptp_settime(struct ksz_device *dev,
 			     const struct timespec64 *ts)
 {

[ ... ]

[Severity: Low]
Should caps.supported_perout_flags be set to PTP_PEROUT_DUTY_CYCLE here?

The implementation accepts that flag:

  ksz_ptp_get_pin():
          if (request->flags & ~PTP_PEROUT_DUTY_CYCLE)
                  return -EOPNOTSUPP;

and ksz_ptp_compute_perout_pulse() consumes request->on when it is set, but
the core rejects it first:

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

so with supported_perout_flags left at 0 the duty-cycle handling is
unreachable.  The same omission exists in ksz_ptp_set_caps().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-ksz-perout-v1-0-14202db763b3%40bootlin.com