Message-ID: <20230905154744.648c1a8b@wsk>
Date:   Tue, 5 Sep 2023 15:47:44 +0200
From:   Lukasz Majewski <lukma@...x.de>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Eric Dumazet <edumazet@...gle.com>, Andrew Lunn <andrew@...n.ch>,
        davem@...emloft.net, Paolo Abeni <pabeni@...hat.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Tristram.Ha@...rochip.com, Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, UNGLinuxDriver@...rochip.com,
        George McCollister <george.mccollister@...il.com>,
        Oleksij Rempel <o.rempel@...gutronix.de>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR
 ksz_hsr_{join|leave} functions

Hi Vladimir,

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

Ok.

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

Ok. I've misuderstood your suggestion. You were asking about having one
hsr offloaded setup (hsr0 => lan1 + lan2) and in the same time
non-offloaded setup (hsr1 => lan3 + lan4).

> 
> > 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?

In the ksz_jsr_join() at ksz_common.c -> we exit from this function
when !partner.

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

Yes. I do get your point.

> 
> > > 
> > > 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;
> 	}
> 

Ok.

> 	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?
> 

I used two boards. I've used the same kernel with and without HSR
offloading patches.

Cables were connected to port1 and port2 on each board. Non HSR network
was connected to port3.

> (a) kernel did not contain the offloading patch set
> 

Yes.

> (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
> 

I did not setup two hsr devices on the same board. I've used two boards
with hsr0 setup on each.

> (d) in some other way (please detail)
> 
> I was specifically asking about situation (b).

I did not tested the hsr0, hsr1 as my (real) use case is with KSZ9477
having only 3 ports available for connection (port 1,2,3).


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists