Re: [PATCH net-next v3 5/5] net: dsa: ks8995: Delete surplus driver

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Silent loss of switch driver functionality on kernel upgrade due to missing Kconfig migration.

Pre-existing issues:
- [High] The replacement driver (ksz_spi) fails to probe for devices instantiated via platform data (non-DT) because it lacks driver_data in its spi_device_id table.
- [High] The replacement driver fails to explicitly enable the Port 5 PHY mode, a required initialization step performed by the old driver.
- [High] The MTU limit logic in ksz8863_change_mtu is completely inverted for packet sizes between 1523 and 1536 bytes, causing hardware packet drops.
--

commit 19ced6cb5c2f1c58320168d553069a57634b8e9d
Author: Linus Walleij <linusw@kernel.org>

net: dsa: ks8995: Delete surplus driver
    
The Microchip ksz driver now handles all switches that the
old driver was handling, but better.
    
Delete the old driver.

> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index e704ab702c189..7e6d98ebbd522 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -97,15 +97,6 @@ config NET_DSA_RZN1_A5PSW
>  	  This driver supports the A5PSW switch, which is embedded in Renesas
>  	  RZ/N1 SoC.
>  
> -config NET_DSA_KS8995
> -	tristate "Micrel KS8995 family 5-ports 10/100 Ethernet switches"
> -	depends on SPI
> -	depends on !NET_DSA_MICROCHIP_KSZ_SPI
> -	select NET_DSA_TAG_NONE
> -	help
> -	  This driver supports the Micrel KS8995 family of 10/100 Mbit ethernet
> -	  switches, managed over SPI.
> -

[Severity: Medium]
Will completely removing CONFIG_NET_DSA_KS8995 cause a silent loss of switch
driver functionality on kernel upgrades?

By not providing a Kconfig transition path, users with CONFIG_NET_DSA_KS8995
enabled will have the driver silently dropped from their configurations during
make oldconfig. 

Does it make sense to leave a dummy NET_DSA_KS8995 config that selects the
new NET_DSA_MICROCHIP_KSZ_SPI driver to ensure a smooth transition?

> diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c
> deleted file mode 100644
> index 77d8b842693c1..0000000000000
> --- a/drivers/net/dsa/ks8995.c
> +++ /dev/null
[ ... ]
> -static int ks8995_check_config(struct ks8995_switch *ks)
> -{
> -	int ret;
> -	u8 val;
> -
> -	ret = ks8995_read_reg(ks, KS8995_REG_GC0, &val);
> -	if (ret) {
> -		dev_err(ks->dev, "failed to read KS8995_REG_GC0\n");
> -		return ret;
> -	}
> -
> -	dev_dbg(ks->dev, "port 5 PHY %senabled\n",
> -		(val & KS8995_GC0_P5_PHY) ? "" : "not ");
> -
> -	val |= KS8995_GC0_P5_PHY;
> -	ret = ks8995_write_reg(ks, KS8995_REG_GC0, val);

[Severity: High]
This isn't a bug introduced by this patch, but removing this driver exposes a
potential regression in the replacement driver.

The replacement generic Microchip ksz driver does not appear to configure
KS8995_GC0_P5_PHY (BIT 3 in REG 0x02) in its setup path for KSZ8995XA
switches. According to the datasheet, the hardware default for this bit
is 0 (disabled).

For hardware designs that rely on software to configure Port 5 into PHY mode
rather than hardware strapping, won't this missing initialization leave the PHY
disabled and break network connectivity for migrated users?

[ ... ]
> -static int ks8995_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> -{
> -	struct ks8995_switch *ks = ds->priv;
> -	unsigned int max_mtu;
> -	int ret;
> -	u8 val;
> -	int i;
[ ... ]
> -	if (max_mtu <= 1522) {
> -		val &= ~KS8995_GC2_HUGE;
> -		val &= ~KS8995_GC2_LEGAL;
> -	} else if (max_mtu > 1522 && max_mtu <= 1536) {
> -		/* This accepts packets up to 1536 bytes */
> -		val &= ~KS8995_GC2_HUGE;
> -		val |= KS8995_GC2_LEGAL;
> -	} else {
> -		/* This accepts packets up to 1916 bytes */
> -		val |= KS8995_GC2_HUGE;
> -		val |= KS8995_GC2_LEGAL;
> -	}

[Severity: High]
This is a pre-existing issue in the replacement driver, but forcing users
off the old driver will expose them to it.

The old driver correctly overrides the 1522-byte limit to allow frames up
to 1536 bytes by setting KS8995_GC2_LEGAL (BIT 1). However,
ksz8863_change_mtu() in the ksz8.c driver appears to have the exact
opposite logic:

    if (frame_size <= legalsz)
        ctrl2 |= KSZ8863_LEGAL_PACKET_ENABLE;

For frame sizes between 1523 and 1536 bytes, this condition is false, leaving
ctrl2 = 0. 

Doesn't this clear the bit and strictly enforce the 1522-byte limit, causing
the hardware to drop these packets instead of allowing them?

[ ... ]
> -static const struct spi_device_id ks8995_id[] = {
> -	{"ks8995", ks8995},
> -	{"ksz8864", ksz8864},
> -	{"ksz8795", ksz8795},
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(spi, ks8995_id);

[Severity: High]
This isn't a bug introduced by this patch, but migrating non-DeviceTree
setups to the ksz_spi replacement driver will result in probe failures.

The deleted driver successfully supported non-DT SPI instantiation because
it read the variant directly via spi_get_device_id(spi)->driver_data. 

The replacement ksz_spi driver relies on device_get_match_data(), which
requires the spi_device_id table to populate the .driver_data field for
fallback matching on non-DT systems. Since ksz_spi_ids only provides string
names (e.g., { "ks8995" }) with a default 0/NULL driver_data,
device_get_match_data() will return NULL and the probe will immediately
abort with -EINVAL. 

Will this stranding of non-DT users require an update to ksz_spi_ids in the
replacement driver before deleting this one?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-ks8995-to-ksz8-v3-0-c530f651989f@kernel.org?part=5