[PATCH net v2 0/2] Fix switch mode for offload and non-offload cases

[PATCH net v2 0/2] Fix switch mode for offload and non-offload cases

[Meghana Malladi]

Non-offloaded switch mode is failing to forward packets. This patch series
introduces fixes to ensure smooth operation of switch mode in both offload
and non-offload cases.

patch(1/2): Fixes port_vlan for both the ports in bridge mode and adds
            non-volatile uc/mc flooding flags
patch(2/2): Fixes R30 command timing and a NULL emac dereference

v1: https://lore.kernel.org/all/20260930124356.3083743-1-m-malladi@ti.com/
logs: https://gist.github.com/MeghanaMalladiTI/7841307d5222ae11192d86e7a0c42535

Changes(v2-v1):
- Split the patch into two separate fixes patch with appropriate commit
  messages as suggested by Andrew Lunn <andrew@lunn.ch>

Meghana Malladi (2):
  net: ti: icssg-prueth: Fix bridge port_vlan and flooding state
  net: ti: icssg-prueth: Fix R30 command timing and NULL emac
    dereference

 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 47 +++++++++++++++-----
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  4 ++
 2 files changed, 40 insertions(+), 11 deletions(-)


base-commit: 4dca97647a3e70eb6446966db8aff4e099775833
-- 
2.43.0

[PATCH net v2 1/2] net: ti: icssg-prueth: Fix bridge port_vlan and flooding state

[Meghana Malladi]

Bridge formation only set port_vlan on the second emac, leaving the first
without VLAN configuration; fixed by setting port_vlan on both emacs.
Firmware restart in non-offload bridge mode cleared IFF_PROMISC-driven
UC/MC flooding state, breaking unicast forwarding; fixed by tracking
flooding state and restoring it after restart.

Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 20 +++++++++++++++++++-
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  4 ++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 591be5c8056b..746afd5d00c9 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -241,6 +241,16 @@ static void icssg_enable_fw_offload(struct prueth *prueth)
 					icssg_set_port_state(emac, ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE);
 			}
 		}
+
+		/* Re-apply flooding state cleared by firmware restart.
+		 * Not needed in switch mode; FDB handles forwarding there.
+		 */
+		if (!prueth->is_switch_mode && netif_running(emac->ndev)) {
+			if (emac->uc_flooding_enabled)
+				icssg_set_port_state(emac, ICSSG_EMAC_PORT_UC_FLOODING_ENABLE);
+			if (emac->mc_flooding_enabled)
+				icssg_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE);
+		}
 	}
 }
 
@@ -1288,14 +1298,21 @@ static void emac_ndo_set_rx_mode_work(struct work_struct *work)
 	if (promisc) {
 		icssg_set_port_state(emac, ICSSG_EMAC_PORT_UC_FLOODING_ENABLE);
 		icssg_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE);
+		emac->uc_flooding_enabled = true;
+		emac->mc_flooding_enabled = true;
 		return;
 	}
 
 	if (allmulti) {
 		icssg_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE);
+		emac->mc_flooding_enabled = true;
+		emac->uc_flooding_enabled = false;
 		return;
 	}
 
