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

[Parvathi Pudi <parvathi@couthit.com>]

Hi,

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

The __packed attribute is not needed as this will be handled by C packing rules. See feedback
from an earlier series: https://lore.kernel.org/all/20250708175301.599c82b8@kernel.org/

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

No. emac->link only reflects the PHY carrier state and does not indicate whether the
firmware servicing the shared queue is running. When one PHY link goes down, the
corresponding firmware continues to consume descriptors and simply discards the frames.
Therefore, using OR here does not cause the shared transmission queue to stall.

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

The inactive port's rd_ptr does not remain frozen just because its PHY link is down.
The firmware continues advancing the read pointer while dropping the frames, so free_blocks
is not exhausted in this scenario. We confirmed this behavior by testing.

Unplugging one link does not stall transmission or lead to a permanent -ENOBUFS. A stall
occurs only if the firmware itself stops running, which is a different failure mode.

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

The __packed attribute is not needed as this will be handled by C packing rules. See feedback
from an earlier series: https://lore.kernel.org/all/20250708175301.599c82b8@kernel.org/

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

We will address this in the next version

Thanks and Regards,
Parvathi.