netdev - RE: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 18 Jun 2024 14:56:53 +0000
From: <Woojung.Huh@...rochip.com>
To: <lukma@...x.de>
CC: <dan.carpenter@...aro.org>, <andrew@...n.ch>, <olteanv@...il.com>,
	<kuba@...nel.org>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<edumazet@...gle.com>, <davem@...emloft.net>, <o.rempel@...gutronix.de>,
	<Tristram.Ha@...rochip.com>, <bigeasy@...utronix.de>, <horms@...nel.org>,
	<ricardo@...liere.net>, <casper.casan@...il.com>,
	<linux-kernel@...r.kernel.org>, <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW
 offloaded ports for KSZ9477

Hi Lukasz,

Thanks for detail explanation.

> Hi Dan, Andrew, Woojung
> 
> > Hi Dan & Andrew,
> >
> > > On Tue, Jun 18, 2024 at 03:52:23PM +0200, Andrew Lunn wrote:
> > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > > b/drivers/net/dsa/microchip/ksz_common.c
> > > > > index 2818e24e2a51..181e81af3a78 100644
> > > > > --- a/drivers/net/dsa/microchip/ksz_common.c
> > > > > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > > > > @@ -3906,6 +3906,11 @@ static int ksz_hsr_join(struct
> > > > > dsa_switch *ds,
> > > int port, struct net_device *hsr,
> > > > >             return -EOPNOTSUPP;
> > > > >     }
> > > > >
> > > > > +   if (hweight8(dev->hsr_ports) > 1) {
> > > > > +           NL_SET_ERR_MSG_MOD(extack, "Cannot offload more
> > > > > than two
> > > ports (in use=0x%x)", dev->hsr_ports);
> > > > > +           return -EOPNOTSUPP;
> > > > > +   }
> > > >
> > > > Hi Dan
> > > >
> > > > I don't know HSR to well, but this is offloading to hardware, to
> > > > accelerate what Linux is already doing in software. It should be,
> > > > if the hardware says it cannot do it, software will continue to
> > > > do the job. So the extack message should never be seen.
> > >
> > > Ah.  Okay.  However the rest of the function prints similar messages
> > > and so probably we could remove those error messages as well.  To be
> > > honest, I just wanted something which functioned as a comment and
> > > mentioned "two ports".  Perhaps the condition would be more clear
> > > as
> > > >= 2 instead of > 1?
> > >
> >
> > I'm not a HSR expert and so could be a dummy question.
> >
> > I think this case (upto 2 HSR port offload) is different from other
> > offload error.
> 
> It is not so different.
> 
> In this case when we'd call:
> ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 interlink lan3
> supervision 45 version 1
> 
> lan1 and lan2 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 (HSR-SAN) would be used with
> SW based HSR interlink support.
> 
> Otherwise, I do see some strange behaviour, as some HSR frames are
> visible on HSR-SAN network and vice versa causing switch to drop frames.
> 
> Also conceptually - the interlink (i.e. HSR-SAN port) shall be only SW
> supported 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).

Got the point.
Didn't think separate HSR (port 0/1 & port 4/5).
Thought the case of port 0/1 (offload) + port 4/.. (SW HSR)

> 
> > Others are checking whether offload is possible or
> > not, so SW HSR can kick in when -EOPNOTSUPP returns.
> 
> Yes, this is exactly the case.
> 
> > However, this
> > happens when joining 3rd (2+) port with hardware offload is enabled.
> > It is still working two ports are in HW HSR offload and next ports
> > are in SW HSR?
> 
> As written above, it seems like the in-chip VLAN register is modified
> and some frames are passed between HSR and SAN networks, which is wrong.
> 
> Best would be to have only two ports with HSR offloading enabled and
> then others with SW based HSR if required.
> 
> For me the:
> 
> NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than two ports (in
> use=0x%x)", dev->hsr_ports);
> 
> is fine - as it informs that no more HSR offloading is possible (and
> allows to SW based RedBox/HSR-SAN operation).
> 

Having message looks good to me too.

Woojung