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

[Lukasz Majewski]

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@xxxxxxx

Attachment: pgpN7usxroxR8.pgp
Description: OpenPGP digital signature