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

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

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

- [Medium] ksz8995xa_change_mtu() (drivers/net/dsa/microchip/ksz8.c:301)
  programs the hardware legal-packet-size limit from frame_size = mtu +…
- [Low] After this patch (and the final patch of the series, which
  deletes drivers/net/dsa/ks8995.c entirely),…
- [Low] Concern (validated as FALSE POSITIVE / disclosed later in
  series): the Kconfig exclusivity silently re-routes the existing…
- [Low] Concern (validated as FALSE POSITIVE): the new `{ "ks8995" }` and
  `{ "ksz8995xa" }` entries in ksz_spi_ids[] advertise SPI-ID (non-…
- [Low] Concern (validated as FALSE POSITIVE): the new KSZ8995XA branch
  of ksz8_reset_switch() writes PORT_PHY_SOFT_RESET to REG_PORT_STATUS_3…

Pre-existing issues:
- [Medium] Concern (validated as a real but pre-existing Medium defect,
  below the reporting threshold): ksz_init_global_dscp_map() in…
- [Low] Concern (validated as FALSE POSITIVE): the new KSZ8995XA special-
  tag programming (ksz_pwrite16(REG_PORT_CTRL_VID, 0) and…
- [Low] Concern (validated as FALSE POSITIVE): in
  ksz8_flush_dyn_mac_table() the new KSZ8995XA sequence sets…

--

net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

This adds KSZ8995XA (Micrel KS8995XA) support to the KSZ DSA driver, with a
restricted set of dev/switch ops, per-port PHY reset polling, header special
tag setup and DCB helper changes for the chip's eight one-bit TOS priority
registers. It also moves the legacy "micrel,ks8995", "micrel,ksz8795" and
"micrel,ksz8864" compatibles over to the KSZ SPI driver and makes
NET_DSA_KS8995 and NET_DSA_MICROCHIP_KSZ_SPI mutually exclusive in Kconfig.

A few questions below.

[ ... ]

[Severity: Medium]

Does frame_size here need to account for the 4-byte KS8995 special tag as
well?

ksz8995xa_change_mtu() computes:

	frame_size = mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;

which allows room for exactly one VLAN header. But ks8995_xmit() in
net/dsa/tag_ks8995.c always inserts an additional outer tag:

	skb = vlan_insert_tag(skb, KS8995M_STPID(portmask), 0);

so a full-size 802.1Q frame injected by the CPU is 14 + 4 (special tag) +
4 (customer VLAN) + 1500 + 4 (FCS) = 1526 bytes on the wire at the CPU
port.

At the default MTU of 1500 the computation above yields exactly 1522,
which equals KSZ8995XA_LEGAL_PACKET_SIZE, so KSZ8863_LEGAL_PACKET_ENABLE
is set and the switch is told to enforce 1522.

DSA does not compensate either: in dsa_user_change_mtu() the CPU port gets
the bare user MTU, and only the conduit MTU gets the tag overhead added:

	cpu_mtu = largest_mtu;
	...
	err = dsa_port_mtu_change(cpu_dp, cpu_mtu);

Can this make the switch drop full-size VLAN-tagged frames coming from the
CPU?

The same allowance question applies to .port_max_mtu, since
ksz8995xa_switch_ops reuses ksz88xx_max_mtu(), which returns
KSZ8863_HUGE_PACKET_SIZE - VLAN_ETH_HLEN - ETH_FCS_LEN and therefore also
leaves no room for the special tag at the largest configurable MTU.

[ ... ]

[ ... ]

[Severity: Medium]

This is a pre-existing issue and not introduced by this patch, but since
this helper now also drives the new KSZ8995XA one-bit-per-DSCP register
layout, is the error return still being dropped by its only caller?

In ksz_init_global_dscp_map() the result is assigned and then discarded:

		ret = ksz_set_global_dscp_entry(dev, dscp, ipm);
	}

	return 0;
}

So a regmap failure while programming any of the 64 DSCP entries leaves the
table partially programmed while ksz8_setup() -> ksz_dcb_init() still
reports success. Would returning ret on failure be preferable?

[ ... ]

[ ... ]

[Severity: Low]

Should the bindings be updated for these three compatibles as well?

After this patch (and after the last patch of the series removes
drivers/net/dsa/ks8995.c), the KSZ driver is the only implementation of
"micrel,ks8995", "micrel,ksz8795" and "micrel,ksz8864", but the schema that
covers them is still
Documentation/devicetree/bindings/net/dsa/micrel,ks8995.yaml, which defines
only compatible, reg and reset-gpios plus the dsa.yaml and
spi-peripheral-props references, and ends with:

  unevaluatedProperties: false

The KSZ implementation, on the other hand, reads several more properties for
whatever it matches, in ksz_switch_register():

	dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
						 "microchip,synclko-125");
	dev->wakeup_source = of_property_read_bool(dev->dev->of_node,
						   "wakeup-source");

plus interrupts, an mdio child node, micrel,fiber-mode,
microchip,synclko-disable, microchip,pme-active-high and the three
microchip,*-drive-strength-microamp properties handled by
ksz8_parse_drive_strength(), which ksz8_setup() calls unconditionally.

Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml documents all of
those and already lists microchip,ksz8995xa, but it does not enumerate the
three micrel strings, and micrel,ks8995.yaml does not $ref it. So a device
tree using compatible = "micrel,ks8995" together with, say, interrupts or
microchip,synclko-125 is rejected by dt_binding_check even though the driver
honours it. The two in-tree device trees
(intel-ixp42x-actiontec-mi424wr.dtsi and intel-ixp42x-linksys-wrv54g.dts)
only use compatible/reg/spi-max-frequency/ethernet-ports, so nothing in tree
fails validation today.

Verified at the end of the series as well: micrel,ks8995.yaml is unchanged
and microchip,ksz.yaml's compatible enum still lacks the micrel strings.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916-ks8995-to-ksz8-v15-0-8825ff5fd4be%40kernel.org