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

[Parvathi Pudi <parvathi@couthit.com>]

Hi,

> 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/#/patchset/20260921134343.200426-4-parvathi@couthit.com?part=3
> ---
> net: ti: icssm-prueth: Support duplicate HW offload feature for HSR and PRP
> 
> This commit introduces hardware duplication offloading for HSR and PRP in
> the PRU Ethernet driver. It uses a shared queue to write frames into OCMC
> RAM once, replicating transmission in firmware, and adds receive-side
> handling for duplicate frames.
> 
>> 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
> [ ... ]
>> @@ -656,6 +725,57 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac
>> *emac,
>>  	if (PRUETH_IS_HSR(prueth))
>>  		wr_buf_desc |= BIT(PRUETH_BD_HSR_FRAME_SHIFT);
>>  
>> +	if (prueth_is_lre(prueth)) {
>> +		ethhdr = (struct ethhdr *)skb_mac_header(skb);
>> +		proto = ethhdr->h_proto;
>> +
>> +		if (proto == htons(ETH_P_8021Q)) {
>> +			vlan_hdr = (struct vlan_ethhdr *)ethhdr;
>> +			proto = vlan_hdr->h_vlan_encapsulated_proto;
>> +			is_vlan = true;
>> +		}
>> +
>> +		/* Check if the SKB has HSR tag */
>> +		if (PRUETH_IS_HSR(prueth) && proto == htons(ETH_P_HSR)) {
>> +			hdr = skb_mac_header(skb);
>> +
>> +			if (is_vlan) {
>> +				hsr_ethhdr =
>> +					(struct hsr_txopt_ethhdr *)(hdr +
>> +								    VLAN_HLEN);
>> +			} else {
>> +				hsr_ethhdr = (struct hsr_txopt_ethhdr *)hdr;
>> +			}
>> +
>> +			/* PTP frames (ETH_P_1588) carry no LAN ID
>> +			 * in the HSR tag
>> +			 */
>> +			if (hsr_ethhdr->hsr_tag.encap_proto !=
>> +			    htons(ETH_P_1588)) {
>> +				wr_buf_desc |= PRUETH_BD_LAN_INFO_MASK;
> 
> [Severity: Medium]
> Is it expected to unconditionally duplicate the frame here?
> 
> The driver exposes NETIF_F_HW_HSR_DUP as a toggleable ethtool feature, but
> this data path checks PRUETH_IS_HSR() and always performs duplication. If a
> user disables hw-hsr-dup, the software HSR core will duplicate packets and
> send two copies. The hardware will then duplicate them again, causing a
> broadcast storm.
> 
> Should this check the active ndev features instead?
> 

No. The HSR offload features can only be turned on or off together, and they cannot
be changed while the port is an HSR slave or running. The firmware enters HSR mode
only if these features are enabled when both ports join the HSR device. Otherwise the
device stays in EMAC mode and this path is never taken. So PRUETH_IS_HSR() here always
means hw-hsr-dup is enabled, and frames are never duplicated twice.

>> +			} else {
>> +				wr_buf_desc |= (txport <<
>> +						PRUETH_BD_LAN_A_SHIFT);
>> +			}
>> +			wr_buf_desc |= PRUETH_BD_RED_PKT_MASK;
>> +		} else if (PRUETH_IS_PRP(prueth)) {
>> +			/* Check if the SKB has PRP tag */
>> +			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: Medium]
> Could this lead to unaligned memory accesses?
> 
> Since skb->len can be odd for arbitrary payloads like ICMP packets, the
> tail pointer may be unaligned. The struct prp_txopt_rct contains __be16
> fields and is not marked as __packed. Dereferencing rct->prp_suffix
> directly from an unaligned address can cause unaligned access exceptions on
> some ARM architectures.
> 

We will address this in the next version.

>> +				wr_buf_desc |= PRUETH_BD_LAN_INFO_MASK;
>> +				wr_buf_desc |= PRUETH_BD_RED_PKT_MASK;
>> +			} else {
>> +				wr_buf_desc |= (txport <<
>> +						PRUETH_BD_LAN_A_SHIFT);
>> +			}
>> +		} else {
>> +			wr_buf_desc |= (txport << PRUETH_BD_LAN_A_SHIFT);
>> +		}
>> +	}
> [ ... ]
>> @@ -741,11 +871,36 @@ int icssm_emac_rx_packet(struct prueth_emac *emac, u16
>> *bd_rd_ptr,
> [ ... ]
>> +	actual_pkt_len = pkt_info->length;
>> +
>> +	if (has_hsr_tag)
>> +		actual_pkt_len -= ICSSM_LRE_TAG_SIZE;
> 
> [Severity: Medium]
> Does this unconditionally strip the HSR tag regardless of the netdev feature
> configuration?
> 
> Similar to the duplication offload, NETIF_F_HW_HSR_TAG_RM is exposed via
> ethtool but not checked in this data path. If a user disables tag removal in
> hardware, the host stack might fail to parse the packet since the driver
> still forcibly strips the tag.
> 

No. The tag is stripped only when the device is in HSR mode, and HSR mode is entered
only when both ports join the HSR device with the HSR offload features enabled. These
features can only be turned on or off together, and cannot be changed while the port
is an HSR slave or running. If they are disabled, the device stays in EMAC mode, this
path is not taken, and the HSR core removes the tag in software.

>>  
>>  	/* Allocate a socket buffer for this packet */
>>  	skb = netdev_alloc_skb_ip_align(ndev, actual_pkt_len);
> [ ... ]
>> @@ -1309,17 +1473,31 @@ static enum netdev_tx icssm_emac_ndo_start_xmit(struct
>> sk_buff *skb,
>>  						struct net_device *ndev)
>>  {
>>  	struct prueth_emac *emac = netdev_priv(ndev);
>> +	/* Spinlock for Tx Queues */
>> +	spinlock_t *lock_queue;
>> +	unsigned long flags;
>>  	int ret;
>>  	u16 qid;
>>  
>>  	qid = icssm_prueth_get_tx_queue_id(emac->prueth, skb);
>> +	/* Select the TX queue spin lock for this queue ID */
>> +	if (prueth_is_lre(emac->prueth))
>> +		lock_queue = &emac->prueth->lre_host_queue_lock[qid - 2];
>> +	else
>> +		lock_queue = &emac->host_queue_lock[qid - 2];
>> +
> 
> [Severity: High]
> Could a race condition occur here when the interface mode changes?
> 
> Because icssm_prueth_change_mode can modify the eth_type (which changes the
> result of prueth_is_lre) and calls ndo_stop directly without waiting for
> in-flight ndo_start_xmit calls to complete, there is a time-of-check to
> time-of-use vulnerability.
> 
> If eth_type changes between this lock selection and the lock acquisition
> below, one thread could acquire host_queue_lock while another acquires
> lre_host_queue_lock. Both might then evaluate prueth_is_lre() as true inside
> icssm_prueth_tx_enqueue, concurrently modifying the exact same shared LRE
> queue descriptors and causing queue corruption.
> 

We will address this in the next version.

Thanks and Regards,
Parvathi.