+	emac->uc_flooding_enabled = false;
+	emac->mc_flooding_enabled = false;
+
 	if (emac->prueth->is_hsr_offload_mode) {
 		/* Track basic add/delete via callbacks */
 		__dev_mc_sync(ndev, icssg_prueth_hsr_add_mcast,
@@ -1919,7 +1936,8 @@ static int prueth_netdevice_port_link(struct net_device *ndev,
 		    prueth->br_members & BIT(PRUETH_PORT_MII1)) {
 			prueth->is_switch_mode = true;
 			prueth->default_vlan = PRUETH_DFLT_VLAN_SW;
-			emac->port_vlan = prueth->default_vlan;
+			prueth->emac[PRUETH_MAC0]->port_vlan = prueth->default_vlan;
+			prueth->emac[PRUETH_MAC1]->port_vlan = prueth->default_vlan;
 			icssg_change_mode(prueth);
 		}
 	}
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index df93d15c5b78..832e654a102c 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -242,6 +242,10 @@ struct prueth_emac {
 	bool offload_fwd_mark;
 	int port_vlan;
 
+	/* UC/MC flooding state - preserved across firmware restarts */
+	unsigned int uc_flooding_enabled : 1;
+	unsigned int mc_flooding_enabled : 1;
+
 	struct delayed_work stats_work;
 	u64 stats[ICSSG_NUM_MIIG_STATS];
 	u64 pa_stats[ICSSG_NUM_PA_STATS];
-- 
2.43.0

Re: [PATCH net v2 1/2] net: ti: icssg-prueth: Fix bridge port_vlan and flooding state

[netdev-bot+sashiko]

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

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

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).

- [High] The flooding restore misses the restart path the commit message
  is about.
- [High] At this commit, icssg_enable_fw_offload() has no NULL check on
  `emac = prueth->emac[mac]` (line 215).
- [Medium] The new flooding flags are written without any synchronization
  against the restore.
- [Low] Concern: the `!prueth->is_switch_mode` guard in
  icssg_enable_fw_offload() also covers HSR/PRP offload mode, so the…
- [Low] The commit fixes two user-visible bugs but has no Fixes: tag, and
  it bundles two unrelated fixes in one patch.
- [Low] The patch treats switch-mode flooding inconsistently.
- [Low] Concern: mc_flooding_enabled is updated only by
  emac_ndo_set_rx_mode_work(), while…

--

net: ti: icssg-prueth: Fix bridge port_vlan and flooding state

This patch sets port_vlan on both emacs when the driver enters switch
mode. It also records the UC/MC flooding state that IFF_PROMISC and
IFF_ALLMULTI set up in emac_ndo_set_rx_mode_work(). The recorded state
is reapplied from icssg_enable_fw_offload() after a firmware restart
when the driver is not in switch mode.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 591be5c8056b4..746afd5d00c9e 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -241,6 +241,16 @@ static void icssg_enable_fw_offload(struct prueth *prueth)
>  					icssg_set_port_state(emac, ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE);
>  			}
>  		}
> +
> +		/* Re-apply flooding state cleared by firmware restart.
> +		 * Not needed in switch mode; FDB handles forwarding there.
> +		 */

[Severity: Low]
This isn't a bug, but does this comment match the rest of the driver?

It says flooding is not needed in switch mode. However,
emac_ndo_set_rx_mode_work() has no mode check, so in switch mode it
still enables UC/MC flooding and sets the tracked flags.
prueth_switchdev_attr_br_flags_set() also programs
ICSSG_EMAC_PORT_MC_FLOODING_ENABLE/DISABLE from BR_MCAST_FLOOD, and
that flag only applies while the port is bridged.

As a result, right after icssg_change_mode() enters switch mode, the
firmware has flooding off while uc/mc_flooding_enabled still say on. A
later promiscuity change then turns flooding back on.

Should the comment or the switch-mode handling be changed so the two
agree?

> +		if (!prueth->is_switch_mode && netif_running(emac->ndev)) {

[Severity: High]
Can this dereference a NULL emac in the single-port configuration?

At this commit, icssg_enable_fw_offload() does not check for NULL after:

	emac = prueth->emac[mac];

Before this patch, nothing in the loop body touched emac in plain EMAC
mode. The new block evaluates netif_running(emac->ndev) whenever
is_switch_mode is false.

prueth_probe() only sets up netdevs for DT nodes that exist (the
"single EMAC mode" case), so one prueth->emac[] slot stays NULL. The
first open of the remaining port would then go:

emac_ndo_open()
  icssg_enable_fw_offload()
    netif_running(emac->ndev)    <- emac == NULL

The next patch in the series ("net: ti: icssg-prueth: Fix R30 command
timing and NULL emac dereference") adds "if (!emac) continue;". Even so,
this commit on its own breaks bisection on single-port boards. Could
the NULL check be moved into this patch?

> +			if (emac->uc_flooding_enabled)
> +				icssg_set_port_state(emac, ICSSG_EMAC_PORT_UC_FLOODING_ENABLE);
> +			if (emac->mc_flooding_enabled)
> +				icssg_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE);
> +		}

