Hi,

From: AI Reviewer <redacted>

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: Support duplicate HW offload feature for HSR and PRP

This commit introduces hardware duplicate offload for HSR and PRP modes. It
implements a common shared queue between the two ports, writing frames once
to OCMC RAM and relying on the firmware to replicate the transmission.

quoted

diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c

b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
index 2ab78a98f8568..cbe666a212c3b 100644

--- a/drivers/net/ethernet/ti/icssm/icssm_prueth.c
+++ b/drivers/net/ethernet/ti/icssm/icssm_prueth.c

@@ -97,6 +125,17 @@ static void icssm_prueth_write_reg(struct prueth *prueth,
 static enum pruss_mem pruss_mem_ids[] = { PRUSS_MEM_DRAM0, PRUSS_MEM_DRAM1,
 					  PRUSS_MEM_SHRD_RAM2 };
 
+struct prp_txopt_rct {
+	__be16 sequence_nr;
+	__be16 lan_id_and_lsdu_size;
+	__be16 prp_suffix;
+};

[Severity: High]
Does this structure definition need the __packed attribute?

This struct consists of three 16-bit fields but lacks the __packed attribute,
so it inherits a 2-byte alignment requirement.  Later in
icssm_prueth_tx_enqueue, a pointer to this structure is derived directly
from skb_tail_pointer.

If an ethernet frame payload has an odd byte length, wouldn't casting and
dereferencing this pointer cause an unaligned 16-bit memory access, resulting
in hardware traps or panics on architectures that do not support it?

[ ... ]

quoted

@@ -549,15 +588,24 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac

*emac,

[ ... ]

quoted

+	other_emac = emac->prueth->emac[(emac->port_id == PRUETH_PORT_MII0) ?
+				PRUETH_PORT_MII1 - 1 : PRUETH_PORT_MII0 - 1];
+
+	if (prueth_is_lre(prueth) && (emac->link || other_emac->link))
+		link_up = true;

[Severity: High]
Could this logical OR cause the shared transmission queue to stall completely
if only one of the slave links goes down?

If one port loses its link, its emac->link is 0, but the other active port
will still evaluate link_up as true. The active port would continue sending
duplicate frames to the shared queue.

[ ... ]

quoted

@@ -605,6 +659,29 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac

*emac,

[ ... ]

quoted

+		if (free_blocks_other_port < free_blocks)
+			free_blocks = free_blocks_other_port;
+	}

[Severity: High]
Following up on the link_up logic above, since the available queue space is
bounded by the slowest reader here, wouldn't the inactive port's read pointer
remain frozen?

This appears to exhaust the free_blocks pool, causing the active port to
eventually spin and return -ENOBUFS permanently, defeating the redundancy.

[ ... ]

quoted

+		} else {
+			/* Read PRP RCT to extract sequence number and LAN ID */
+			struct prp_txopt_rct *rct =
+				(struct prp_txopt_rct *)(skb_tail_pointer(skb) -
+							 ICSSM_LRE_TAG_SIZE);
+
+			if (rct->prp_suffix == htons(ETH_P_PRP)) {

[Severity: High]
Is it safe to directly dereference rct->prp_suffix here?

As mentioned above regarding the struct definition, this might lead to an
unaligned memory access on certain architectures. Perhaps using the
get_unaligned_be16 helper would be safer here, or adding the __packed
attribute to the structure definition.

[ ... ]

quoted

@@ -1341,18 +1516,30 @@ static enum netdev_tx icssm_emac_ndo_start_xmit(struct

sk_buff *skb,

[ ... ]

quoted

+	raw_spin_lock(lock_queue);
+	ret = icssm_prueth_tx_enqueue(emac, skb, qid);
+	raw_spin_unlock(lock_queue);

[Severity: High]
Does this need to use the irqsave variant to protect against cross-device
deadlocks with netpoll?

The lre_host_queue_lock is shared between the two ports. If a hardirq
preempts a CPU while it holds this lock during a transmission, and the
interrupt handler invokes a printk that triggers netconsole over the second
interface, netpoll might bypass the netif_tx_lock.

When the second interface calls into ndo_start_xmit, it would attempt to
acquire the exact same lre_host_queue_lock, causing it to spin forever on
the same CPU.
--
This is an AI-generated review.

We are in the process of reviewing the comments and will revert with additional
details.

Thanks and Regards,
Parvathi.