Re: [PATCH net v2] net: dsa: microchip: fix KSZ8765 fiber detection

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message does not say what users actually see, and
  its Fixes: tag points at the wrong commit.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  ksz_switch_detect() (drivers/net/dsa/microchip/ksz_common.c:2931)…

--

net: dsa: microchip: fix KSZ8765 fiber detection

This commit changes KSZ8_PORT_STATUS_0 from 0x08 to 0x18. With that,
ksz_switch_detect() reads the Port 1 Status 0 register to check the Fiber
Mode bit that tells a KSZ8765 apart from a KSZ8795, instead of reading the
Global Control 6 MIB Control register.

> Fixes: 91a98917a883 ("net: dsa: microchip: move switch chip_id detection to ksz_common")

[Severity: Medium]

Is this the right commit for the Fixes: tag? 91a98917a883 seems to only
move the detection code into ksz_common. Before that commit,
ksz8_switch_detect() already did a global read of the same 0x08 offset:

drivers/net/dsa/microchip/ksz8795.c:ksz8_switch_detect() {
    ...
			id2 = 0x95;
			ksz_read8(dev, REG_PORT_STATUS_0, &val);
			if (val & PORT_FIBER_MODE)
				id2 = 0x65;
    ...
}

The wrong offset appears to come from 4b20a07e103f ("net: dsa: microchip:
ksz8795: add support for ksz88xx chips"). That commit replaced the
REG_PORT_1_STATUS_0 (0x18) read with REG_PORT_STATUS_0 (0x08), which is a
per-port offset.

Stable trees that have 4b20a07e103f but not 91a98917a883 (5.15.y, for
example) also have this bug. Tooling that follows the current tag would
skip them. Should the tag be this instead?

Fixes: 4b20a07e103f ("net: dsa: microchip: ksz8795: add support for ksz88xx chips")

Could the commit message also describe what users see? With the old value,
bit 7 of REG_SW_CTRL_6 (SW_MIB_COUNTER_FLUSH) reads 0 after power-on. A
KSZ8765 is then detected as KSZ8795, and on a board using the
"microchip,ksz8765" compatible, ksz_check_device_id() fails the probe with
-ENODEV:

    Device tree specifies chip KSZ8765 but found KSZ8795, please fix it!

There is also a change in the other direction. Today, a KSZ8765 board
described as "microchip,ksz8795" probes. After this patch it would be
detected as KSZ8765 and fail with -ENODEV. No in-tree DTS uses either
compatible, but would it be worth mentioning this for backporters?

> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index cbe98494578c3..f6104f714a8d1 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -853,7 +853,7 @@ static inline bool ksz_is_sgmii_port(struct ksz_device *dev, int port)
>  #define KSZ88_FAMILY_ID			0x88
>  #define KSZ8895_FAMILY_ID		0x95
>  
> -#define KSZ8_PORT_STATUS_0		0x08
> +#define KSZ8_PORT_STATUS_0		0x18

[Severity: Low]

This isn't a bug introduced by this patch, but the read in
ksz_switch_detect() that uses this register does not check its return
value:

drivers/net/dsa/microchip/ksz_common.c:ksz_switch_detect() {
    ...
			dev->chip_id = KSZ8795_CHIP_ID;

			ksz_read8(dev, KSZ8_PORT_STATUS_0, &val);
			if (val & KSZ8_PORT_FIBER_MODE)
				dev->chip_id = KSZ8765_CHIP_ID;
    ...
}

If regmap_read() fails, ksz_read8() logs an error but still copies its
uninitialized local into *val:

drivers/net/dsa/microchip/ksz_common.h:ksz_read8() {
	unsigned int value;
	int ret = regmap_read(ksz_regmap_8(dev), reg, &value);
    ...
	*val = value;
	return ret;
}

Could the fiber mode test then use uninitialized stack data to set
dev->chip_id? ksz_check_device_id() would catch any resulting mismatch.
However, the probe would then fail with -ENODEV and the misleading "please
fix it" message instead of returning the real I/O error.

The other register reads in ksz_switch_detect() check for errors. Now that
this read is meaningful, would it make sense to do the same here?

	ret = ksz_read8(dev, KSZ8_PORT_STATUS_0, &val);
	if (ret)
		return ret;

>  #define KSZ8_PORT_FIBER_MODE		BIT(7)
>  
>  #define SW_CHIP_ID_M			GENMASK(7, 4)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007080557.15719-1-sebastien.royen%40armadeus.com