Re: [PATCH net-next v2 1/3] net: ti: icssm-prueth: Add HSR and PRP HW offload mode support for AM57xx, AM437x and AM335x

[Paolo Abeni <pabeni@redhat.com>]

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/
---
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.

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
[ ... ]
> @@ -564,7 +592,7 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac *emac,
[ ... ]
>         /* 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.

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

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

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

[ ... ]
> @@ -1360,6 +1465,8 @@ static void icssm_emac_ndo_set_rx_mode(struct net_device *ndev)
[ ... ]
>  	/* 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?

[ ... ]
> +static int icssm_prueth_hsr_port_link(struct net_device *ndev,
> +				      struct net_device *hsr_ndev)
> +{
[ ... ]
> +	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.

[ ... ]
> +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)
-- 
This is an AI-generated review.