[Severity: High]
Does this restore reach the restart path that the commit message
describes?

icssg_enable_fw_offload() has two callers: emac_ndo_open() on first
open, and icssg_change_mode(). On the bridge-link restart,
icssg_change_mode() runs after is_switch_mode has already been set to
true, so this block is skipped.

The restart that leaves a running, promiscuous port in non-offload
bridge mode is the switch to EMAC transition in
prueth_netdevice_port_unlink(). It calls prueth_emac_restart() directly:

	if (prueth->is_switch_mode) {
		prueth->is_switch_mode = false;
		emac->port_vlan = 0;
		ret = prueth_emac_restart(prueth);
		...
	}

prueth_emac_restart() only reissues the forward state:

	ret = icssg_set_port_state(emac0, ICSSG_EMAC_PORT_FORWARD);
	ret |= icssg_set_port_state(emac1, ICSSG_EMAC_PORT_FORWARD);

prueth_hsr_port_unlink() also calls prueth_emac_restart() directly.

For example, put eth1 and eth2 in br0 and then run
"ip link set eth2 nomaster":

1. del_nbp()->br_manage_promisc() runs. br_port_set_promisc() returns
   early for eth1 because eth1 is already promiscuous.

2. prueth_netdevice_port_unlink() reloads the EMAC firmware and returns.

eth1 is still a bridge port with IFF_PROMISC set, and its
uc/mc_flooding_enabled flags are still true. The firmware, however, now
has flooding disabled, and nothing calls ndo_set_rx_mode again.

Wouldn't unicast forwarding through eth1 stay broken?

Could the restore be done in prueth_emac_restart(), or from every
restart path? The "preserved across firmware restarts" comment on the
new fields in struct prueth_emac is also not true for these unlink
paths.

>  	}
>  }
>  
> @@ -1288,14 +1298,21 @@ static void emac_ndo_set_rx_mode_work(struct work_struct *work)
>  	if (promisc) {
>  		icssg_set_port_state(emac, ICSSG_EMAC_PORT_UC_FLOODING_ENABLE);
>  		icssg_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE);
> +		emac->uc_flooding_enabled = true;
> +		emac->mc_flooding_enabled = true;
>  		return;
>  	}

[Severity: Medium]
Can these flag updates race with the restore in
icssg_enable_fw_offload()?

emac_ndo_set_rx_mode_work() runs from schedule_work() without RTNL. It
sends the firmware commands first and stores the flags afterwards. Each
command holds emac->cmd_lock only for itself and can wait up to 10ms.

icssg_change_mode()->prueth_emac_restart()->icssg_enable_fw_offload()
reads the flags under RTNL. Nothing on that path cancels or flushes
rx_mode_work.

hsr_portdev_setup() opens this window when forwarding is not offloaded.
It calls dev_set_promiscuity(dev, 1), which queues rx_mode_work. It then
calls netdev_master_upper_dev_link(), which runs NETDEV_CHANGEUPPER->
prueth_hsr_port_link()->icssg_change_mode().

One possible interleaving:

rx_mode_work                            icssg_change_mode()
icssg_set_port_state(UC_FLOODING_ENABLE)
  (times out while PRUs restart)
                                        icssg_enable_fw_offload()
                                          uc_flooding_enabled false, skip
emac->uc_flooding_enabled = true;

The flags then say flooding is on while the firmware has it off. That
is the state this patch is meant to fix.

The reverse case, where promisc is being cleared, can leave flooding
enabled with IFF_PROMISC clear.

netif_device_detach() in prueth_emac_restart() does not clear
__LINK_STATE_START, so the queued work still passes its netif_running()
check.

>  
>  	if (allmulti) {
>  		icssg_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE);
> +		emac->mc_flooding_enabled = true;
> +		emac->uc_flooding_enabled = false;
>  		return;
>  	}

[ ... ]

