Re: [PATCH net-next v7 6/6] net: dsa: motorcomm: Add support for Motorcomm YT922x

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

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

- [Critical] YT922x PCS callbacks recover their private structure from an
  uninitialized `pp->index`.
- [High] Four of the six YT922x serdes-mode constants are encoded into
  the wrong register field.
- [High] `yt922x_sds_phyaddr_get()` (chip.c:4901-4918) accepts an `enum
  yt922x_phy_reg_space` argument and its own comment says "sds phyaddr…
- [High] `yt922x_dsa_setup()` (chip.c:5507) takes `priv->reg_lock` at
  chip.c:5530 and calls `yt922x_chip_setup()` (chip.c:5463) with the…
- [Medium] The driver advertises `PHY_INTERFACE_MODE_100BASEX` for YT922x
  external ports (in `yt922x_dsa_phylink_get_caps()` at chip.c:5317 and…
- [Medium] `yt922x_pcs_config()` (chip.c:5217) ignores `neg_mode`,
  `advertising` and `permit_pause_to_mac`.
- [Medium] The driver adds the OF compatible "motorcomm,yt9224" to
  `yt921x_of_match[]` (chip.c:5695) but no device-tree binding documents…
- [Low] The commit message states the YT922X series "includes YT9224 and
  YT9228", but only YT9224 is implemented: a single `yt921x_infos` entry…
- [Low] `NET_DSA_YT921X` keeps the prompt "Motorcomm YT9215 ethernet
  switch chip support" and YT9215-only help text while the same patch…
- [Low] `yt922x_chip_setup_dsa()` writes a hardcoded `GENMASK(9, 0)` (10
  port bits) to `YT922X_FILTER_UNK_UCAST` at chip.c:5440, while the two…
- [Low] `yt922x_pcs_config()` commits the new serdes mode to
  `YT922X_PORT_SDSn` (chip.c:~5262) and then runs the ~40-write…

--

net: dsa: motorcomm: Add support for Motorcomm YT922x

This adds a YT922x variant to the Motorcomm DSA driver: a new yt92xx_series
entry with its own dsa_switch_ops and phylink_mac_ops, a YT9224 chip info
entry, a large block of YT922x register definitions, and a per-port embedded
phylink PCS for the serdes ports. It also registers the motorcomm,yt9224 OF
compatible and selects the YT922x tagger.

A few questions on the commit message and the new code below.

    Add support for Motorcomm YT922X, which is series of
    ethernet switches developed by Motorcomm Electronic
    Technology, includes YT9224 and YT9228.
    
    This patch adds basic support for a working DSA switch.

[Severity: Low]
Is YT9228 actually supported here?  Only YT9224 appears in the code: one
yt921x_infos entry for YT9224_MAJOR (0x9004), one motorcomm,yt9224
compatible, and yt92xx_series_lookup() matching only YT9224_MAJOR.  A grep
of the tree finds no YT9228 identifier, so a YT9228 would be rejected in
yt921x_chip_detect() with "Unexpected chipid" or by
yt92xx_series_lookup() returning NULL.

Could the second sentence also state what "basic" covers?
yt922x_dsa_switch_ops provides only get_tag_protocol, phylink_get_caps,
port_setup and setup, and yt922x_port_setup() sets YT922X_PORT_LEARN_DIS on
every port, so bridging, VLAN, FDB and statistics are not available.

[Severity: Low]
Should the prompt and help text mention YT922x too?  The same patch makes
this symbol bind motorcomm,yt9224, select NET_DSA_TAG_YT922X and set
MODULE_DESCRIPTION("Driver for Motorcomm YT921x and YT922x Switch"), but
the only user-visible description still says YT9215 only.

[ ... ]

[Severity: Critical]
yt921x_port_to_priv() recovers priv by subtracting
offsetof(struct yt921x_priv, ports[pp->index]), so it is only correct when
pp->index equals the slot the yt921x_port actually occupies.

Where is pp->index assigned on the YT922x path?  The only assignment in the
driver is in yt921x_dsa_setup():

	pp->index = i;
	INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);

yt922x_dsa_setup() has no equivalent loop, and priv is zero-allocated, so
every priv->ports[i].index stays 0 on YT922x.

