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

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
net: ti: icssm-prueth: Add priority based RX IRQ handlers

This patch adds support for priority based interrupt handling for the STP/
RSTP Switch, HSR and PRP protocols along with extra logic to address first
come first served to avoid port dominance. The driver will read the RX HW
timestamp from the frame to achieve wire-arrival ordering.

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
> --- a/drivers/net/ethernet/ti/icssm/icssm_prueth.c
> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
[ ... ]
> @@ -1188,26 +1188,26 @@ static int icssm_emac_ndo_stop(struct net_device *ndev)
>  	/* disable the mac port */
>  	icssm_prueth_port_enable(emac, false);
>  
> +	netif_stop_queue(ndev);
> +
>  	/* stop PHY */
>  	phy_stop(emac->phydev);
>  
> -	napi_disable(&emac->napi);
>  	hrtimer_cancel(&emac->tx_hrtimer);
>  
[ ... ]
>  	/* stop the PRU */
>  	if (!PRUETH_IS_EMAC(prueth))
>  		icssm_prueth_sw_shutdown_prus(emac, ndev);
>  
>  	if (prueth_is_lre(prueth))
>  		icssm_prueth_lre_cleanup(prueth);

[Severity: High]
This is a pre-existing issue, but is the LRE duplicate detection timer
prematurely canceled when a single port goes down?

In icssm_emac_ndo_stop, icssm_prueth_lre_cleanup is called unconditionally
when any interface is brought down.

If one port in the HSR/PRP bridge goes down while the other remains active,
this cancels the shared tbl_check_timer, potentially causing duplicate
frames to stop being detected and dropped by the hardware.

> 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
> --- /dev/null
> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c
> @@ -0,0 +1,286 @@
[ ... ]
> +static int icssm_prueth_common_emac_rx_packets(struct prueth_emac *emac,
> +					       int quota, u8 qid1, u8 qid2)
> +{
[ ... ]
> +		if ((*pkt_info_p).length < EMAC_MIN_PKTLEN) {
> +			/* Undersized frame: firmware should have filtered
> +			 * these before they reach the host queue. Advance
> +			 * the read pointer to skip it.
> +			 */
> +			update_rd_ptr = *bd_wr_ptr_p;
> +			ndevstats_p->rx_length_errors++;

[Severity: High]
This is a pre-existing issue, but does this error path flush the entire
pending RX queue?

When the driver detects an undersized or oversized packet, it attempts to
skip it by updating the read pointer. However, setting update_rd_ptr to
*bd_wr_ptr_p appears to assign the hardware's write pointer (the very end of
the queue), rather than advancing by one descriptor.

Could this discard all pending descriptors instead of just the malformed one?

[Severity: High]
Does this introduce a race condition on the network statistics?

This patch introduces split NAPI polling with separate priority threads
(napi_hpq and napi_lpq) that process the high and low priority queues across
both physical ports. These NAPI instances can be scheduled on different CPUs
concurrently.

Both instances invoke icssm_prueth_common_emac_rx_packets, which updates raw
unsigned long statistics like ndevstats_p->rx_length_errors (and 64-bit
stats like emac->stats.rx_bytes in icssm_emac_rx_packet) using += without
atomic instructions or locks.

On 32-bit platforms, could this cause lost updates for concurrent writers, or
torn reads when read by ndo_get_stats64?

> +		} else if ((*pkt_info_p).length > EMAC_MAX_FRM_SUPPORT) {
> +			/* Oversized frame: firmware should have filtered
> +			 * these before they reach the host queue. Advance
> +			 * the read pointer to skip it.
> +			 */
> +			update_rd_ptr = *bd_wr_ptr_p;
> +			ndevstats_p->rx_length_errors++;
> +		} else {
> +			update_rd_ptr = *bd_rd_ptr_p;
> +			ret = icssm_emac_rx_packet(emac_p, &update_rd_ptr,
> +						   pkt_info_p, rxqueue_p);
> +			if (ret)
> +				return used;

[Severity: High]
This is a pre-existing issue, but does a memory allocation failure in this RX
path cause a permanent RX queue stall?

If icssm_emac_rx_packet fails to allocate an SKB, it returns -ENOMEM. The
driver immediately aborts the NAPI polling loop without updating the hardware
read pointer or consuming the packet.

Because it returns used (which is less than budget), it triggers
napi_complete_done() which re-enables the RX interrupt. Since the packet is
still pending in the hardware queue, the interrupt fires again immediately in
an infinite loop.

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth_lre.c b/drivers/net/ethernet/ti/icssm/icssm_prueth_lre.c
> --- a/drivers/net/ethernet/ti/icssm/icssm_prueth_lre.c
> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth_lre.c
> @@ -152,6 +152,14 @@ static void icssm_prueth_lre_protocol_init(struct prueth *prueth)
>  	       dram1 + ICSS_LRE_SUP_ADDR_LOW);
>  }
>  
> +static void icssm_prueth_lre_config_packet_timestamping(struct prueth *prueth)
> +{
> +	void __iomem *sram = prueth->mem[PRUETH_MEM_SHARED_RAM].va;
> +
> +	writeb(1, sram + ICSS_LRE_PRIORITY_INTRS_STATUS_OFFSET);
> +	writeb(1, sram + ICSS_LRE_TIMESTAMP_PKTS_STATUS_OFFSET);
> +}
> +
[ ... ]
> @@ -202,6 +210,11 @@ void icssm_prueth_lre_config(struct prueth *prueth)
>  	icssm_prueth_lre_init(prueth);
>  	icssm_prueth_lre_dbg_init(prueth);
>  	icssm_prueth_lre_protocol_init(prueth);
> +	/* Enable per-packet timestamping so the driver can order
> +	 * received frames by arrival time across the two slave ports.
> +	 */
> +	icssm_prueth_lre_config_packet_timestamping(prueth);
> +
>  }

[Severity: High]
Does this fail to enable hardware timestamping in Switch mode?

The patch introduces timestamp-based packet ordering for Switch, HSR, and PRP
modes, requiring configuration via ICSS_LRE_PRIORITY_INTRS_STATUS_OFFSET and
ICSS_LRE_TIMESTAMP_PKTS_STATUS_OFFSET.

However, icssm_prueth_lre_config_packet_timestamping is only called from
icssm_prueth_lre_config, which is gated by prueth_is_lre() (only true for HSR
and PRP).

In Switch mode, these registers appear to never be written, potentially
leaving timestamps disabled in firmware and breaking the wire-arrival
ordering logic.

[Severity: High]
This is a pre-existing issue, but are the dynamically allocated FDB learning
work items tracked for cleanup?

When learning a new source MAC address in icssm_prueth_sw_learn_fdb()
(drivers/net/ethernet/ti/icssm/icssm_prueth_switch.c), the driver dynamically
allocates a struct icssm_prueth_sw_fdb_work and queues it.

If these are not tracked in a list, unloading the module could leave pending
work items that execute unmapped memory, potentially causing a kernel panic.

[Severity: High]
This is a pre-existing issue, but can the unbounded GFP_ATOMIC allocations
for FDB learning lead to memory exhaustion?

Every received frame with an unlearned source MAC address triggers
icssm_prueth_sw_learn_fdb(), which uses kzalloc_obj(..., GFP_ATOMIC) without
rate limiting.

A continuous flood of frames with randomized source MAC addresses could
trigger thousands of allocations per second, potentially exhausting the
kernel's atomic memory pools.
-- 
This is an AI-generated review.