Re: [net-next,v3] net: dsa: microchip: enable the SGMII port of the KSZ9897S

[netdev-bot+sashiko]

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

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

- [Medium] The new `[KSZ9897S]` chip_data entry in
  drivers/net/dsa/microchip/ksz_common.c (lines 1489-1522) declares…
- [Low] The patch carries no Fixes: tag even though its commit message
  describes a user-visible functional gap ("its chip_info entry never…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patch
  newly routes KSZ9897S parts through it.

--

net: dsa: microchip: enable the SGMII port of the KSZ9897S

This adds a separate KSZ9897S chip_info entry that is the KSZ9897 one plus
.sgmii_port = 7, detects the variant at run time from the read-only bit 7 of
the port 7 XMII control 0 register in ksz_switch_detect(), and lets
ksz_check_device_id() accept a detected KSZ9897S for a device tree naming the
KSZ9897.

Link: https://lore.kernel.org/r/20260811-ksz9897-sgmii-port-v2-1-04ae287a0b27@xxxxxxxxxxx
Link: https://lore.kernel.org/r/20260703-ksz9897-sgmii-port-v1-1-387e7632dd0b@xxxxxxxxxxx

One comment on the commit message before the diff:

> The KSZ9897 has an SGMII/1000BASE-X capable port 7, but its chip_info
> entry never set .sgmii_port. ksz_has_sgmii_port() is therefore false,
> ksz9477_pcs_create() is never called and ksz9477_phylink_mac_select_pcs()
> returns NULL for port 7, so a phy-mode = "sgmii" or "1000base-x" link on
> that port cannot come up.

[Severity: Low]
This isn't a bug, but the description above reads like a user-visible
functional gap while the patch carries no Fixes: tag.  Was that
deliberate?

For what it is worth, it looks correct to leave it out: this is hardware
enablement (a new enum ksz_model value, a new KSZ9897S_CHIP_ID in
include/linux/platform_data/microchip-ksz.h, a new chip_data entry and new
run-time variant detection) aimed at net-next, and the direct precedent,
commit e8c35bfce4c13 ("net: dsa: microchip: Add SGMII port support to
KSZ9477 switch"), which introduced .sgmii_port and enabled the previously
non-working SGMII port on the KSZ9477 and LAN9646, has no Fixes: tag
either.

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 1a9d6f83a023..78442cc37643 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1486,6 +1486,41 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.gbit_capable	= {true, true, true, true, true, true, true},
>  	},
>  
> +	[KSZ9897S] = {
> +		.chip_id = KSZ9897S_CHIP_ID,
> +		.dev_name = "KSZ9897S",

[ ... ]

> +		.supports_mii	= {false, false, false, false,
> +				   false, true, true},
> +		.supports_rmii	= {false, false, false, false,
> +				   false, true, true},
> +		.supports_rgmii = {false, false, false, false,
> +				   false, true, true},

[Severity: Medium]
Should index 6 of these three arrays be false in the KSZ9897S entry?

Index 6 is physical port 7, which this same entry declares as the SGMII
port, and the commit message says "Only the KSZ9897S has that port; the
KSZ9897R has a second RGMII port instead".  The [KSZ9477] entry, the other
7-port part with SGMII on port 7, sets index 6 of all three arrays to
false:

	drivers/net/dsa/microchip/ksz_common.c:
	[KSZ9477] = {
		...
		.supports_rgmii = {false, false, false, false,
				   false, true, false},
		...
		.sgmii_port = 7,

What happens if a device tree names microchip,ksz9897 (which the new bypass
in ksz_check_device_id() now accepts for a detected KSZ9897S) and declares
phy-mode = "rgmii-id", "mii" or "rmii" on port 7?

ksz_phylink_get_caps() turns the flags above into advertised interface
modes, so phylink accepts the mode:

	drivers/net/dsa/microchip/ksz_common.c:ksz_phylink_get_caps() {
		...
		if (dev->info->supports_rgmii[port])
			phy_interface_set_rgmii(config->supported_interfaces);
		...
	}

But because .sgmii_port = 7 makes ksz_is_sgmii_port() true for that port,
the xMII control registers are then never programmed:

	drivers/net/dsa/microchip/ksz_common.c:ksz_phylink_need_config() {
		/* No need to configure XMII control register when using SGMII. */
		if (ksz_is_sgmii_port(dev, port))
			return false;
		...
	}

and no PCS is attached either:

	drivers/net/dsa/microchip/ksz9477.c:ksz9477_phylink_mac_select_pcs() {
		if (ksz_is_sgmii_port(dev, dp->index) &&
		    (interface == PHY_INTERFACE_MODE_SGMII ||
		    interface == PHY_INTERFACE_MODE_1000BASEX))
			return p->pcs;

		return NULL;
	}

Does that leave port 7 with neither xMII programming nor a PCS, and without
any error or warning, instead of rejecting the mode?

Note the in-tree [LAN9646] entry carries the same combination since commit
e8c35bfce4c13, so there is precedent for it, but it disagrees with the
vendor-authored [KSZ9477] entry and with this commit message.

> +		.internal_phy	= {true, true, true, true,
> +				   true, false, false},
> +		.gbit_capable	= {true, true, true, true, true, true, true},
> +		.sgmii_port = 7,

[Severity: Medium]
This isn't a bug introduced by this patch, but setting .sgmii_port here
makes ksz_has_sgmii_port() true and newly routes KSZ9897S parts through a
PCS lifecycle that has no release path.

ksz9477_pcs_create() allocates the dw_xpcs and stores it in p->pcs:

	drivers/net/dsa/microchip/ksz9477.c:ksz9477_pcs_create() {
		...
		pcs = xpcs_create_pcs_mdiodev(bus, 0);
		if (IS_ERR(pcs))
			return PTR_ERR(pcs);
		p->pcs = pcs;
		...
	}

The counterpart, which does xpcs_clear_clks() plus xpcs_free_data() and
drops the mdio_device reference taken by xpcs_create(), is never called from
drivers/net/dsa/microchip/:

	drivers/net/pcs/pcs-xpcs.c:
	void xpcs_destroy_pcs(struct phylink_pcs *pcs)
	{
		xpcs_destroy(phylink_pcs_to_xpcs(pcs));
	}

Per-port teardown only frees the ACL, and p->pcs is left set:

	drivers/net/dsa/microchip/ksz9477.c:
	static void ksz9477_port_teardown(struct dsa_switch *ds, int port)
	{
		struct ksz_device *dev = ds->priv;

		if (dsa_is_user_port(ds, port))
			ksz9477_port_acl_free(dev, port);
	}

ksz_teardown() has no xpcs_destroy_pcs() call either.  Does every
unbind/rebind cycle, or a DSA setup failure after ksz9477_setup() created
the PCS, then leak the dw_xpcs and its mdio_device reference while the
devm-managed mdiobus it points at goes away?  The omission dates back to
commit e8c35bfce4c13, which first set .sgmii_port for the KSZ9477 and
LAN9646.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-ksz9897-sgmii-port-v3-1-2376aa4edf11%40vaisala.com