Re: [PATCH net-next v3 9/9] net: dsa: realtek: rtl8365mb: add support for RTL8367S-VB

[netdev-bot+sashiko@kernel.org]

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

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

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] On family D, rtl8365mb_pcs_get_state() replaces the configured
  speed and duplex with fixed values.
- [Low] The comment above RTL8365MB_SDS_EXT_INTERFACE_ID
  (drivers/net/dsa/realtek/rtl8365mb_main.c:347-350) says "The embedded…

--

net: dsa: realtek: rtl8365mb: add support for RTL8367S-VB

This patch adds the RTL8367S-VB (chip ID 0x6642, version 0x0010) to the
chip table as the first family D chip. On this chip the SGMII/HSGMII
SerDes is on external interface 0 (port 6) and RGMII is on external
interface 1 (port 7). The patch also caps priv->num_ports at 8 for
family D and updates the comment on the learn limit.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 5d804099f131a..c6045607ade06 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -874,6 +881,19 @@ static const struct rtl8365mb_chip_info rtl8365mb_chip_infos[] = {
>  		.jam_table = rtl8365mb_init_jam_8365mb_vc,
>  		.jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
>  	},
> +	{
> +		.name = "RTL8367S-VB",
> +		.chip_id = 0x6642,
> +		.chip_ver = 0x0010,
> +		.family = RTL8365MB_FAMILY_D,
> +		.extints = {
> +			{ 6, 0, PHY_INTF(SGMII) | PHY_INTF(HSGMII) },

[Severity: Medium]
This entry lets port 6 run SGMII on a family D chip. That makes the
family D branch of rtl8365mb_pcs_get_state() reachable for the first
time.

Does that branch discard the forced speed and duplex that phylink has
already filled in?

drivers/net/dsa/realtek/rtl8365mb_main.c:rtl8365mb_pcs_get_state() {
    ...
	if (is_d) {
		state->duplex = DUPLEX_FULL;
		state->speed = state->interface == PHY_INTERFACE_MODE_2500BASEX ?
				SPEED_2500 : SPEED_1000;
		return;
	}
    ...
}

rtl8365mb_phylink_get_caps() advertises MAC_10 | MAC_100 | MAC_1000FD
for this port, so phylink accepts a forced 10/100 setting.

Take port 6 as a PHY-less SGMII user port with
managed = "in-band-status", and run:

  ethtool -s <port> autoneg off speed 100 duplex full

phylink_ethtool_ksettings_set() stores link_config.speed = 100.
phylink_pcs_neg_mode() then picks PHYLINK_PCS_NEG_OUTBAND, and the mode
stays MLO_AN_INBAND because there is no phydev.

After that:

phylink_resolve()
  phylink_mac_pcs_get_state()
    state->speed = pl->link_config.speed;    /* 100 */
    rtl8365mb_pcs_get_state()
      state->speed = SPEED_1000;
  phylink_link_up()
    rtl8365mb_phylink_mac_link_up()          /* 1000/full */
      rtl8365mb_ext_config_forcemode()

Would the MAC then be forced to 1000 Mbps while the configured speed is
100? Would ethtool get_link_ksettings also report 1000/full?

The family C branch avoids this by reading the forced values back from
SDS_MISC. When in-band autoneg is disabled, could the family D branch
keep the speed and duplex that phylink passed in?

[Severity: Low]
This isn't a bug, but this entry makes the comment above
RTL8365MB_SDS_EXT_INTERFACE_ID inaccurate:

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

On RTL8367S-VB the SerDes is on external interface 0, and external
interface 1 is the port 7 RGMII interface.

rtl8365mb_pcs_config() still does:

	const int id = RTL8365MB_SDS_EXT_INTERFACE_ID;

This only works today because every use of id is in the !is_d branch.

Could this comment be updated for family D, as was done for the
LEARN_LIMIT comment?

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