yt922x_pcs_setup() then installs pp->pcs.ops for each bit of
info->external_mask, which for YT9224 is BIT(0) | BIT(8), and
yt922x_phylink_mac_select_pcs() returns &priv->ports[dp->index].pcs.  So
phylink calls the PCS ops with pp == &priv->ports[8] while pp->index == 0,
and yt921x_port_to_priv() yields (char *)priv + 8 * sizeof(struct
yt921x_port) instead of priv.

Does that mean yt922x_pcs_get_state() runs
mutex_lock(&priv->reg_lock) on bytes that are really part of
priv->ports[] (MIB counters and a delayed_work), and then calls
priv->reg_ops->read() through a function pointer loaded from that same
memory?

A second effect of the uninitialised index: int port = pp->index is 0 for
port 8 in all three PCS callbacks, so the

	if (port != 0 && port != 8)
		return -EINVAL;

check in yt922x_pcs_config() passes with the wrong port and port 0's serdes
registers are programmed.

[ ... ]

[ ... ]

+static int yt922x_sds_phyaddr_get(int port,
+				  enum yt922x_phy_reg_type reg_type,
+				  enum yt922x_phy_reg_space reg_space)
+{
+	int res = port;
+
+	/*
+	 * sds phyaddr mapping depend on reg_type and reg_space
+	 */
+	if (!yt922x_port_is_internal_sds(port))
+		return -EOPNOTSUPP;
+	if (reg_type == YT922X_PHY_REG_TYPE_COMMON_EXT) {
+		res = YT922X_COMMON_EXT_PHYADDR;
+		return res;
+	}
+
+	return res;
+}

[Severity: High]
The comment says the mapping depends on reg_type and reg_space, but
reg_space is never read: for every reg_type other than
YT922X_PHY_REG_TYPE_COMMON_EXT the function returns port.  Do the SGMII and
USXGMII register spaces then alias to the same internal MDIO address?

The closing soft reset in yt922x_sds_init() looks like it becomes
self-cancelling:

	addr = yt922x_sds_phyaddr_get(port, YT922X_PHY_REG_TYPE_MII,
				      YT922X_PHY_REG_SPACE_SGMII);
	...
	data &= ~(1 << 15);
	res = yt921x_intif_write(priv, addr, 0x0, data);
	...
	addr = yt922x_sds_phyaddr_get(port, YT922X_PHY_REG_TYPE_MII,
				      YT922X_PHY_REG_SPACE_USXGMII);
	...
	data |= 1 << 15;
	res = yt921x_intif_write(priv, addr, 0x0, data);

Both lookups return the same addr, so bit 15 is cleared and then set on one
register and the USXGMII PCS is never touched.  The same aliasing makes
yt922x_pcs_get_state() read YT922X_PCS_LINK_CTRL/MII_LPA from the SGMII
address in the USXGMII case, and makes yt922x_pcs_an_restart() perform the
identical BMCR read-modify-write twice.

Two smaller points in the same area:

	if (addr < 0)
		return res;

res is 0 at both of these sites in yt922x_sds_init(), so a failed address
lookup reports success while silently skipping the soft reset.  Should
these return addr?

And yt922x_pcs_get_state() and yt922x_pcs_an_restart() pass addr straight
to yt921x_intif_read() without checking for the -EOPNOTSUPP that
yt922x_sds_phyaddr_get() returns for non-serdes ports.

[ ... ]

