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

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

[Imre Kaloz]

prestera_pcs_config() writes a new mode or inband setting to a port
that is already enabled, as when phylink switches an SFP port to
inband SGMII. The firmware can then report link down and no link up
until the port is disabled and enabled again.

Disable the port before such a write. Exercising this needs a link
partner, so no in-tree test covers it.

Fixes: 52323ef75414 ("net: marvell: prestera: add phylink support")
Signed-off-by: Imre Kaloz <kaloz@kernel.org>
---
 .../net/ethernet/marvell/prestera/prestera_main.c  | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

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
@@ -307,13 +307,14 @@ static int prestera_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			       bool permit_pause_to_mac)
 {
 	struct prestera_port *port = prestera_pcs_to_port(pcs);
-	struct prestera_port_mac_config cfg_mac;
+	struct prestera_port_mac_config cfg_mac, old;
 	int err;
 
 	err = prestera_port_cfg_mac_read(port, &cfg_mac);
 	if (err)
 		return err;
 
+	old = cfg_mac;
 	cfg_mac.admin = true;
 	cfg_mac.fec = PRESTERA_PORT_FEC_OFF;
 	cfg_mac.inband = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
@@ -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)) {
+		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;
-- 
2.47.3

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Imre Kaloz]

On Wed, Nov 11, 2026, at 15:39, Elad Nachman wrote:

<snip>

