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

[netdev-bot+sashiko@kernel.org]

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.

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 1a9d6f83a0232..d01fba253af57 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1194,6 +1194,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.ptp_capable = true,
>  		.wr_table = &ksz8563_register_set,
>  		.rd_table = &ksz8563_register_set,
> +		.n_pins = 2,
>  	},
>  
>  	[KSZ8795] = {

[ ... ]

> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index cbe98494578c3..c678fc6597dc4 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -139,6 +139,7 @@ struct ksz_chip_data {
>  	u8 sgmii_port;
>  	const struct regmap_access_table *wr_table;
>  	const struct regmap_access_table *rd_table;
> +	const u8 n_pins;
>  };

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

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index 39cc70d65900b..c92936bf78d3d 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -1050,14 +1050,14 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
>  	ptp_data->caps.do_aux_work	= ksz_ptp_do_aux_work;
>  	ptp_data->caps.enable		= ksz_ptp_enable;
>  	ptp_data->caps.verify		= ksz_ptp_verify_pin;
> -	ptp_data->caps.n_pins		= KSZ_PTP_N_GPIO;
> +	ptp_data->caps.n_pins		= dev->info->n_pins;
>  	ptp_data->caps.n_per_out	= 3;
>  
>  	ret = ksz_ptp_start_clock(dev);
>  	if (ret)
>  		return ret;
>  
> -	for (i = 0; i < KSZ_PTP_N_GPIO; i++) {
> +	for (i = 0; i < dev->info->n_pins; i++) {
>  		struct ptp_pin_desc *ptp_pin = &ptp_data->pin_config[i];
>  
>  		snprintf(ptp_pin->name,

[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