Re: [PATCH net-next v5 2/3] net: ti: icssm-prueth: Add priority based RX IRQ handlers

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The new 'do not restart TX during teardown' logic is a check-
  then-act sequence with no lock or ordering against ndo_stop().
- [Medium] icssm_prueth_common_emac_rx_packets() reads the two queues'
  write pointers at different times before the loop and never re-reads…
- [Medium] The shared HP/LP NAPI pollers use a plain, unsynchronised read
  of prueth->emac_configured as the gate for dereferencing a port's…
- [Low] This feature patch also carries a fix for an existing bug, with
  no Fixes: tag.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  extends it to the new shared resources.

--

net: ti: icssm-prueth: Add priority based RX IRQ handlers

This patch adds shared high/low priority RX IRQs and NAPI instances for
switch, HSR and PRP modes. Frames from the two PRU ports are delivered
in order of the IEP arrival timestamp that the firmware records. It also
converts the per-port stats to atomic64_t, adds a firmware revision field
that selects the IEP wrap value, and changes ndo_stop() to stop TX before
it cancels the TX hrtimer.

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
> index 453c9b259d77a..42c83946f17a7 100644
> --- a/drivers/net/ethernet/ti/icssm/icssm_prueth.c
> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth.c

[ ... ]

> @@ -1126,11 +1157,28 @@ static int icssm_emac_ndo_stop(struct net_device *ndev)
>  	/* disable the mac port */
>  	icssm_prueth_port_enable(emac, false);
>  
> +	/* Stop TX first. netif_tx_disable() also waits for an xmit that is
> +	 * already running. Then cancel any tx_hrtimer that xmit may have
> +	 * armed.
> +	 */
> +	netif_tx_disable(ndev);
> +	hrtimer_cancel(&emac->tx_hrtimer);

[Severity: Low]
This isn't a bug, but these TX teardown changes look like a fix for an
existing problem. They are netif_tx_disable() before hrtimer_cancel()
here, plus the new emac_configured checks in
icssm_emac_tx_timer_callback() and icssm_emac_adjust_link().

The problem comes from icssm_prueth_change_mode(), which calls
ndo_stop()/ndo_open() directly while the netdev stays running. Before this
patch, icssm_emac_ndo_stop() never stopped TX, and the timer callback woke
the queue unconditionally. TX could therefore restart while the PRUs were
shut down and shared RAM/OCMC was being re-initialised.

Could this be split into its own patch, with a Fixes: tag pointing at the
commit that introduced icssm_prueth_change_mode()? Right now it is mixed
in with the HPQ/LPQ rework, the atomic64 stats conversion and the fw_rev
probe requirement, so it can't be backported on its own.

> +
>  	/* stop PHY */
>  	phy_stop(emac->phydev);
>  
> -	napi_disable(&emac->napi);
> -	hrtimer_cancel(&emac->tx_hrtimer);
> +	if (PRUETH_IS_EMAC(prueth)) {
> +		napi_disable(&emac->napi);
> +		free_irq(emac->rx_irq, ndev);
> +	} else {
> +		if (!prueth->emac_configured &&
> +		    (PRUETH_IS_SWITCH(prueth) || prueth_is_lre(prueth))) {
> +			napi_disable(&prueth->napi_lpq);
> +			napi_disable(&prueth->napi_hpq);
> +		}
> +		/* Free IRQs on last port before halting PRU */
> +		icssm_prueth_common_free_irqs(emac);
> +	}

[Severity: High]
This is a pre-existing issue, but this patch extends it to the new shared
resources. The shared NAPIs and IRQs are torn down whenever
emac_configured drops to zero, with no check that this port actually
acquired them. What happens if ndo_stop() runs on a port whose ndo_open()
failed?

icssm_prueth_change_mode() calls ndo_open() directly and just returns on
error:

    ret = ndev->netdev_ops->ndo_open(ndev);
    if (ret < 0) {
        netdev_err(ndev, "failed to start: %d", ret);
        return ret;
    }

The netdev keeps __LINK_STATE_START, but its emac_configured bit is never
set and napi_hpq/napi_lpq stay disabled (SCHED|NPSVC). One way to get
here: the switch firmware is missing, so
icssm_prueth_sw_boot_prus()->rproc_boot() fails during the EMAC to switch
change that bridge creation triggers.

A later "ip link set ethX down" then does:

    dev_close()
      icssm_emac_ndo_stop()
        napi_disable(&prueth->napi_lpq)
          napi_disable_locked()   <- spins forever on SCHED|NPSVC

and it does this while holding rtnl_lock and the netdev lock.

Would icssm_prueth_common_free_irqs() then also call free_irq() on rx_lp
and rx_hp IRQs that were never requested, with rproc_shutdown() and
icss_iep_exit() running a second time?

The EMAC path has the same pattern with the unconditional
napi_disable(&emac->napi) and free_irq(emac->rx_irq, ndev), which the
baseline also had.

[ ... ]

