Re: [PATCH net-next v4 3/3] net: ti: icssm-prueth: Support duplicate HW offload feature for HSR and PRP
From: Simon Horman <horms@kernel.org>
Date: 2026-09-25 16:40:06
Also in:
linux-arm-kernel, lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
+ } 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.
+ 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);
+ }
+ }[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
/* Allocate a socket buffer for this packet */ skb = netdev_alloc_skb_ip_align(ndev, actual_pkt_len);
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
+ if (eth_skb_pad(skb)) {
+ if (netif_msg_tx_err(emac) && net_ratelimit())
+ netdev_err(ndev, "packet pad failed\n");
+ atomic64_inc(&emac->stats.tx_dropped);
+ return NETDEV_TX_OK;
+ }
+
+ spin_lock_irqsave(lock_queue, flags);
ret = icssm_prueth_tx_enqueue(emac, skb, qid);
- if (ret) {
- if (ret != -ENOBUFS && netif_msg_tx_err(emac) &&
- net_ratelimit())
- netdev_err(ndev, "packet queue failed: %d\n", ret);
+ spin_unlock_irqrestore(lock_queue, flags);