Re: [PATCH net-next v3 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] At this commit, every new family D branch is dead code.
- [Low] Several unconditional comments are wrong for family D after this
  patch: (a) rtl8365mb_pcs_config() line 1649 says "Keep SGMII in-band…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  rtl8365mb_pcs_ops has no .pcs_an_restart, and phylink calls that…

--

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

This patch extends the rtl8365mb SGMII/HSGMII PCS code to family D
switches, whose CPU SerDes is reached through the indexed SDS13 window.
It adds an index argument to the SDS indirect accessors, family D tuning
tables and SDS_MISC settings, and skips the family C link forcing in
pcs_link_up() and mac_link_up() on family D.

> Extend the existing RTL8367S SGMII/HSGMII PCS implementation in this
> driver for family D switches, whose CPU SerDes is reached through the
> indexed SDS13 window and whose SDS_MISC fields differ from family C.

[Severity: Low]
This isn't a bug, but at this commit every entry in rtl8365mb_chip_infos[]
is RTL8365MB_FAMILY_C. So rtl8365mb_get_family() never returns
RTL8365MB_FAMILY_D.

That makes all the new is_d branches unreachable for now:

  - rtl8365mb_pcs_config()
  - rtl8365mb_pcs_get_state()
  - rtl8365mb_pcs_link_up()
  - rtl8365mb_phylink_mac_link_up()
  - the family D early return in rtl8365mb_sds_probe_option()

They only become reachable with the later patch in the series, "net: dsa:
realtek: rtl8365mb: add support for RTL8367S-VB". That patch adds a
family D chip_info entry.

Could the commit message say that this patch is preparatory, and that the
code is enabled by the RTL8367S-VB patch?

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index c15b88ba7bb5c..0d3a2a385a204 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -1565,14 +1650,16 @@ static int rtl8365mb_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>  	/* Keep SGMII in-band autonegotiation disabled: the link parameters are
>  	 * forced from rtl8365mb_pcs_link_up() instead.
>  	 */
> -	ret = rtl8365mb_sds_read(priv, RTL8365MB_SDS_REG_NWAY, &val);
> +	ret = rtl8365mb_sds_read(priv, sds_index,
> +				 RTL8365MB_SDS_REG_NWAY, &val);
>  	if (ret)
>  		return ret;

[Severity: Low]
This isn't a bug, but does this comment still hold for family D? With this
patch, rtl8365mb_pcs_link_up() returns right away on family D.

Several other comments that apply to both families have the same problem
after this change:

In rtl8365mb_pcs_get_state():

    /* In-band autonegotiation is not implemented, so the link parameters are
     * forced from rtl8365mb_pcs_link_up(). ...

The family D branch there hard-codes DUPLEX_FULL and takes the speed from
state->interface instead.

In rtl8365mb_phylink_mac_link_up():

    /* The SerDes forced link state is programmed by the PCS in
     * rtl8365mb_pcs_link_up(); ...

This is not true on family D.

Above RTL8365MB_SDS_EXT_INTERFACE_ID:

    /* The embedded SerDes can only be muxed to external interface 1 (MAC8),
     * which is port 6.
     */

The new family D defines (RTL8365MB_D_SDS_EXT0_INDEX and
RTL8365MB_D_SDS_MISC_MAC6_SEL_SDS0) put the SerDes on EXT0/MAC6. The
RTL8367S-VB chip_info added at the end of the series also uses extint
id 0.

The family D path in rtl8365mb_pcs_config() still applies
RTL8365MB_SDS_BYPASS_LINE_RATE_MASK, which is built from
RTL8365MB_SDS_EXT_INTERFACE_PORT. However, the BYPASS_LINE_RATE comment
says the port-to-bit mapping "only holds for the RTL8367C-style parts
this driver supports".

None of these comments change by the end of the series. Could they be
updated to cover family D?

[ ... ]

> @@ -1660,6 +1762,12 @@ static void rtl8365mb_pcs_link_up(struct phylink_pcs *pcs,
>  	u32 r_speed;
>  	int ret;
>  
> +	/* Family D forces the external MAC ability from mac_link_up(); its
> +	 * SDS_MISC fields do not share the family C link-force layout.
> +	 */
> +	if (rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D)
> +		return;
> +

[Severity: High]
This is a pre-existing issue, but rtl8365mb_pcs_ops has no .pcs_an_restart
callback:

static const struct phylink_pcs_ops rtl8365mb_pcs_ops = {
	.pcs_inband_caps = rtl8365mb_pcs_inband_caps,
	.pcs_config = rtl8365mb_pcs_config,
	.pcs_get_state = rtl8365mb_pcs_get_state,
	.pcs_link_up = rtl8365mb_pcs_link_up,
};

phylink calls that callback without a NULL check:

drivers/net/phy/phylink.c:phylink_pcs_an_restart() {
	if (pl->pcs && linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
					 pl->link_config.advertising) &&
	    phy_interface_mode_is_8023z(pl->link_config.interface) &&
	    phylink_autoneg_inband(pl->act_link_an_mode))
		pl->pcs->ops->pcs_an_restart(pl->pcs);
}

Suppose the DT gives port 6 phy-mode = "2500base-x" and
managed = "in-band-status":

  - phylink_parse_mode() sets Autoneg in the advertising mask.
  - pcs_inband_caps reports LINK_INBAND_DISABLE, so phylink_pcs_neg_mode()
    sets pcs_neg_mode to PHYLINK_PCS_NEG_INBAND_DISABLED.
  - act_link_an_mode still stays MLO_AN_INBAND.

Can this then oops during port setup through this path?

dsa_port_enable_rt()
  phylink_start()
    phylink_mac_initial_config(pl, true)
      phylink_major_config(..., restart = true)
        phylink_pcs_config()
          rtl8365mb_pcs_config()
        phylink_pcs_an_restart()
          pl->pcs->ops->pcs_an_restart(pl->pcs)   <-- NULL

rtl8365mb_phylink_mac_config() only logs an error for in-band mode.
There is no pcs_validate or mac_prepare to reject the configuration
earlier.

The same path already exists for family C (RTL8367S, RTL8367SB with
HSGMII). This patch adds family D (RTL8367S-VB at the end of the series)
as another way to reach it.

Should rtl8365mb_pcs_ops provide a .pcs_an_restart callback?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/e84d76ee-03df-49b0-8c9a-b289dfae8728%40yahoo.com