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

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 3571 bytes --]

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

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

> 
> Thanks.
> Woojung




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@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]