Re: [PATCH net-next v2 3/3] net: ti: icssm-prueth: Support duplicate HW offload feature for HSR and PRP

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

> 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?

[ ... ]
> @@ -549,15 +588,24 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac *emac,
[ ... ]
> +	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.

[ ... ]
> @@ -605,6 +659,29 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac *emac,
[ ... ]
> +		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.

[ ... ]
> +		} 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.

[ ... ]
> @@ -1341,18 +1516,30 @@ static enum netdev_tx icssm_emac_ndo_start_xmit(struct sk_buff *skb,
[ ... ]
> +	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.