Re: [PATCH net-next v5 7/9] net: dsa: realtek: rtl8365mb: add family D SDS13 PCS support

[netdev-bot+sashiko@kernel.org]

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

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

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] Consider family D port 6 in SGMII with managed = "in-band-
  status" and no PHY.

--

net: dsa: realtek: rtl8365mb: add family D SDS13 PCS support

This extends the rtl8365mb SGMII/HSGMII PCS code to family D switches. On
those switches the CPU SerDes is reached through the indexed SDS13 window,
and the SDS_MISC layout differs from family C. For family D,
pcs_link_up() becomes a no-op and the link is forced only from
mac_link_up().

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 9ba31dfb161c8..6c51f4805b595 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -1636,6 +1740,17 @@ static void rtl8365mb_pcs_get_state(struct phylink_pcs *pcs,
>  	if (!state->link)
>  		return;
>  
> +	if (is_d) {
> +		/* Only 2500base-x is a fixed rate; for the other modes keep
> +		 * the speed and duplex phylink passed in.
> +		 */
> +		if (state->interface == PHY_INTERFACE_MODE_2500BASEX) {
> +			state->duplex = DUPLEX_FULL;
> +			state->speed = SPEED_2500;
> +		}
> +		return;
> +	}
> +

[Severity: Medium]
On family D in SGMII mode with no PHY, can phylink pass SPEED_UNKNOWN to
mac_link_up() here?

Take port 6 with phy-mode = "sgmii", managed = "in-band-status" and no
phy-handle. rtl8365mb_pcs_inband_caps() only reports LINK_INBAND_DISABLE.
That makes phylink_pcs_neg_mode() pick PHYLINK_PCS_NEG_OUTBAND, and since
there is no phydev the mode stays MLO_AN_INBAND.

phylink_resolve() then gets the link state through
phylink_mac_pcs_get_state(), which seeds speed and duplex like this:

drivers/net/phy/phylink.c:phylink_mac_pcs_get_state() {
    ...
	} else {
		state->speed =  pl->link_config.speed;
		state->duplex = pl->link_config.duplex;
    ...
}

phylink_create() sets link_config.speed/duplex to SPEED_UNKNOWN and
DUPLEX_UNKNOWN, and nothing updates them on this path.

When the SerDes reports link up, the family D branch above only overrides
speed and duplex for 2500BASEX. For SGMII it returns with SPEED_UNKNOWN
still set. The call chain would then be:

phylink_link_up()
  rtl8365mb_phylink_mac_link_up()
    rtl8365mb_ext_config_forcemode()
      dev_err(..., "unsupported port speed %s\n", ...)
      return -EINVAL

mac_link_up() returns void, so phylink still marks the link up and turns
the carrier on. However, the family D MAC force register is never
programmed, so wouldn't the port show link up and pass no traffic?

Family C avoids this because its pcs_get_state() reads speed and duplex
back from SDS_MISC. This also seems to still be present at the end of the
series.

Could the family D branch report a concrete speed and duplex for SGMII as
well, or otherwise avoid returning link up with an unknown speed?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/aa5fa12c-e43e-4e5e-9f77-c6eaf477abc4%40yahoo.com