Hi,

From: AI Reviewer <redacted>

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/
---
Subject: net: ti: icssm-prueth: Add HSR and PRP HW offload mode support for
AM57xx, AM437x and AM335x

This commit introduces hardware offload support for the HSR and PRP redundancy
protocols on TI PRU-ICSS Ethernet MACs. It enables firmware-based frame
duplicate detection and L2 forwarding, treating PRU Ethernet ports as Link
Redundancy Entity (LRE) slaves. The driver can now dynamically switch between
dual-EMAC, switch, and HSR/PRP modes.

quoted

diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c

b/drivers/net/ethernet/ti/icssm/icssm_prueth.c

[ ... ]

quoted

@@ -564,7 +592,7 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac *emac,

[ ... ]

quoted

        /* update first buffer descriptor */
 	wr_buf_desc = (pktlen << PRUETH_BD_LENGTH_SHIFT) &
 		       PRUETH_BD_LENGTH_MASK;
+	if (PRUETH_IS_HSR(prueth))
+		wr_buf_desc |= BIT(PRUETH_BD_HSR_FRAME_SHIFT);

[Severity: High]
Does padding small packets with eth_skb_pad() in icssm_emac_ndo_start_xmit()
interfere with PRP trailer detection here?

When a short PRP frame is padded to 60 bytes in icssm_emac_ndo_start_xmit(),
the zero-padding is appended after the RCT. Later, in
icssm_prueth_tx_enqueue(), the driver attempts to read the RCT like this:

   struct prp_txopt_rct *rct =
       (struct prp_txopt_rct *)(skb_tail_pointer(skb) - ICSSM_LRE_TAG_SIZE);

Won't this calculation point into the trailing zero-padding instead of the
actual RCT? This would cause small PRP frames to bypass redundancy
duplication.

No, eth_skb_pad() doesn't interfere with PRP trailer detection.

The kernel's HSR/PRP layer always pads a short frame to 60 bytes first, and only
then appends the 6-byte RCT trailer on top. So the trailer is always the last 6
bytes, and by the time this driver sees the frame it's already past 60 bytes.
This padding call never fires and the RCT read is always correct. Not an issue.

[ ... ]

quoted

@@ -912,6 +963,25 @@ static int icssm_emac_request_irqs(struct prueth_emac

*emac)
 	return ret;
 }
 
+static int icssm_emac_sanitize_feature_flags(struct prueth_emac *emac)
+{
+	netdev_features_t request_lre;
+
+	request_lre = emac->ndev->features & NETIF_PRUETH_LRE_OFFLOAD_FEATURES;
+
+	if (prueth_is_lre(emac->prueth) && !request_lre) {
+		netdev_err(emac->ndev, "Error: Turn ON HSR offload\n");
+		return -EINVAL;
+	}

[Severity: Medium]
Is this check sufficient to handle partial feature toggling?

If a user disables NETIF_F_HW_HSR_TAG_RM via ethtool while leaving
NETIF_F_HW_HSR_FWD active, request_lre will still evaluate to non-zero and
bypass this validation. Since the firmware unconditionally strips the tag,
could this mismatched configuration cause the software stack to misinterpret
the inner IP header as an HSR tag?

We will address this in the next version.

[ ... ]

quoted

@@ -1079,6 +1162,9 @@ static int icssm_emac_ndo_stop(struct net_device *ndev)
 	else
 		rproc_shutdown(emac->pru);
 
+	if (prueth_is_lre(prueth))
+		icssm_prueth_lre_cleanup(prueth);

[Severity: High]
Could this unconditionally cancel the global HSR/PRP duplicate table check
timer while the other LRE port is still running?

If eth0 is brought down, the timer is canceled here, but if eth1 remains
active, the duplication tables could silently fill up and drop legitimate
frames because the check timer is never restarted.

We will address this in the next version.

[ ... ]

quoted

@@ -1122,7 +1208,8 @@ static int icssm_prueth_change_mode(struct prueth *prueth,
 		}
 	}
 
