Re: [PATCH net] net: marvell: prestera: disable port before changing its MAC mode

[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] The new disable/re-enable step in prestera_pcs_config()
  (drivers/net/ethernet/marvell/prestera/prestera_main.c) only runs when…

--

net: marvell: prestera: disable port before changing its MAC mode

This commit makes prestera_pcs_config() disable an already enabled port
before it writes a new MAC mode or inband setting. Without this, the
firmware can report link down and never report link up again.

> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> index a82e7a8029851..39ebd1944be55 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c

[ ... ]

> @@ -339,6 +340,17 @@ static int prestera_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>  		break;
>  	}
>  
> +	/* Firmware may not report link up after the mode or inband setting of
> +	 * an enabled port changes until the port is disabled and re-enabled.
> +	 */
> +	if (old.admin && (old.mode != cfg_mac.mode ||
> +			  old.inband != cfg_mac.inband)) {

[Severity: Medium]
Does this check catch a change between 2500BASE-X and SGMII?

The PRESTERA_MAC_MODE_* values in prestera_hw.h include no 2500BASE-X
mode. Both interfaces map to the same firmware mode earlier in
prestera_pcs_config():

	case PHY_INTERFACE_MODE_2500BASEX:
		cfg_mac.speed = SPEED_2500;
		cfg_mac.duplex = DUPLEX_FULL;
		cfg_mac.mode = PRESTERA_MAC_MODE_SGMII;
		break;
	case PHY_INTERFACE_MODE_SGMII:
		cfg_mac.mode = PRESTERA_MAC_MODE_SGMII;
		break;

The only field that tells them apart is the speed value that
prestera_hw_port_mac_mode_set() sends to the firmware.

Say phylink moves an enabled port between these two interfaces and the
inband value stays the same, for example when a 2.5G SFP is swapped for
an SGMII copper module. Then old.mode == cfg_mac.mode and
old.inband == cfg_mac.inband. The disable step is skipped and the
SerDes rate changes on a live port.

This looks reachable for two reasons:

- port->cfg_mac.admin stays true across module swaps.
  prestera_mac_link_down() only clears state_mac, and there is no
  pcs_disable op.

- prestera has no pcs_inband_caps op, so phylink_pcs_neg_mode() can pick
  PHYLINK_PCS_NEG_INBAND_ENABLED for both 2500BASE-X and SGMII. That
  makes old.inband == cfg_mac.inband == 1 possible.

There is a second gap. The SGMII case never sets cfg_mac.speed, so after
2500BASE-X the port keeps speed == SPEED_2500. Adding speed to this
comparison alone would still miss a 2500BASE-X to SGMII change. Would
SGMII also need to set its own speed here?

The source doesn't show whether the firmware needs a disable and enable
for a speed-only change within PRESTERA_MAC_MODE_SGMII. The commit
message only mentions the "mode or inband setting". Should the bounce
cover this case too?

> +		old.admin = false;
> +		err = prestera_port_cfg_mac_write(port, &old);
> +		if (err)
> +			return err;
> +	}
> +
>  	err = prestera_port_cfg_mac_write(port, &cfg_mac);
>  	if (err)
>  		return err;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111101521.9878-1-kaloz%40kernel.org