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

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index f2a23e2e474ac..6b08bb8fed5b7 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -2478,7 +2478,7 @@ static int ksz8463_setup(struct dsa_switch *ds)
>  		if (ret)
>  			goto free_girq;
>  
> -		ksz_ptp_set_caps(ds);
> +		ksz8463_ptp_set_caps(ds);

[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?

>  		ret = ksz_ptp_clock_register(ds);
>  		if (ret) {
>  			dev_err(dev->dev, "Failed to register PTP clock: %d\n",
> 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,
>  	},
>  
>  	[KSZ8563] = {
> 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;

[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.

> +
> +	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;
> +}
> +
>  static int ksz_ptp_tou_start(struct ksz_device *dev, u8 unit)
>  {
>  	u32 data;
> @@ -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]
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;
> +

[ ... ]

> +	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,
> @@ -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: 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?

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

[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?

> +
>  static int __ksz_ptp_settime(struct ksz_device *dev,
>  			     const struct timespec64 *ts)
>  {
> @@ -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;
> +}
> +
>  static int ksz_ptp_settime(struct ptp_clock_info *ptp,
>  			   const struct timespec64 *ts)
>  {

[ ... ]

> @@ -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]
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