>> +     /* 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)) {
>> +             old.admin = false;
>> +             err = prestera_port_cfg_mac_write(port, &old);
>> +             if (err)
>> +                     return err;
>> +     }
> 
> From my experience, if you take the port down, it takes time for the firmware
> state machine to act accordingly, so you need some significant delay here
> (1-3 seconds) before you write the new configuration.

Thanks Elad. I found this bringing up the SFP+ ports on a Delta TN48M
(98DX3265) with firmware 4.1.0. With an S-RJ01 module in sfp-52,
the port came up in 30 of 30 boots with this patch, about 2.1 s after
link up, and in 21 of 30 without it. The disable write waits for the
firmware’s acknowledgement, and I saw no failures without a delay.

Is the delay needed on particular firmware versions or devices? This
path runs under rtnl when an SFP module changes the mode, so I would
rather not sleep for seconds there. If the firmware reports when the
port is really down, I can wait for that instead, with a bounded
timeout.


Thanks,
Imre

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

[Imre Kaloz]

Hi Elad,

On Thu, Nov 12, 2026, at 09:22, Elad Nachman wrote:

>> > From my experience, if you take the port down, it takes time for the firmware
>> > state machine to act accordingly, so you need some significant delay here
>> > (1-3 seconds) before you write the new configuration.
>> 
>> Thanks Elad. I found this bringing up the SFP+ ports on a Delta TN48M
>> (98DX3265) with firmware 4.1.0. With an S-RJ01 module in sfp-52,
>> the port came up in 30 of 30 boots with this patch, about 2.1 s after
>> link up, and in 21 of 30 without it. The disable write waits for the
>> firmware’s acknowledgement, and I saw no failures without a delay.
>> 
>> Is the delay needed on particular firmware versions or devices? This
>> path runs under rtnl when an SFP module changes the mode, so I would
>> rather not sleep for seconds there. If the firmware reports when the
>> port is really down, I can wait for that instead, with a bounded
>> timeout.
> 
> Your best shot is monitoring what *prestera_hw_port_phy_mode_get* 
> <https://elixir.bootlin.com/linux/v7.3.1/C/ident/prestera_hw_port_phy_mode_get>() 
> returns as mdix status.

Thanks. I looked at prestera_hw_port_phy_mode_get(): it only maps the
firmware's mdix byte to ETH_TP_MDI/MDI_X/AUTO, anything else becoming
ETH_TP_MDI_INVALID, and the driver never calls it for SFP ports (the
sfp-52 port goes through phylink and the MAC mode attribute only).

Two things the code cannot tell me: which mdix value does the firmware
report while the port is admin down (INVALID?), and does that attribute
carry anything for an SFP port in SGMII mode with a copper module, where
the PHY sits in the module?


Thanks,
Imre

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

[Imre Kaloz]

Hi Elad,

On Sun, Nov 15, 2026, at 08:32, Elad Nachman wrote:

<snip>

>> Thanks. I looked at prestera_hw_port_phy_mode_get(): it only maps the
>> firmware's mdix byte to ETH_TP_MDI/MDI_X/AUTO, anything else becoming
>> ETH_TP_MDI_INVALID, and the driver never calls it for SFP ports (the
>> sfp-52 port goes through phylink and the MAC mode attribute only).
>> 
>> Two things the code cannot tell me: which mdix value does the firmware
>> report while the port is admin down (INVALID?), and does that attribute
> 
> I have managed to find the specific PHY type for TN48M.
> At least for this specific board, it will not report invalid MDI/MDI-X mode.
> 
>> carry anything for an SFP port in SGMII mode with a copper module, where
>> the PHY sits in the module?
> 
> This API only works for Marvell PHYs. If you have a Marvell PHY at the end of
> The copper module, I think it should be relevant.

Thanks for checking. Then the mdix status cannot tell me when the
port is down on TN48M, and v2 keeps the disable without a delay; the
port came up on every boot without one here. If you know a device or
firmware version where the reconfiguration fails without the delay, I
can add a bounded wait for that case.


Thanks,
Imre

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

[netdev-bot+sashiko]

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

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

[Imre Kaloz]

On Sun, Nov 15, 2026, at 11:59, netdev-bot+sashiko@kernel.org wrote:

<snip>

>> @@ -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?
No. Both use PRESTERA_MAC_MODE_SGMII and phylink selects inband for
both, so swapping a 2.5G module for an SGMII one on an enabled port
skips the disable. v2 will also compare the speed and set SPEED_1000
for SGMII, which is the speed the port already runs with on the SGMII
path I tested. I have no 2.5G module, so the 2500BASE-X case is
untested.


Imre

pw-bot: cr

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

[Imre Kaloz]

Hi,

On Sun, Nov 15, 2026, at 17:49, Elad Nachman wrote:
>> prestera_pcs_config() writes a new mode or inband setting to a port
>> that is already enabled, as when phylink switches an SFP port to
>> inband SGMII. The firmware can then report link down and no link up
>> until the port is disabled and enabled again.
>> 
>> Disable the port before such a write. Exercising this needs a link
>> partner, so no in-tree test covers it.
>> 
>> Fixes: 52323ef75414 ("net: marvell: prestera: add phylink support")
>> Signed-off-by: Imre Kaloz <kaloz@kernel.org>
>> ---
>>  .../net/ethernet/marvell/prestera/prestera_main.c  | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>> 
>> 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
>> @@ -307,13 +307,14 @@ static int prestera_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>>                              bool permit_pause_to_mac)
>>  {
>>       struct prestera_port *port = prestera_pcs_to_port(pcs);
>> -     struct prestera_port_mac_config cfg_mac;
>> +     struct prestera_port_mac_config cfg_mac, old;
>>       int err;
>> 
>>       err = prestera_port_cfg_mac_read(port, &cfg_mac);
>>       if (err)
>>               return err;
>> 
>> +     old = cfg_mac;
>>       cfg_mac.admin = true;
>>       cfg_mac.fec = PRESTERA_PORT_FEC_OFF;
>>       cfg_mac.inband = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
>> @@ -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)) {
>> +             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;
>> --
>> 2.47.3
>> 
> 
> Reviewed-by: Elad Nachman <enachman@marvell.com>

Thanks, Elad.

The Sashiko review found that a switch between 2500BASE-X and SGMII,
which share one MAC mode, skips the disable. v2 also compares the
speed and sets it for SGMII. Since that changes the logic you
reviewed, v2 will not carry your Reviewed-by; I would be glad if you
could look at v2 and add it there.


Thanks,
Imre