linux-kernel - Re: [PATCH v2 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]

Message-ID: <20240620140044.07191e24@wsk>
Date: Thu, 20 Jun 2024 14:00:44 +0200
From: Lukasz Majewski <lukma@...x.de>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org, Paolo Abeni
 <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>, "David S. Miller"
 <davem@...emloft.net>, Oleksij Rempel <o.rempel@...gutronix.de>,
 Tristram.Ha@...rochip.com, Sebastian Andrzej Siewior
 <bigeasy@...utronix.de>, Simon Horman <horms@...nel.org>, Dan Carpenter
 <dan.carpenter@...aro.org>, "Ricardo B. Marliere" <ricardo@...liere.net>,
 Casper Andersson <casper.casan@...il.com>, linux-kernel@...r.kernel.org,
 Woojung Huh <woojung.huh@...rochip.com>, UNGLinuxDriver@...rochip.com,
 Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v2 net-next] net: dsa: Allow only up to two HSR HW
 offloaded ports for KSZ9477

Hi Vladimir,

> On Thu, Jun 20, 2024 at 09:59:20AM +0200, Lukasz Majewski wrote:
> > > It will return -EOPNOTSUPP for port 0,   
> > 
> > This comment is for xrs700x_hsr_join()?  
> 
> Yes.
> 
> > For the ksz_hsr_join() we do explicitly check for the
> > KSZ9477_CHIP_ID.
> > 
> > I do regard this fix as a ksz9477 specific one, as there are some
> > issues (IMHO - this is the "unexpected behaviour" case for this IC)
> > when we add interlink to SoC VLAN.
> > 
> > I don't understand why you bring up xrs700x case here? Is it to get
> > a "broader context"?  
> 
> You have the Fixes: tag set to a HSR driver change, the fix to which
> you provide in an offloading device driver. What I'm trying to tell
> you is to look around and see that KSZ9477 is not the only one which
> is confused by the addition of an interlink port.

As of now - the HSR interlink was tested with hsr_redbox.sh script with
QEMU setup and with KSZ9477 IC with and without offloading enabled.

> So is XRS700X, yet
> for another reason.
> 
> > > falling back to
> > > software mode for the first ring port, then accept offload for
> > > ring ports 1 and 2. But it doesn't match what user space
> > > requested, because port 2 should be interlink...  
> > 
> > Please correct me if I'm wrong, but this seems to not be the case
> > for ksz9477 - as I stated in the other mail - the ordering is
> > correct (I've checked it).  
> 
> I was never claiming it to be about KSZ9477.

Ok.

> 
> > > I think you really should pass the port type down to drivers and
> > > reject offloading interlink ports...  
> > 
> > As stated above - IMHO I do provide a fix for this particular IC
> > (KSZ9477). With xrs700x we do have fixed ports supporting HSR (port
> > 1,2), so there is no other choice. As a result the HSR Interlink
> > would be supporting only SW emulation.  
> 
> But there is another choice, and I think I've already explained it.
> 
>         HSR_PT_SLAVE_A    HSR_PT_SLAVE_B      HSR_PT_INTERLINK
>  ----------------------------------------------------------------
>  user
>  space        0                 1                   2
>  requests
>  ----------------------------------------------------------------
>  XRS700X
>  driver       1                 2                   -
>  understands
> 
> I am bringing this as an argument for the fact that you should pass
> the port type explicitly from HSR to the offload, and use it
> throughout the offloading drivers. The hweight(ports) >= 2 happens to
> work for KSZ9477,

And hence it is added to ksz_hsr_join() function, which for now only
checks if we use this particular IC.

> but IMO misidentifies the problem as having to do
> with the number of ports rather than the port type.

In general I do understand your concerns - however, as I've stated this
patch fixes oddity of the KSZ9477. I can test it with it.

> Because of this,
> a largely similar issue introduced by the same blamed commit but in
> XRS700X is left unaddressed and unidentified (the fixed ports check
> which is already present masks the fact that it's not really about
> the ports, but their type, which must still be checked, otherwise the
> driver has no idea what HSR wants from it).

To keep it short: I do see your point, but I believe that it is out of
the scope for this particular patch.

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