Re: [PATCH v2 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Jun 19, 2024 at 03:42:48PM +0200, Lukasz Majewski wrote:
> The KSZ9477 allows HSR in-HW offloading for any of two selected ports.
> This patch adds check if one tries to use more than two ports with
> HSR offloading enabled.
> 
> The problem is with RedBox configuration (HSR-SAN) - when configuring:
> ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 interlink lan3 \
> 	supervision 45 version 1
> 
> The lan1 (port0) and lan2 (port1) are correctly configured as ports, which
> can use HSR offloading on ksz9477.
> 
> However, when we do already have two bits set in hsr_ports, we need to
> return (-ENOTSUPP), so the interlink port (lan3) would be used with

EOPNOTSUPP

> SW based HSR RedBox support.
> 
> Otherwise, I do see some strange network behavior, as some HSR frames are
> visible on non-HSR network and vice versa.
> 
> This causes the switch connected to interlink port (lan3) to drop frames
> and no communication is possible.
> 
> Moreover, conceptually - the interlink (i.e. HSR-SAN port - lan3/port2)
> shall be only supported in software as it is also possible to use ksz9477
> with only SW based HSR (i.e. port0/1 -> hsr0 with offloading, port2 ->
> HSR-SAN/interlink, port4/5 -> hsr1 with SW based HSR).
> 
> Fixes: 5055cccfc2d1 ("net: hsr: Provide RedBox support (HSR-SAN)")
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> 
> ---
> Changes for v2:
> - Add more verbose description with Fixes: tag
> - Check the condition earlier and remove extra check if SoC is ksz9477
> - Add comment in the source code file
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 2818e24e2a51..72bb419e34b0 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -3906,6 +3906,13 @@ static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	/* KSZ9477 can only perform HSR offloading for up to two ports */
> +	if (hweight8(dev->hsr_ports) >= 2) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Cannot offload more than two ports - use software HSR");
> +		return -EOPNOTSUPP;
> +	}
> +
>  	/* Self MAC address filtering, to avoid frames traversing
>  	 * the HSR ring more than once.
>  	 */
> -- 
> 2.20.1
> 

How do you know you are rejecting the offloading of the interlink port,
and not of one of the ring ports? Basing this off of calling order is
fragile, and does not actually reflect the hardware limitation.