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

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

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.

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

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

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

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

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.

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

We will address this in the next version.

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