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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The driver's YT922x port map contradicts itself, so the
  YT9224's second SerDes port (port 9) cannot be used.
- [Medium] The patch adds compatible "motorcomm,yt9224" to
  yt921x_of_match[], but the DT binding is not updated.
- [Medium] The 5 Gb/s capability that YT922x SerDes ports advertise does
  not work: the MAC is never brought up at 5G.
- [Medium] yt922x_pcs_config() (pcs-922x.c:110-161) does not meet the
  phylink pcs_config contract.
- [Low] yt92xx_register_switch() declares `u32 chipid;` with no
  initializer.
- [Low] yt921x_dsa_teardown() does not compile with
  CONFIG_NET_DSA_YT921X_LEDS=n.

--

net: dsa: motorcomm: Add support for Motorcomm YT922x

This patch adds basic DSA support for the Motorcomm YT9224 switch to the
existing yt921x driver. It adds a YT922x series descriptor with its own
dsa_switch_ops and phylink_mac_ops, a new SerDes PCS implementation in
pcs-922x.c, and a "motorcomm,yt9224" compatible.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 9380d74d79ba2..175855cbc2375 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -141,6 +141,18 @@ static const struct yt921x_info yt921x_infos[] = {
>  		BIT(8) | BIT(9),
>  		BIT(8) | BIT(9),
>  	},
> +	{
> +		"YT9224", YT9224_MAJOR, 0, 0,
> +		GENMASK_U16(7, 4),
> +		0,
> +		BIT(8) | BIT(9),
> +	},

[Severity: Medium]
This entry marks port 9 as a SerDes port on YT9224. The YT922x series,
however, uses max_ports = YT922X_PORT_NUM, which is 9, and
yt92xx_register_switch() copies that into ds->num_ports. Can port 9
actually be used?

With ds->num_ports == 9, dsa_switch_parse_ports_of() rejects a DT node
for ethernet-port@9:

net/dsa/dsa.c:dsa_switch_parse_ports_of() {
    ...
	if (reg >= ds->num_ports) {
    ...
}

A board that wires the second SerDes (for example as the CPU port)
would then fail to probe with -EINVAL.

The rest of the new code also seems to assume 9 ports:

  yt922x_pcs_setup() loops with for_each_set_bit(port, &mask,
  priv->series->max_ports), so the PCS for port 9 is never set up.

  yt922x_chip_setup_dsa() writes GENMASK_U32(8, 0) to
  YT922X_FILTER_UNK_UCAST. It also builds the unknown ucast/mcast
  action word only for ports 0..8.

  YT922X_COMMON_EXT_PHYADDR is 9, and yt922x_sds_phyaddr_get() uses the
  port index as the SerDes MII address. A port 9 PCS would therefore
  use the same address as the common ext block.

The new tag_yt922x.c tagger, on the other hand, encodes destination
ports 1..9 in tag[1] bits 8:0 and port 0 in tag[2] BIT(15). That
describes a 10 port device.

Is either serdes_mask here or YT922X_PORT_NUM wrong?

[ ... ]

> @@ -3932,6 +3944,11 @@ static void yt921x_dsa_teardown(struct dsa_switch *ds)
>  	}
>  }
>  
> +static bool yt921x_needs_extmode_check(u32 major)
> +{
> +	return (major == YT9224_MAJOR) ? false : true;
> +}
> +

[Severity: Low]
This comes from an earlier patch in the series rather than this one.
Does yt921x_dsa_teardown(), just above, still build with
CONFIG_NET_DSA_YT921X_LEDS=n?