> @@ -1662,6 +1707,11 @@ static enum hrtimer_restart icssm_emac_tx_timer_callback(struct hrtimer *timer)
>  {
>  	struct prueth_emac *emac =
>  			container_of(timer, struct prueth_emac, tx_hrtimer);
> +	struct prueth *prueth = emac->prueth;
> +
> +	/* Don't restart TX on a port that ndo_stop() is tearing down */
> +	if (!(READ_ONCE(prueth->emac_configured) & BIT(emac->port_id)))
> +		return HRTIMER_NORESTART;
>  
>  	if (netif_queue_stopped(emac->ndev))
>  		netif_wake_queue(emac->ndev);

[Severity: High]
Is this check enough to keep TX from restarting during teardown? The
check and the wake are not atomic with respect to icssm_emac_ndo_stop().
That function does a plain clear of the bit, then netif_tx_disable(),
then hrtimer_cancel():

CPU0 (tx_hrtimer callback)          CPU1 (icssm_emac_ndo_stop)
READ_ONCE(emac_configured) has bit
                                    emac_configured &= ~BIT(port_id)
                                    icssm_prueth_port_enable(emac, false)
                                    netif_tx_disable(ndev)
netif_queue_stopped() -> true
netif_wake_queue()
                                    hrtimer_cancel() returns

hrtimer_cancel() only waits for the callback to finish. Nothing stops the
queue again afterwards.

In the icssm_prueth_change_mode() path the qdisc is still active. So
ndo_start_xmit() could run against the PRU queues while the PRUs are shut
down and icssm_prueth_hostinit() clears shared RAM/OCMC.

icssm_emac_adjust_link() has the same check-then-wake pattern. It uses a
plain load under emac->lock, and ndo_stop() never takes that lock.

emac_configured itself is also accessed racily: a plain u8
read-modify-write in ndo_stop(), READ_ONCE() here, and a plain load in
icssm_emac_adjust_link().

Would clearing the bit with release semantics and calling
netif_tx_disable() again after hrtimer_cancel() close this? Serialising
the check-and-wake against teardown with a lock would be another option.

[ ... ]

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c b/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c
> new file mode 100644
> index 0000000000000..1a5e5a84345e1
> --- /dev/null
> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c
> @@ -0,0 +1,309 @@

[ ... ]

> +	port0_configured = prueth->emac_configured & BIT(emac->port_id);
> +	port1_configured = other_emac &&
> +			   (prueth->emac_configured & BIT(other_emac->port_id));

[ ... ]

> +	if (port1_configured) {
> +		queue_desc_o = other_emac->rx_queue_descs + qid2;

[Severity: Medium]
Are these plain loads of emac_configured enough to make rx_queue_descs
safe to dereference?

When the second port opens while the first is already up,
icssm_prueth_sw_port_config() stores emac->rx_queue_descs. After that,
icssm_emac_ndo_open() sets the bit:

    prueth->emac_configured |= BIT(emac->port_id);

In this case icssm_prueth_sw_init_fdb_table(), icssm_prueth_sw_boot_prus()
and icssm_prueth_common_request_irqs() all return early. So no lock,
barrier or MMIO write orders the rx_queue_descs store before the bit
store.

On SMP (AM57xx), could the HPQ/LPQ poller on the other CPU see the bit
before the pointer? If the port has never been opened since probe, it
would then compute other_emac->rx_queue_descs + qid2 from NULL and call
readb()/readw() on the result.

On the teardown side, ndo_stop() for a non-last port clears its bit but
does not synchronise with the shared NAPIs. A poll that has already read
port1_configured as true can keep calling icssm_emac_rx_packet(other_emac,
...) and writing its rd_ptr, up to the budget, while ndo_stop() runs.

Are smp_store_release()/smp_load_acquire() on the mask (or a lock shared
with the pollers), plus napi_synchronize() when a non-last port stops,
needed here?

[ ... ]

> +		bd_rd_ptr_o = readw(&queue_desc_o->rd_ptr);
> +		bd_wr_ptr_o = readw(&queue_desc_o->wr_ptr);
> +		port1_q_empty = (bd_rd_ptr_o == bd_wr_ptr_o);

[ ... ]

> +	while (!port0_q_empty || !port1_q_empty) {
> +		rd_buf_desc = readl(shared_ram + bd_rd_ptr);
> +		rd_buf_desc_o = readl(shared_ram + bd_rd_ptr_o);

[ ... ]

> +		} else if (!port0_q_empty) {
> +			/* Packet(s) in port0 queue only */
> +			port = 0;
> +		} else {
> +			/* Packet(s) in port1 queue only */
> +			port = 1;
> +		}

[Severity: Medium]
Can this deliver frames out of wire order? bd_wr_ptr and bd_wr_ptr_o are
each read once before the loop, at different times, and never re-read.

Consider:

  readw(&queue_desc->wr_ptr)     port0 looks empty
  frame A completes on port0
  frame B completes on port1
  readw(&queue_desc_o->wr_ptr)   port1 has B

port0_q_empty stays true for the whole loop. The else branch picks port1,
and B goes to netif_receive_skb() without being compared against A's
timestamp. A is only seen on the next poll.

The same thing happens once port0's snapshot has been used up. In HSR/PRP,
consecutive frames of one flow can arrive first on different ports, so
this could show up as TCP reordering.

Would re-reading wr_ptr for a queue that looks empty, before taking a
frame from the peer queue, avoid this?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005154654.576663-1-parvathi%40couthit.com