-	if (mode == PRUSS_ETHTYPE_EMAC || mode == PRUSS_ETHTYPE_SWITCH) {
+	if (mode == PRUSS_ETHTYPE_EMAC || mode == PRUSS_ETHTYPE_SWITCH ||
+	    mode == PRUSS_ETHTYPE_HSR || mode == PRUSS_ETHTYPE_PRP) {

[Severity: High]
This is a pre-existing issue, but can the direct invocation of ndo_open() and
ndo_stop() inside icssm_prueth_change_mode() cause a deadlock?

If ndev->netdev_ops->ndo_open(ndev) fails and returns an error, the function
aborts without restoring the NAPI state. Later, when the interface is brought
down, the kernel naturally invokes ndo_stop() again. A second napi_disable()
call on an already disabled NAPI might trigger an infinite sleep waiting for
NAPI_STATE_SCHED.

Yes, this is causing a deadlock. icssm_prueth_change_mode() already existed before
this series and this patch series only adds two more mode values to an existing check,
it doesn't touch the open/stop logic itself. We will fix it as its own separate
patch later, instead of mixing it into this HSR/PRP series.

[ ... ]

quoted

@@ -1360,6 +1465,8 @@ static void icssm_emac_ndo_set_rx_mode(struct net_device

*ndev)

[ ... ]

quoted

 	/* It is a shared table. So lock the access */
-	spin_lock_irqsave(&emac->addr_lock, flags);
+	spin_lock_irqsave(mc_filter_tbl_lock, flags);
 
 	/* Disable and reset multicast filter, allows allmulti */
 	icssm_emac_mc_filter_ctrl(emac, false);

[Severity: High]
Does this function corrupt the shared multicast hardware filter table in LRE
mode?

By calling icssm_emac_mc_filter_reset() the shared table in DRAM1 is cleared.
The code then repopulates the table by iterating only over the current
net_device's multicast addresses. Could this silently drop multicast traffic
for the other active slave port since its addresses are wiped out?

No, this is safe in LRE mode. Although icssm_emac_mc_filter_reset() clears the
shared multicast filter table in DRAM1, it is rebuilt right away from the current
net_device's multicast list. 

The HSR/PRP layer keeps the multicast lists of both slave ports in sync by copying
the master's list whenever it changes, so both ports always have the same multicast
addresses. That means rebuilding the shared table from either port's list restores
all the required entries and does not drop multicast addresses needed by the other port.

[ ... ]

quoted

+static int icssm_prueth_hsr_port_link(struct net_device *ndev,
+				      struct net_device *hsr_ndev)
+{

[ ... ]

quoted

+	spin_lock_irqsave(&emac->addr_lock, flags);
+
+	if (!prueth->hsr_members) {

[Severity: Low]
Is the per-port emac->addr_lock the correct lock to use when modifying the
device-wide shared state in prueth->hsr_members and prueth->hsr_dev?

While the networking core's global rtnl_lock might serialize this path during
NETDEV_CHANGEUPPER, using a local instance lock to protect global state could
be a maintainability hazard.

We will address this in the next version.

[ ... ]

quoted

+free_hsr:
+	spin_lock_irqsave(&emac->addr_lock, flags);
+
+	prueth->hsr_dev = NULL;
+	prueth->hsr_members &= ~BIT(emac->port_id);
+
+	spin_unlock_irqrestore(&emac->addr_lock, flags);
+	return ret;
+}

[Severity: High]
If adding the second HSR/PRP port fails (e.g., during
icssm_prueth_change_mode), does this error path corrupt the bridge state?

Unconditionally clearing the shared prueth->hsr_dev pointer while the first
port is still present in prueth->hsr_members appears to permanently break
subsequent link attempts for the second port, because future links will fail
on the earlier condition:

   if (prueth->hsr_dev != hsr_ndev)
--

We will address this in the next version.

Thanks and Regards,
Parvathi.