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

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

Thanks and Regards,
Parvathi.