> @@ -1919,7 +1936,8 @@ static int prueth_netdevice_port_link(struct net_device *ndev,
>  		    prueth->br_members & BIT(PRUETH_PORT_MII1)) {
>  			prueth->is_switch_mode = true;
>  			prueth->default_vlan = PRUETH_DFLT_VLAN_SW;
> -			emac->port_vlan = prueth->default_vlan;
> +			prueth->emac[PRUETH_MAC0]->port_vlan = prueth->default_vlan;
> +			prueth->emac[PRUETH_MAC1]->port_vlan = prueth->default_vlan;
>  			icssg_change_mode(prueth);
>  		}
>  	}

[Severity: Low]
This patch fixes two separate user-visible bugs, but it has no Fixes:
tags. Could it be split into two patches, each with its own Fixes: tag?

The single-emac port_vlan assignment appears to come from commit
abd5576b9c57 ("net: ti: icssg-prueth: Add support for ICSSG switch
firmware").

The flooding loss on restart comes from the restart-based mode change
in icssg_change_mode()/prueth_emac_restart(). That was added by commit
95540ad6747c ("net: ti: icssg-prueth: Add support for HSR frame forward
offload").

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027131732.3411403-1-m-malladi%40ti.com

[PATCH net v2 2/2] net: ti: icssg-prueth: Fix R30 command timing and NULL emac dereference

[Meghana Malladi]

emac_adjust_link() called before prueth_emac_start() caused R30 FORWARD
commands to time out (PRU not running), leaving stale R30 values that made
prueth_emac_restart() fail; fixed by deferring adjust_link until after PRU
starts. icssg_enable_fw_offload() dereferences a NULL second emac slot when
the first port opens before the second is initialized; fixed by checking
emac existence before accessing.

Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 27 ++++++++++++--------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 746afd5d00c9..9afda5da3ca3 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -213,6 +213,8 @@ static void icssg_enable_fw_offload(struct prueth *prueth)
 
 	for (mac = PRUETH_MAC0; mac < PRUETH_NUM_MACS; mac++) {
 		emac = prueth->emac[mac];
+		if (!emac)
+			continue;
 		if (prueth->is_hsr_offload_mode) {
 			if (emac->ndev->features & NETIF_F_HW_HSR_TAG_RM)
 				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE);
@@ -288,13 +290,22 @@ static int prueth_emac_common_start(struct prueth *prueth)
 		ret = icssg_config(prueth, emac, slice);
 		if (ret)
 			goto disable_class;
+	}
+
+	ret = prueth_emac_start(prueth);
+	if (ret)
+		goto disable_class;
+
+	/* Reset link state and reconfigure after firmware is running.
+	 * Must be done after prueth_emac_start() so that R30 commands
+	 * (e.g. ICSSG_EMAC_PORT_FORWARD from emac_adjust_link) reach
+	 * the PRU firmware instead of timing out with stale R30 values.
+	 */
+	for (slice = 0; slice < PRUETH_NUM_MACS; slice++) {
+		emac = prueth->emac[slice];
+		if (!emac)
+			continue;
 
-		/* Reset link state to force reconfiguration in
-		 * emac_adjust_link(). Without this, if the link was already up
-		 * before restart, emac_adjust_link() won't detect any state
-		 * change and will skip critical configuration like writing
-		 * speed to firmware.
-		 */
 		emac->link = 0;
 
 		mutex_lock(&emac->ndev->phydev->lock);
@@ -302,10 +313,6 @@ static int prueth_emac_common_start(struct prueth *prueth)
 		mutex_unlock(&emac->ndev->phydev->lock);
 	}
 
-	ret = prueth_emac_start(prueth);
-	if (ret)
-		goto disable_class;
-
 	emac = prueth->emac[ICSS_SLICE0] ? prueth->emac[ICSS_SLICE0] :
 	       prueth->emac[ICSS_SLICE1];
 	ret = icss_iep_init(emac->iep, &prueth_iep_clockops,
-- 
2.43.0

Re: [PATCH net v2 2/2] net: ti: icssg-prueth: Fix R30 command timing and NULL emac dereference

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 3 · 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).

- [High] The commit message describes the crash as happening when "the
  first port opens before the second is initialized".
- [Medium] The NULL-emac guard this patch adds to
  icssg_enable_fw_offload() fixes a crash introduced by the previous…
- [Medium] The patch fixes a user-visible regression but has no Fixes:
  tag.
- [Medium] prueth_emac_restart() (line 1858) stops and restarts the PRU
  firmware while phylib is still running for the active ports.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  prueth_cleanup_tx_ts() (icssg_common.c:1424-1434) frees every non-NULL…

--

net: ti: icssg-prueth: Fix R30 command timing and NULL emac dereference

This patch moves the per-port emac_adjust_link() reconfiguration in
prueth_emac_common_start() so it runs after prueth_emac_start() has booted
the PRU firmware. The goal is to stop R30 commands from timing out. It
also adds a NULL check for the prueth->emac[] slots in
icssg_enable_fw_offload().

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 746afd5d00c9e..9afda5da3ca3a 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -213,6 +213,8 @@ static void icssg_enable_fw_offload(struct prueth *prueth)
>  
>  	for (mac = PRUETH_MAC0; mac < PRUETH_NUM_MACS; mac++) {
>  		emac = prueth->emac[mac];
> +		if (!emac)
> +			continue;

[Severity: Medium]
The commit message says icssg_enable_fw_offload() "dereferences a NULL
second emac slot when the first port opens before the second is
initialized". Is that what actually triggers it?

prueth_probe() calls prueth_netdev_init() for both ports, which fills both
prueth->emac[] slots, before the first register_netdev(). So at ndo_open
time a slot is only NULL when the port is missing from DT or disabled
there.

In the baseline, every emac dereference in icssg_enable_fw_offload() was
under is_hsr_offload_mode or is_switch_mode, and both of those modes need
two ports. The dereference that plain EMAC mode can reach seems to come
from the previous patch in this series, commit 12b7fbafdb45 ("net: ti:
icssg-prueth: Fix bridge port_vlan and flooding state"):

    if (!prueth->is_switch_mode && netif_running(emac->ndev)) {

Take a single-port board with that commit applied but not this one. The
first emac_ndo_open()->icssg_enable_fw_offload() would then dereference a
NULL emac, so a bisect would land on the intermediate commit.

Could this guard be folded into that patch, with the R30 ordering change
kept as its own patch?

>  		if (prueth->is_hsr_offload_mode) {
>  			if (emac->ndev->features & NETIF_F_HW_HSR_TAG_RM)
>  				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE);
> @@ -288,13 +290,22 @@ static int prueth_emac_common_start(struct prueth *prueth)
>  		ret = icssg_config(prueth, emac, slice);
>  		if (ret)
>  			goto disable_class;
> +	}
> +
> +	ret = prueth_emac_start(prueth);
> +	if (ret)
> +		goto disable_class;
> +

[Severity: Medium]
This fixes a user-visible regression: R30 FORWARD commands time out, and
prueth_emac_restart() then fails when ports with link up join or leave a
bridge or HSR. The commit has no Fixes: tag, though.

The emac_adjust_link() call ahead of prueth_emac_start() came from
06feac15406f ("net: ti: icssg-prueth: Fix emac link speed handling").

The emac->link = 0 reset, which makes adjust_link send FORWARD on every
restart while the link is up, came from 147792c395db ("net: ti:
icssg-prueth: Fix ping failure after offload mode setup when link speed is
not 1G").

Should this carry Fixes: tags for one or both of those, so that stable
trees containing them also pick up this fix?

> +	/* Reset link state and reconfigure after firmware is running.
> +	 * Must be done after prueth_emac_start() so that R30 commands
> +	 * (e.g. ICSSG_EMAC_PORT_FORWARD from emac_adjust_link) reach
> +	 * the PRU firmware instead of timing out with stale R30 values.
> +	 */

[Severity: Medium]
Does this close the R30 timeout window completely?

prueth_emac_restart() stops and restarts the firmware without calling
phy_stop() and without holding phydev->lock. That means phylib can still
call emac_adjust_link() on its own while the PRU is halted:

prueth_emac_restart()
    prueth_emac_common_stop()       <- PRU halted
    prueth_emac_common_start()
        icssg_config()
            emac_r30_cmd_init()
        prueth_emac_start()

Suppose the link changes (autoneg completes, or the link flaps) after
prueth_emac_common_stop(), or between emac_r30_cmd_init() and
prueth_emac_start(). The phylib state machine then calls
emac_adjust_link()->icssg_set_port_state(). That writes the R30 command
words to a halted PRU, times out in read_poll_timeout(), and leaves the
words in place. This looks like the same failure the commit message
describes.

There is also a data race on the link fields. icssg_config() writes
emac->speed and emac->duplex, and this loop writes emac->link = 0, all
without phydev->lock. The phylib callback updates the same fields while
holding phydev->lock.

Should the restart path quiesce phylib, or hold phydev->lock, across the
stop/start window?

> +	for (slice = 0; slice < PRUETH_NUM_MACS; slice++) {
> +		emac = prueth->emac[slice];
> +		if (!emac)
> +			continue;
>  
> -		/* Reset link state to force reconfiguration in
> -		 * emac_adjust_link(). Without this, if the link was already up
> -		 * before restart, emac_adjust_link() won't detect any state
> -		 * change and will skip critical configuration like writing
> -		 * speed to firmware.
> -		 */
>  		emac->link = 0;
>  
>  		mutex_lock(&emac->ndev->phydev->lock);

[Severity: High]
The loop only checks emac. Can emac->ndev->phydev still be NULL at this
point?

prueth_probe() fills both prueth->emac[] slots and then does:

prueth_probe()
    register_netdev(prueth->emac[PRUETH_MAC0]->ndev);
    emac_phy_connect(prueth->emac[PRUETH_MAC0]);
    register_netdev(prueth->emac[PRUETH_MAC1]->ndev);
    emac_phy_connect(prueth->emac[PRUETH_MAC1]);

Userspace can open eth0 as soon as it is registered, for example via udev
or a network manager. emac_ndo_open() sees emacs_initialized == 0 and
calls prueth_emac_common_start(). This loop then reaches slice 1, where
emac[1] is non-NULL but its ndev->phydev has not been set yet. So
mutex_lock(&emac->ndev->phydev->lock) dereferences NULL.

prueth_remove() has a similar window:

    phy_disconnect(prueth->emac[i]->ndev->phydev);
    prueth->emac[i]->ndev->phydev = NULL;
    unregister_netdev(prueth->registered_netdevs[i]);

emac[0] stays non-NULL here. If eth1 is opened during this window, the
loop walks slice 0 with a NULL phydev.

emac_ndo_stop() already checks if (ndev->phydev) before phy_stop(). The
dereference itself existed before this patch. However, the commit message
describes this exact scenario, "the first port opens before the second is
initialized", and that scenario still crashes.

Should this loop also skip slices that have no phydev?

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. On link
down, the emac_adjust_link() call made from this loop (and from the phylib
state machine) reaches prueth_cleanup_tx_ts():

    } else {
        netif_tx_stop_all_queues(ndev);
        prueth_cleanup_tx_ts(emac);
    }

prueth_cleanup_tx_ts() frees every non-NULL emac->tx_ts_skb[] slot. No
lock synchronizes it with the TX path or the tx_ts IRQ thread. Also,
netif_tx_stop_all_queues() does not wait for an icssg_ndo_start_xmit()
that is already running on another CPU.

Can this end up freeing an ERR_PTR? The xmit path reserves a slot in
prueth_tx_ts_cookie_get() first:

    emac->tx_ts_skb[i] = ERR_PTR(-EBUSY); /* reserve slot */

and stores the skb in icssg_ndo_start_xmit() only later:

    emac->tx_ts_skb[tx_ts_cookie] = skb_get(skb);

If cleanup runs between those two stores, it calls
dev_kfree_skb_any(ERR_PTR(-EBUSY)).

The xmit error paths also free the slot and set it to NULL. And
tx_ts_work() does:

    skb = emac->tx_ts_skb[tsr.cookie];
    emac->tx_ts_skb[tsr.cookie] = NULL;	/* free slot */

and uses and frees skb later on. Could a concurrent
prueth_cleanup_tx_ts() free the same skb in between, causing a double
free or a use-after-free?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027131732.3411403-1-m-malladi%40ti.com

Re: [PATCH net v2 0/2] Fix switch mode for offload and non-offload cases

[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.