Re: [PATCH net-next v6 4/5] net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

This patch adds support for the Microchip KSZ8995XA (a.k.a. Micrel
KS8995XA) switch to the KSZ DSA driver, introducing a new chip entry, a
restricted dev_ops/switch_ops set, and a ksz_is_ksz8995xa() helper for
future KSZ8995E/MA differentiation.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index 0f84b2e7d25d..0b02588e22fc 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -137,7 +138,18 @@ static int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 dat
>  
>  static int ksz8_reset_switch(struct ksz_device *dev)
>  {
> -	if (ksz_is_ksz88x3(dev)) {
> +	if (ksz_is_ksz8995xa(dev)) {
> +		unsigned int port;
> +
> +		/* KSZ8995XA is "reset" by stopping the switch and starting it */
> +		ksz_cfg(dev, REG_SW_ID_1, SW_START_SWITCH, false);
> +		usleep_range(10, 100);
> +		ksz_cfg(dev, REG_SW_ID_1, SW_START_SWITCH, true);

[Medium]
For KSZ8995XA, ksz8_reset_switch() asserts SW_START_SWITCH before
ksz8_setup() programs broadcast storm rate, REG_SW_CTRL_2,
REG_SW_CTRL_9 SW_SPECIAL_TAG, port-membership and the
PORT_INSERT_TAG/PORT_REMOVE_TAG bits. Other variants in this same file
(for example ksz88x3) keep the forwarding engine quiescent during
configuration and only assert SW_START at the end of ksz8_setup() via:

    regmap_update_bits(ksz_regmap_8(dev), regs[S_START_CTRL],
                       SW_START, SW_START);

For KSZ8995XA, ksz8995xa_regs has [S_START_CTRL] = 0x01, the same
register written here, so by the time ksz8_setup() reaches its final
"start switch" step the engine has been forwarding throughout the
reconfiguration window. Is that intentional? Could the switch be left
stopped here and only started at the end of ksz8_setup() like the
other variants?

> +
> +		/* Often boot loaders have fun with ports, so reset them */
> +		for (port = 0; port < dev->info->port_cnt; port++)
> +			ksz_port_cfg(dev, port, 0x0f, BIT(4), true);

[Medium]
This writes BIT(4) at port-relative offset 0x0F once per port and
never clears it, with no readback or settling delay before the rest of
the setup proceeds to read/write port registers. If that bit is a
level-triggered reset, every port is left held in reset; if it is
self-clearing or a feature bit, the code happens to work.

The KSZ8863 path in this same function explicitly clears its reset
bit:

    ksz_cfg(dev, KSZ8863_REG_SW_RESET,
            KSZ8863_GLOBAL_SOFTWARE_RESET | KSZ8863_PCS_RESET, true);
    ksz_cfg(dev, KSZ8863_REG_SW_RESET,
            KSZ8863_GLOBAL_SOFTWARE_RESET | KSZ8863_PCS_RESET, false);

and the KSZ87xx path uses an explicit power-down/power-up pair. Could
this code use named macros in ksz8_reg.h (matching the other named
constants added in this patch like SW_SPECIAL_TAG, SW_START_SWITCH,
PORT_INSERT_TAG, PORT_REMOVE_TAG) and document whether the bit is
self-clearing or whether a paired clear is needed?

> +	} else if (ksz_is_ksz88x3(dev)) {
>  		/* reset switch */
>  		ksz_cfg(dev, KSZ8863_REG_SW_RESET,
>  			KSZ8863_GLOBAL_SOFTWARE_RESET | KSZ8863_PCS_RESET, true);

[ ... ]

> @@ -1693,14 +1729,16 @@ static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  		offset = P1CR1;
>  	ksz_port_cfg(dev, port, offset, PORT_BROADCAST_STORM, true);
>  
> -	ksz8_port_queue_split(dev, port, dev->info->num_tx_queues);
> +	if (!ksz_is_ksz8995xa(dev)) {
> +		ksz8_port_queue_split(dev, port, dev->info->num_tx_queues);

[Medium]
ksz_chip_data for KSZ8995XA sets .num_tx_queues = 4, and ksz8_setup()
unconditionally does:

    ds->num_tx_queues = dev->info->num_tx_queues;

But ksz8_port_setup() now skips ksz8_port_queue_split() for
KSZ8995XA, so the chip's queue-split hardware registers are left at
their power-on default while DSA is told there are 4 TX queues. If the
default is not a 4-queue split, classification decisions made by the
stack (priority/PCP/DSCP-to-queue mapping) will not correspond to the
queues the hardware actually uses.

Should .num_tx_queues reflect the actual default for this chip, or
should the queue-split registers be programmed?

[ ... ]

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 57c8abfe0147..ac27beb3054b 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1485,6 +1499,21 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.internal_phy = {true, true, true, true, false},
>  	},
>  
> +	[KSZ8995XA] = {
> +		.chip_id = KSZ8995XA_CHIP_ID, /* Also known as KS8995XA */
> +		.dev_name = "KSZ8995XA",
> +		.cpu_ports = 0x10,	/* can be configured as cpu port */
> +		.port_cnt = 5,		/* total cpu and user ports */
> +		.num_tx_queues = 4,
> +		.num_ipms = 4,
> +		.ops = &ksz8995xa_dev_ops,
> +		.switch_ops = &ksz8995xa_switch_ops,
> +		.phylink_mac_ops = &ksz88x3_phylink_mac_ops,
> +		.regs = ksz8995xa_regs,
> +		.supports_mii = {true, true, true, true, true},
> +		.internal_phy = {true, true, true, true, false},
> +	},

[Low]
The KSZ8995XA chip data omits .num_vlans, .num_alus, and .num_statics,
so they default to 0. ksz8_setup() then unconditionally calls:

    dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
                                   dev->info->num_vlans, GFP_KERNEL);
    if (!dev->vlan_cache)
            return -ENOMEM;

devm_kcalloc with n = 0 returns ZERO_SIZE_PTR, which is non-NULL and
therefore not caught by the !dev->vlan_cache check. Today every
dereference of vlan_cache is gated by !ksz_is_ksz8995xa(dev) or by the
absence of VLAN ops in ksz8995xa_switch_ops, so there is no crash, but
any future shared helper that touches vlan_cache without a chip-id
gate would dereference ZERO_SIZE_PTR.

Could the allocation be skipped for KSZ8995XA, or could .num_vlans be
set to a value that makes the !NULL check meaningful?

[ ... ]

> @@ -2826,6 +2855,10 @@ void ksz_init_mib_timer(struct ksz_device *dev)
>  {
>  	int i;
>  
> +	/* KSZ8995XA lacks MiB features */
> +	if (ksz_is_ksz8995xa(dev))
> +		return;
> +
>  	INIT_DELAYED_WORK(&dev->mib_read, ksz_mib_read_work);

[Low]
This early return skips INIT_DELAYED_WORK(&dev->mib_read, ...). The
patch then relies on the undocumented invariant that mib_read_interval
== 0 means dev->mib_read must not be touched.

ksz_suspend()/ksz_resume() in ksz_common.c call
cancel_delayed_work_sync(&dev->mib_read) and
schedule_delayed_work(&dev->mib_read, ...) without any chip-id gate.
ksz8995xa_switch_ops happens to omit .suspend/.resume today, so there
is no current crash, but any future addition of those ops or any other
unguarded user of dev->mib_read on this chip will operate on
uninitialised work_struct memory.

Would it be safer to call INIT_DELAYED_WORK unconditionally and only
skip the schedule for KSZ8995XA?

[ ... ]

> @@ -4662,6 +4700,10 @@ int ksz_switch_register(struct ksz_device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* Override ops with something simpler for this legacy chip */
> +	if (ksz_is_ksz8995xa(dev))
> +		dev->ds->ops = &ksz8995xa_switch_ops;
> +

[Low]
Is this assignment a no-op in the supported flow? ksz_switch_alloc()
has already set ds->ops = chip->switch_ops, and
ksz_switch_chips[KSZ8995XA].switch_ops is &ksz8995xa_switch_ops, so
the same pointer is being written back.

The comment "Override ops with something simpler for this legacy chip"
implies a transformation that does not actually occur, and no other
chip in the family does this. Could this hunk be dropped, or the
comment reworded to match what the code does?

> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index b67038cf1bd0..661a82026e47 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -751,7 +752,13 @@ static inline bool ksz_is_ksz88x3(struct ksz_device *dev)
>  static inline bool ksz_is_8895_family(struct ksz_device *dev)
>  {
>  	return dev->chip_id == KSZ8895_CHIP_ID ||
> -	       dev->chip_id == KSZ8864_CHIP_ID;
> +	       dev->chip_id == KSZ8864_CHIP_ID ||
> +	       dev->chip_id == KSZ8995XA_CHIP_ID;
>  }

[Low]
This silently expands ksz_is_8895_family() to include KSZ8995XA, which
through is_ksz8()/is_ksz88xx() classifies the new chip as an 8895
family member across ksz_dcb.c, ksz8.c and ksz_common.c.

Today every potentially-divergent path is explicitly gated by
ksz_is_ksz8995xa() opt-outs or by the dev->ds->ops override, but this
"opt in via family helper, then opt out via per-chip checks" pattern
is brittle: future helper-driven logic touching the 8895 family will
silently include KSZ8995XA, and comments such as "/* KSZ8864, KSZ8895
*/" in ksz88xx_get_tag_protocol/ksz88xx_connect_tag_protocol become
inaccurate.

The commit message also does not acknowledge this expansion or
explain why a feature-bit approach in ksz_chip_data was not used,
especially given that the KSZ8995E/MA variants are mentioned as
future work. Could the commit message cover this design choice, or
could the family classification be reconsidered?