+static void yt922x_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
+				 struct phylink_link_state *state)
+{
+	struct yt921x_port *pp = pcs_to_yt921x_port(pcs);
+	struct yt921x_priv *priv = yt921x_port_to_priv(pp);
+	int port = pp->index;
+	int res = 0;
+	u16 data;
+	int addr;
+	u16 lp;
+
+	mutex_lock(&priv->reg_lock);
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_100BASEX:
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		addr = yt922x_sds_phyaddr_get
+			(port, YT922X_PHY_REG_TYPE_SDS_COMMON_EXT,
+			 YT922X_PHY_REG_SPACE_SGMII);
+		res = yt921x_intif_read(priv, addr, MII_BMSR, &data);
+		if (res)
+			goto err;
+		res = yt921x_intif_read(priv, addr, MII_LPA, &lp);
+		if (res)
+			goto err;
+		phylink_mii_c22_pcs_decode_state(state, neg_mode, data, lp);
+		break;

[Severity: Medium]
Can a 100BASE-X link ever be reported up through this path?
phylink_mii_c22_pcs_decode_state() in drivers/net/phy/phylink.c has cases
only for 1000BASE-X, 2500BASE-X, SGMII/PSGMII/QSGMII and QUSGMII:

	default:
		state->link = false;
		break;

100BASE-X is advertised as supported by yt922x_dsa_phylink_get_caps() and
yt922x_pcs_setup(), and yt922x_phylink_mac_select_pcs() returns this PCS
for it, so the mode looks reachable but always decodes as link down.

[ ... ]

+static int yt922x_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
+			     phy_interface_t interface,
+			     const unsigned long *advertising,
+			     bool permit_pause_to_mac)
+{
+	struct yt921x_port *pp = pcs_to_yt921x_port(pcs);
+	struct yt921x_priv *priv = yt921x_port_to_priv(pp);
+	int res, port;
+	u16 data;
+	u16 ctrl;
+	int addr;

[Severity: Medium]
neg_mode, advertising and permit_pause_to_mac appear only in the parameter
list; the body only rewrites the serdes mode field and runs
yt922x_sds_init().  Should this callback enable or disable in-band
negotiation according to neg_mode and program the local advertisement for
the Base-X modes?

As written, yt922x_pcs_get_state() hands neg_mode to
phylink_mii_c22_pcs_decode_state(), so the read path assumes a negotiation
setup that the write path never applied.

+	port = pp->index;
+	if (port != 0 && port != 8)
+		return -EINVAL;
+
+	mutex_lock(&priv->reg_lock);
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+		ctrl = YT92XX_SERDES_MODE_SGMII;
+		break;
+	case PHY_INTERFACE_MODE_100BASEX:
+		ctrl = YT92XX_SERDES_MODE_100BASEX;
+		break;
+	case PHY_INTERFACE_MODE_1000BASEX:
+		ctrl = YT92XX_SERDES_MODE_1000BASEX;
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		ctrl = YT92XX_SERDES_MODE_2500BASEX;
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		ctrl = YT92XX_SERDES_MODE_USXGMII;
+		break;
+	default:
+		res = -EINVAL;
+		goto err;
+	}
+	addr = yt922x_sds_phyaddr_get
+		(port, YT922X_PHY_REG_TYPE_SDS_COMMON_EXT,
+		 YT922X_PHY_REG_SPACE_SGMII);
+	if (addr < 0) {
+		res = addr;
+		goto err;
+	}
+
+	res = yt921x_intif_ext_read(priv, addr, YT922X_PORT_SDSn, &data);
+	if (res)
+		goto err;
+	data &= ~YT922X_SERDES_MODE_M;
+	data |= ctrl;
+	res = yt921x_intif_ext_write(priv, addr, YT922X_PORT_SDSn, data);
+	if (res)
+		goto err;
+	/* SERDES init and interface configuration */
+	res = yt922x_sds_init(priv, port, interface);
+	if (res)
+		goto err;
+	mutex_unlock(&priv->reg_lock);

[Severity: Low]
The new mode is committed to YT922X_PORT_SDSn before the roughly forty
writes of yt922x_sds_init() run.  If one of those indirect MDIO
transactions fails, the error path only unlocks and returns, leaving the
new mode selected with a mix of old and new CDR/PLL/VCO/eye/CTLE settings
and no closing PCS reset.  Is a rollback wanted here, or is the intent to
rely on the next successful pcs_config?  The YT921x paths such as
yt921x_port_up() have the same shape today.

[ ... ]