drivers/net/dsa/motorcomm/chip.c:yt921x_dsa_teardown() {
#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
	struct yt921x_priv *priv = to_yt921x_priv(ds);

	yt921x_leds_remove(priv);
#endif

	for (size_t i = ARRAY_SIZE(priv->ports); i-- > 0; ) {
    ...
}

priv is declared only inside the #if block, but the loop after the
#endif uses it. NET_DSA_YT921X_LEDS is a user visible bool that depends
on LEDS_CLASS=y || LEDS_CLASS=NET_DSA_YT921X. With LEDS_CLASS=n, or
with LEDS_CLASS=m and NET_DSA_YT921X=y, the build would fail because
priv is undeclared.

The breakage was introduced by "net: dsa: motorcomm: move mib stop from
remove() to teardown()" earlier in the series. It is still there after
this patch, which also edits the driver's Kconfig entry.

[ ... ]

> @@ -4411,6 +4439,412 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = {

[ ... ]

> +static int
> +yt922x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
> +	       phy_interface_t interface, int speed, int duplex,
> +	       bool tx_pause, bool rx_pause)
> +{
> +	int ps = ethtool_speed_to_yt921x(speed);
> +	u32 ctrl = 0;
> +	u32 mask;
> +	int res;
> +
> +	if (ps == YT921X_SPEED_INVALID)
> +		return -EINVAL;

[Severity: Medium]
What happens here when phylink resolves SPEED_5000?

yt922x_dsa_phylink_get_caps() advertises USXGMII and MAC_5000FD for
every SerDes port. However, ethtool_speed_to_yt921x() has no
SPEED_5000 case:

drivers/net/dsa/motorcomm/chip.h:ethtool_speed_to_yt921x() {
	...
	case SPEED_2500:
		return YT921X_SPEED_2500;
	case SPEED_10000:
		return YT921X_SPEED_10000;
	default:
		return YT921X_SPEED_INVALID;
	}
}

YT921X_SPEED_INVALID is 5. That is the same value as the new
YT922X_PORT_SPEED_5000, which is defined but never used.

So with a 5G USXGMII link (for example yt922x_pcs_get_state() ->
phylink_decode_usxgmii_word() with MDIO_USXGMII_5000), yt922x_port_up()
returns -EINVAL before it writes YT922X_PORTn_CTRL, and en_phy is never
re-enabled.

yt922x_phylink_mac_link_up() only logs the error, and phylink_link_up()
still calls netif_carrier_on() afterwards. Would the port then report
carrier up while passing no traffic?

[ ... ]

> @@ -4423,12 +4857,25 @@ static const struct yt92xx_series yt92xx_series_table[] = {

[ ... ]

>  static const struct yt92xx_series *yt92xx_series_lookup(u32 major)
>  {
>  	if (major == YT9215_MAJOR || major == YT9218_MAJOR)
>  		return &yt92xx_series_table[YT92XX_MODE_YT921X];
> +	else if (major == YT9224_MAJOR)
> +		return &yt92xx_series_table[YT92XX_MODE_YT922X];
>  	else
>  		return NULL;
>  }

[Severity: Low]
This patch relies on yt92xx_register_switch(), the caller of this
lookup, to pick the YT922x series. Can its error path print an
uninitialized chipid?

drivers/net/dsa/motorcomm/chip.c:yt92xx_register_switch() {
    ...
	u32 chipid;
    ...
	res = yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid);
	mutex_unlock(&priv->reg_lock);
	if (res) {
		dev_err(dev, "Unexpected chipid 0x%x\n", chipid);
		return res;
	}
    ...
}

The read can fail before *valp is written. For example,
yt921x_reg_mdio_read() bails out after an MDIO write error. In that case
chipid is still uninitialized stack memory when it is printed. The
message also says "Unexpected chipid" when the read itself failed.

This was introduced earlier in the series by "net: dsa: motorcomm:
initialize dsa_switch based on chipid".

[ ... ]

> @@ -4547,6 +4994,7 @@ static int yt921x_mdio_probe(struct mdio_device *mdiodev)
>  
>  static const struct of_device_id yt921x_of_match[] = {
>  	{ .compatible = "motorcomm,yt9215" },
> +	{ .compatible = "motorcomm,yt9224" },
>  	{}
>  };

[Severity: Medium]
Should the DT binding be updated along with this new compatible?

Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml still
has:

  compatible:
    const: motorcomm,yt9215

together with unevaluatedProperties: false. No binding under
Documentation/devicetree/bindings mentions yt9224.

The binding's description, port list and mdio text also cover only the
YT921x parts. The YT9224 layout is not documented: internal PHYs on
ports 4-7, SerDes on ports 8 and 9, and no xMII.

A DTS that uses "motorcomm,yt9224" would fail dt_binding_check, and
checkpatch would flag the compatible as undocumented.

[ ... ]

> diff --git a/drivers/net/dsa/motorcomm/pcs-922x.c b/drivers/net/dsa/motorcomm/pcs-922x.c
> new file mode 100644
> index 0000000000000..98d403d01ba3c
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/pcs-922x.c
> @@ -0,0 +1,173 @@

[ ... ]

> +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)
> +{

[ ... ]

> +	/* write protect */
> +	res = yt921x_intif_ext_write(priv, addr, 0x4be, 0xd);
> +	if (res)
> +		goto err;
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +		ctrl = YT922X_SERDES_MODE_SGMII;
> +		break;

[ ... ]

> +	default:
> +		res = -EINVAL;
> +		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;
> +	mutex_unlock(&priv->reg_lock);
> +
> +	return res;

[ ... ]

> +const struct phylink_pcs_ops yt922x_phylink_pcs_ops = {
> +	.pcs_get_state = yt922x_pcs_get_state,
> +	.pcs_config = yt922x_pcs_config,
> +	.pcs_an_restart = yt922x_pcs_an_restart,
> +};

[Severity: Medium]
Does yt922x_pcs_config() need to handle neg_mode and advertising?

It only rewrites the YT922X_SERDES_MODE field. neg_mode, advertising
and permit_pause_to_mac are unused. BMCR_ANENABLE is never set or
cleared, and MII_ADVERTISE is never written for 1000BASE-X or
2500BASE-X.

The pcs_config kernel-doc in include/linux/phylink.h says:

 * When operating in %MLO_AN_INBAND, inband should always be enabled,
 * otherwise inband should be disabled.
 ...
 * For 1000BASE-X, the advertisement should be programmed into the PCS.

The sibling yt921x_serdes_config() in pcs-921x.c does this explicitly:

	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
		bmcr |= BMCR_ANENABLE;
	else
		bmcr &= ~BMCR_ANENABLE;

Without that, the PCS keeps whatever in-band AN state it had after
reset. In-band links may then fail to come up, and out-of-band or fixed
links may run with AN enabled.

yt922x_pcs_config() also returns 0 without programming the
advertisement. phylink_change_inband_advert() then never restarts AN,
so advertisement changes made through ethtool are silently dropped.

yt922x_phylink_pcs_ops has no pcs_inband_caps, pcs_link_up, pcs_enable
or pcs_disable. yt922x_port_up() only writes the MAC side
YT922X_PORTn_CTRL. Is anything forcing the SerDes speed for
out-of-band SGMII at 10/100M?

The 0x4be = 0xd write also happens before the interface is checked. For
an unsupported interface the function returns -EINVAL with that write
left in place, and the write is never undone. Should it be moved after
the switch on interface?

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