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

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

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

- [Medium] The generic legacy compatible "micrel,ks8995" is newly bound
  to ksz_switch_chips[KSZ8995XA] in drivers/net/dsa/microchip/ksz_spi.c,…
- [Low] ksz_spi.c now matches three compatible strings —
  "micrel,ksz8864", "micrel,ksz8795" and "micrel,ks8995" — that do not…
- [Low] The new KSZ8995XA branch of ksz8_reset_switch() iterates ports
  0..dev->info->port_cnt-1 (5 ports) writing PORT_PHY_SOFT_RESET…
- [Low] ksz8_port_vlan_filtering() gained a special case `if
  (ksz_is_ksz88x3(dev) || ksz_is_ksz8995xa(dev)) return -ENOTSUPP;`,…

--

net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

This adds KSZ8995XA (Micrel KS8995XA) support to the KSZ driver, with a
restricted set of dev/switch ops, a header-tag protocol, one-bit-per-DSCP
priority registers and reworked DCB register helpers.  It also moves the
legacy Micrel compatibles over to the KSZ SPI driver and makes the old
ks8995 driver mutually exclusive with it in Kconfig.

[ ... ]

[Severity: Low]
This isn't a bug, but should this loop use dev->phy_port_cnt rather than
dev->info->port_cnt?

The chip data for KSZ8995XA has .port_cnt = 5 and
.internal_phy = {true, true, true, true, false}, so the last iteration
writes PORT_PHY_SOFT_RESET to the port 5 register block, and ksz8_reg.h
only names REG_PORT_1..4_STATUS_3.  Other PHY-oriented loops in ksz8.c
bound on dev->phy_port_cnt.

The legacy driver does define KS8995_GC0_P5_PHY BIT(3) /* Port 5 PHY
enabled */ and its KS8995_REG_PS(p, r) macro is valid for p = 0..4, so
this register does appear to exist on the hardware; it is just
inconsistent with how internal_phy[] is declared here.

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

[ ... ]

[Severity: Low]
Can this new ksz_is_ksz8995xa() branch ever be reached?

ksz8995xa_switch_ops (added further down in this patch) does not set
.port_vlan_filtering, and the core checks that first:

net/dsa/port.c:dsa_port_vlan_filtering() {
	...
	if (!ds->ops->port_vlan_filtering)
		return -EOPNOTSUPP;
	...
}

So for a KSZ8995XA device ksz8_port_vlan_filtering() is never entered and
the added condition looks like dead code that suggests the op is wired up.
As a side note, the existing return here is -ENOTSUPP while the core path
already returns the canonical -EOPNOTSUPP.

[ ... ]

[ ... ]

+	.port_get_apptrust	= ksz_port_get_apptrust,
+	.port_set_apptrust	= ksz_port_set_apptrust,
+};

[Severity: Medium]
The commit message says:

  "Notice: there are also KSZ8995E and KSZ8995MA. These are BOTH
   different from the KSZ8995XA."

Given that, is it safe for family 0x95 with chip ID nibble 0x0 to be
mapped unconditionally to KSZ8995XA_CHIP_ID here?

The legacy driver attributes that same ID to KS8995MA:

drivers/net/dsa/ks8995.c:
	[ks8995] = {
		.name		= "KS8995MA",
		.family_id	= FAMILY_KS8995,
		.chip_id	= KS8995_CHIP_ID,

and the new define in this patch is:

	#define KSZ8895_CHIP_ID_95XA		0x0

Combined with the new "micrel,ks8995" entry in ksz_dt_ids pointing at
ksz_switch_chips[KSZ8995XA], and with NET_DSA_KS8995 now disabled
whenever NET_DSA_MICROCHIP_KSZ_SPI is enabled, an MA part behind the
generic compatible would be probed and programmed as XA hardware:
SW_SPECIAL_TAG in REG_SW_CTRL_9, per-port PORT_INSERT_TAG /
PORT_REMOVE_TAG, PVID 0, the descending one-bit-per-DSCP registers, the
single-bit port-based priority and the inverted packet-size polarity,
plus DSA_TAG_PROTO_KS8995 from ksz8995xa_get_tag_protocol().

Two in-tree device trees use the generic string today,
arch/arm/boot/dts/intel/ixp/intel-ixp42x-linksys-wrv54g.dts and
intel-ixp42x-actiontec-mi424wr.dtsi, so if MA behaves differently in
special-tag mode those boards would lose CPU-port traffic.

For context, the later "net: dsa: ks8995: Delete surplus driver" patch in
the series records that this ID cannot distinguish MA from XA, that no
KS8995MA users are known and that neither variant has been verified, but
it does not change this mapping.  Could this detection either reject the
ambiguous case or at least warn?

+		ret = ksz_read8(dev, REG_KSZ8864_CHIP_ID, &id4);
+		if (ret)
+			return ret;

[ ... ]

[ ... ]

[Severity: Low]
Should the three Micrel strings also be added to the compatible enum in
Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml?

The series adds "microchip,ksz8995xa" to that enum, but "micrel,ksz8864",
"micrel,ksz8795" and "micrel,ks8995" are not listed anywhere under
Documentation/, and the legacy ks8995 driver is removed later in the
series, so the KSZ schema becomes the only place that could describe
them.  As a result the two ixp42x device trees using "micrel,ks8995"
stay unvalidated by dt_binding_check / dtbs_check.  Listing them (for
instance as deprecated) would close that gap.

+	{
+		.compatible = "microchip,ksz8463",
+		.data = &ksz_switch_chips[KSZ8463]

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906-ks8995-to-ksz8-v9-0-8d8815a91bd8%40kernel.org