+static int yt922x_chip_setup_dsa(struct yt921x_priv *priv)
+{
+	unsigned long cpu_ports_mask;
+	u32 ctrl;
+	int port;
+	int res;
+
+	/* cpu port set */
+	mutex_lock(&priv->reg_lock);
+	res = yt922x_cpu_port_set(priv);
+	mutex_unlock(&priv->reg_lock);
+	if (res)
+		return res;

[Severity: High]
Can this deadlock the probing thread?  yt922x_dsa_setup() already holds
priv->reg_lock across the call:

	mutex_lock(&priv->reg_lock);
	res = yt922x_chip_setup(priv);
	mutex_unlock(&priv->reg_lock);

and yt922x_chip_setup() calls yt922x_chip_setup_dsa() directly, so this
mutex_lock() re-acquires a mutex the same task already owns.  struct mutex
is not recursive, so this looks like it hangs on the first YT9224 probe
inside dsa_register_switch(), with lockdep reporting possible recursive
locking.

The register accesses further down in this function need the outer lock
anyway (yt921x_reg_read()/yt921x_reg_write() contain
lockdep_assert_held_once(&priv->reg_lock)), so removing this inner
lock/unlock pair rather than moving the outer one seems to be what is
wanted.

+
+	ctrl = GENMASK(9, 0);
+	res = yt921x_reg_write(priv, YT922X_FILTER_UNK_UCAST, ctrl);
+	if (res)
+		return res;
+
+	ctrl = 0;
+	for (int i = 0; i < priv->series->max_ports; i++)
+		ctrl |= YT922X_ACT_UNK_ACTn_TRAP(i);

[Severity: Low]
The loops here size their masks with priv->series->max_ports, which is
YT922X_PORT_NUM == 9, but GENMASK(9, 0) covers ten port bits.  The YT921x
equivalent writes GENMASK(10, 0) with YT921X_PORT_NUM == 11, i.e. exactly
max_ports bits.  Is bit 9 intentional here, or should this be derived from
YT922X_PORT_NUM?

[ ... ]

+static int yt922x_dsa_setup(struct dsa_switch *ds)
+{
+	struct yt921x_priv *priv = to_yt921x_priv(ds);
+	struct device *dev = to_device(priv);
+	struct device_node *np = dev->of_node;
+	struct device_node *child;
+	int res;
+
+	mutex_lock(&priv->reg_lock);
+	res = yt921x_chip_reset(priv);
+	mutex_unlock(&priv->reg_lock);
+	if (res)
+		return res;
+
+	/* Register the internal mdio bus. */
+	child = of_get_child_by_name(np, "mdio");
+	if (child) {
+		res = yt921x_mbus_int_init(priv, child);
+		of_node_put(child);
+		if (res)
+			return res;
+	}
+
+	mutex_lock(&priv->reg_lock);
+	res = yt922x_chip_setup(priv);
+	mutex_unlock(&priv->reg_lock);
+	if (res)
+		return res;
+
+	/* switch sds pcs setup */
+	yt922x_pcs_setup(ds);
+
+	return 0;
+}

[ ... ]

[Severity: Medium]
Is a binding update missing for this compatible?
Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml still has:

  compatible:
    const: motorcomm,yt9215

so a DTS using motorcomm,yt9224 fails dtbs_check with "'motorcomm,yt9224'
is not one of ['motorcomm,yt9215']".

The same binding also describes the port map as internal GbE PHYs on Port
0-7 with the GMACs on Port 8-9, which does not match the new YT9224 entry
(internal_mask GENMASK(7, 4), external_mask BIT(0) | BIT(8),
YT922X_PORT_NUM 9).  Should the mdio/port description be extended for
YT922x as well?

[ ... ]

[ ... ]

+#define YT922X_PORT_SDSn	0x400
+#define  YT922X_SERDES_MODE_M		GENMASK(6, 4)
+#define   YT922X_SERDES_MODE(x)			FIELD_PREP(YT922X_SERDES_MODE_M, (x))
+#define YT92XX_SERDES_MODE_SGMII	YT922X_SERDES_MODE(0)
+#define YT92XX_SERDES_MODE_REVSGMII	YT921X_SERDES_MODE(1)
+#define YT92XX_SERDES_MODE_1000BASEX	YT921X_SERDES_MODE(2)
+#define YT92XX_SERDES_MODE_100BASEX	YT921X_SERDES_MODE(3)
+#define YT92XX_SERDES_MODE_2500BASEX	YT921X_SERDES_MODE(4)
+#define YT92XX_SERDES_MODE_USXGMII	YT922X_SERDES_MODE(6)

[Severity: High]
Should the four middle constants use YT922X_SERDES_MODE() rather than
YT921X_SERDES_MODE()?  YT921X_SERDES_MODE(x) is
FIELD_PREP(GENMASK(9, 7), x), while the YT922x field is GENMASK(6, 4).

yt922x_pcs_config() does:

	data &= ~YT922X_SERDES_MODE_M;
	data |= ctrl;

so for 100BASE-X, 1000BASE-X and 2500BASE-X the mode field in bits 6:4 is
cleared to 0, which is SGMII, and the value ends up in bits 9:7 of
YT922X_PORT_SDSn instead.  Only SGMII (value 0) and USXGMII use the YT922x
constructor.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914103009.3007019-1-kyle.switch%40motor-comm.com