Re: [RFC PATCH] net: dsa: microchip: Prevent overriding of HSR port forwarding

["Łukasz Majewski" <lukma@nabladev.com>]

Hi Frieder,

> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> The KSZ9477 supports NETIF_F_HW_HSR_FWD to forward packets between
> HSR ports. This is set up when creating the HSR interface via
> ksz9477_hsr_join() and ksz9477_cfg_port_member().
> 
> At the same time ksz_update_port_member() is called on every
> state change of a port and reconfiguring the forwarding to the
> default state which means packets get only forwarded to the CPU
> port.
> 
> If the ports are brought up before setting up the HSR interface
> and then the port state is not changed afterwards, everything works
> as intended:
> 
>   ip link set lan1 up
>   ip link set lan2 up
>   ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision
> 45 version 1 ip addr add dev hsr 10.0.0.10/24
>   ip link set hsr up
> 
> If the port state is changed after creating the HSR interface, this
> results in a non-working HSR setup:
> 
>   ip link add name hsr type hsr slave1 lan1 slave2 lan2 supervision
> 45 version 1 ip addr add dev hsr 10.0.0.10/24
>   ip link set lan1 up
>   ip link set lan2 up
>   ip link set hsr up
> 
> In this state, packets will not get forwarded between the HSR ports
> and communication between HSR nodes that are not direct neighbours in
> the topology fails.
> 
> To avoid this, we prevent all forwarding reconfiguration requests for
> ports that are part of a HSR setup with NETIF_F_HW_HSR_FWD enabled.
> 
> Fixes: 2d61298fdd7b ("net: dsa: microchip: Enable HSR offloading for
> KSZ9477") Signed-off-by: Frieder Schrempf
> <frieder.schrempf@kontron.de> ---
> I'm posting this as RFC as my knowledge of the driver and the stack in
> general is very limited. Please review thoroughly and provide
> feedback. Thanks!

I don't have the HW at hand at the moment (temporary).

Could you check if this patch works when you create two hsr interfaces
- i.e. hsr1 would use HW offloading from KSZ9744 and hsr2 is just the
  one supporting HSR in software.

> ---
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 11 +++++++++++
>  include/net/dsa.h                      | 12 ++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c index
> 7c142c17b3f69..56370ecdfe4ee 100644 ---
> a/drivers/net/dsa/microchip/ksz_common.c +++
> b/drivers/net/dsa/microchip/ksz_common.c @@ -2286,6 +2286,17 @@
> static void ksz_update_port_member(struct ksz_device *dev, int port)
> return; 
>  	dp = dsa_to_port(ds, port);
> +
> +	/*
> +	 * HSR ports might use forwarding configured during setup.
> Prevent any
> +	 * modifications as long as the port is part of a HSR setup
> with
> +	 * NETIF_F_HW_HSR_FWD enabled.
> +	 */
> +	if (dev->hsr_dev && dp->user &&
> +	    (dp->user->features & NETIF_F_HW_HSR_FWD) &&
> +	    dsa_is_hsr_port(ds, dev->hsr_dev, port))
> +		return;
> +
>  	cpu_port = BIT(dsa_upstream_port(ds, port));
>  
>  	for (i = 0; i < ds->num_ports; i++) {
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 55e2d97f247eb..846a2cc2f2fc3 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -565,6 +565,18 @@ static inline bool dsa_is_user_port(struct
> dsa_switch *ds, int p) return dsa_to_port(ds, p)->type ==
> DSA_PORT_TYPE_USER; }
>  
> +static inline bool dsa_is_hsr_port(struct dsa_switch *ds, struct
> net_device *hsr, int p) +{
> +	struct dsa_port *hsr_dp;
> +
> +	dsa_hsr_foreach_port(hsr_dp, ds, hsr) {
> +		if (hsr_dp->index == p)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +

I thought that we already had such function implemented. Apparently I
must have been wrong.

>  #define dsa_tree_for_each_user_port(_dp, _dst) \
>  	list_for_each_entry((_dp), &(_dst)->ports, list) \
>  		if (dsa_port_is_user((_dp)))



-- 
Best regards,

Lukasz Majewski

--
Nabla Software Engineering GmbH
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: office@nabladev.com
Geschftsfhrer : Stefano Babic