netdev - 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: <20240620090210.drop6jwh7e5qw556@skbuf>
Date: Thu, 20 Jun 2024 12:02:10 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Lukasz Majewski <lukma@...x.de>
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

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

> > 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,
but IMO misidentifies the problem as having to do with the number of
ports rather than the port type. 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).