Re: [PATCH net-next v2 01/10] net: dsa: microchip: add the number of pins to chip infos

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

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

- [Medium] KSZ8463 loses its two PTP pins in this commit:
  `ksz_ptp_clock_register()` (drivers/net/dsa/microchip/ksz_ptp.c:1053)…
- [Low] Decoupling of `ptp_data->caps.n_pins` / pin_config loop bound
  from the fixed-size array `struct ptp_pin_desc…

--

net: dsa: microchip: add the number of pins to chip infos

This patch adds an n_pins field to struct ksz_chip_data and uses it in
ksz_ptp_clock_register() instead of the hardcoded KSZ_PTP_N_GPIO, both for
ptp_data->caps.n_pins and for the pin_config[] initialization loop.  The
new field is set to 2 for the switches marked ptp_capable.

[ ... ]

[Severity: Low]
This isn't a bug today, but the new n_pins field has nothing tying it to
the capacity of the array it indexes.  ksz_ptp.h still declares:

drivers/net/dsa/microchip/ksz_ptp.h:
	#define KSZ_PTP_N_GPIO		2
	...
	struct ptp_pin_desc pin_config[KSZ_PTP_N_GPIO];
	/* Serializes all operations on the PTP hardware clock */
	struct mutex lock;
	spinlock_t clock_lock;

I checked all ten entries that get .n_pins here and they are all 2, and
the later commit that raises KSZ8463 to 12 bumps KSZ_PTP_N_GPIO to 12 in
the same patch, so no entry ever exceeds the array.  Would it be worth
adding a static_assert() or a
BUILD_BUG_ON(n_pins <= ARRAY_SIZE(ptp_data->pin_config)) so a future chip
entry with a larger n_pins cannot walk past pin_config[] into lock and
clock_lock?

Also, is the const qualifier on n_pins intentional?  The other scalar
members of struct ksz_chip_data (u8 sgmii_port, port_cnt, ...) are plain.

[Severity: Medium]
Does this leave KSZ8463 with zero PTP pins at this commit?

The commit message says:

  "Set it to 2 for all the PTP-capable switches."

The .n_pins = 2 initializers are added only to the ten entries that carry
.ptp_capable = true (KSZ8563, KSZ9477, KSZ9563, KSZ8567, KSZ9567 and
LAN9370/71/72/73/74).  The [KSZ8463] entry in ksz_switch_chips[] does not
have .ptp_capable and does not get .n_pins, so dev->info->n_pins is 0 for
it.

KSZ8463 still reaches this function though, since ksz8463_setup() gates
the registration on the interrupt only:

drivers/net/dsa/microchip/ksz8.c:ksz8463_setup() {
	if (dev->irq > 0) {
		...
		ret = ksz8463_ptp_irq_setup(ds);
		...
		ret = ksz_ptp_clock_register(ds);

With caps.n_pins == 0 and the loop body never executed, no pin descriptor
is initialized, PTP_PIN_GETFUNC/PTP_PIN_SETFUNC reject every index, and

drivers/net/dsa/microchip/ksz_ptp.c:ksz_ptp_enable_perout() {
	pin = ptp_find_pin(ptp_data->clock, PTP_PF_PEROUT, request->index);
	if (pin < 0)
		return -EINVAL;

can never find a pin, so every PTP_PEROUT_REQUEST on a KSZ8463 with a
working interrupt returns -EINVAL, where two pins were advertised before
this patch.

The end of the series is fine: the later commit "net: dsa: microchip: add
periodic output support for the KSZ8463" adds .n_pins = 12 (and
.n_per_out = 12) to the [KSZ8463] entry.  Could the .n_pins value for
KSZ8463 be set in this patch instead, so that the intermediate commits of
the series do not regress periodic output on that chip?

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