Re: [PATCH net-next v4 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,

> 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=1
> ---
> net: ti: icssm-prueth: Add HSR and PRP HW offload mode support for AM57xx,
> AM437x and AM335x
> 
> This commit extends the ICSSM PRUETH driver to support HSR and PRP redundancy
> protocols on compatible TI SoCs. It enables dynamically switching PRU ethernet
> ports from dual-EMAC mode into HSR/PRP slave modes via firmware reconfiguration.
> 
>> 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
> [ ... ]
>> +static int icssm_prueth_hsr_port_link(struct net_device *ndev,
>> +				      struct net_device *hsr_ndev)
>> +{
> [ ... ]
>> +free_hsr:
>> +	prueth->eth_type = prev_mode;
>> +
>> +	spin_lock_irqsave(&prueth->addr_lock, flags);
>> +
>> +	prueth->hsr_dev = NULL;
>> +	prueth->hsr_members &= ~all_slaves;
> 
> [Severity: High]
> Can this rollback improperly destroy the state for the already-linked first
> port alongside the failing second port?
> 
> If the second port fails to transition to HSR/PRP mode, the free_hsr error
> path clears the hsr_members mask for all ports by applying ~all_slaves.
> 
> Because the first port was successfully enrolled into the HSR bridge in a
> previous invocation, it remains an active slave in the networking stack's
> view. By wiping its membership state here, the driver drastically
> desynchronizes its internal state from the kernel.
> 
> Would it be safer to only clear the bit for the specific port that is
> failing to link?
> 

We will address this in the next version.

Thanks and Regards,
Parvathi.