Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Vladimir Oltean <olteanv@gmail.com>]

On Tue, Sep 05, 2023 at 01:23:51PM +0200, Lukasz Majewski wrote:
> > Should be squashed into patch 3/4. The split does not make the code
> > easier to review for me.
> 
> So you recommend to have only one patch in which the hsr_join{leave}
> function from ksz_common.c and ksz9477_hsr_join{leave} from ksz9477.c
> are added?

Correct. In addition, patch 1/4 will be dropped. So there will be 2
patches, one on net/dsa/tag_ksz.c and the other on drivers/net/dsa/microchip/.

> > I don't see any restriction to allow offloading a single HSR device.
> 
> As I've written in the other response - I've followed the xrs700x.c
> convention. 

"the xrs700x.c convention"

xrs700x_hsr_join():

	/* Only ports 1 and 2 can be HSR/PRP redundant ports. */
	if (port != 1 && port != 2)
		return -EOPNOTSUPP;

So, checking for ports 1 and 2 is a stronger condition than the one I'm
asking you to enforce.

The KSZ9477 is more flexible. It can enable HSR offload on any 2 ports,
not just on ports 1 and 2. But it can still be 2 ports max, no more.
You haven't copied the xrs700x convention, but you haven't adapted it,
either.

> Moreover, for me it seems more natural, that we only allow full HSR
> support for 2 ports or none. Please be aware, that HSR supposed to
> support only 2 ports, and having only one working is not recommended by
> vendor.

And where is it enforced that full HSR offload is only applied to 2
ports or none?

> > Looking at patch 3/4, that will obviously not work due to some
> > hardware registers which are global and would be overwritten by the
> > second HSR device.
> 
> I cannot guarantee that there will not be any "side effects" with this
> approach. And to be honest - I would prefer to spent time on testing
> recommended setups.

Please read my reply again, keeping in mind that by "HSR device" I mean
"hsr0" in the commands below, and not the member ports as you've assumed
when responding.

> > 
> > For example, a5psw_port_bridge_join() has a similar restriction to
> > offload a single bridge device.
> 
> HSR is IMHO a bit different than plain "bridge" offloading.

Maybe this was not clear, but by "similar" I mean: if you replace the
"bridge" word with "hsr", and you copy that code snippet from a5psw,
you get the desired outcome.

static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr,
			/* optionally pass the extack argument from the caller */)
{
	struct ksz_device *dev = ds->priv;

	/* We only support 1 HSR device */
	if (dev->hsr_dev && hsr != dev->hsr_dev) {
		NL_SET_ERR_MSG_MOD(extack,
				   "Offload supported for a single HSR");
		return -EOPNOTSUPP;
	}

	dev->hsr_dev = hsr;

	...

	return 0;
}

I did not imply that HSR is not different than bridge offloading.
I don't see how that is even related to the discussion.

> > If you return -EOPNOTSUPP, then DSA should fall back to an
> > unoffloaded, 100% software-based HSR device, and that should work
> > too. 
> 
> And then we would have one port with SW HSR and another one with HW
> HSR?

No. One HSR device (hsr0, with 2 member ports) with offload and one
HSR device (hsr1, with 2 member ports) without offload (see (b) below).

> >It would be good if you could verify that the unoffloaded HSR
> > works well after the changes too.
> 
> I've tested on KSZ9477-EVB the SW HSR operation with two ports (and two
> or three boards) and HW HSR offloading. Results are presented in the
> cover-letter.

"results in the cover letter"

Results:
With KSZ9477 offloading support added: RX: 100 Mbps TX: 98 Mbps
With no offloading                     RX: 63 Mbps  TX: 63 Mbps

What was the setup for the "no offloading" case?

(a) kernel did not contain the offloading patch set

(b) the testing was on hsr1, in the situation below:

ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1 # offloaded
ip link add name hsr1 type hsr slave1 lan3 slave2 lan4 supervision 45 version 1 # unoffloaded

(d) in some other way (please detail)

I was specifically asking about situation (b).