netdev - Re: [RFC PATCH net-next 2/3] net: hsr: add DSA offloading support

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:   Fri, 22 Jan 2021 12:50:13 -0600
From:   George McCollister <george.mccollister@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Jonathan Corbet <corbet@....net>,
        Murali Karicheri <m-karicheri2@...com>, netdev@...r.kernel.org
Subject: Re: [RFC PATCH net-next 2/3] net: hsr: add DSA offloading support

On Fri, Jan 22, 2021 at 11:56 AM Florian Fainelli <f.fainelli@...il.com> wrote:
>
>
>
> On 1/22/2021 7:59 AM, George McCollister wrote:
> > Add support for offloading of HSR/PRP (IEC 62439-3) tag insertion
> > tag removal, duplicate generation and forwarding on DSA switches.
> >
> > Use a new netdev_priv_flag IFF_HSR to indicate that a device is an HSR
> > device so DSA can tell them apart from other devices in
> > dsa_slave_changeupper.
> >
> > Add DSA_NOTIFIER_HSR_JOIN and DSA_NOTIFIER_HSR_LEAVE which trigger calls
> > to .port_hsr_join and .port_hsr_leave in the DSA driver for the switch.
> >
> > The DSA switch driver should then set netdev feature flags for the
> > HSR/PRP operation that it offloads.
> >     NETIF_F_HW_HSR_TAG_INS
> >     NETIF_F_HW_HSR_TAG_RM
> >     NETIF_F_HW_HSR_FWD
> >     NETIF_F_HW_HSR_DUP
> >
> > For HSR, insertion involves the switch adding a 6 byte HSR header after
> > the 14 byte Ethernet header. For PRP it adds a 6 byte trailer.
> >
> > Tag removal involves automatically stripping the HSR/PRP header/trailer
> > in the switch. This is possible when the switch also preforms auto
> > deduplication using the HSR/PRP header/trailer (making it no longer
> > required).
> >
> > Forwarding involves automatically forwarding between redundant ports in
> > an HSR. This is crucial because delay is accumulated as a frame passes
> > through each node in the ring.
> >
> > Duplication involves the switch automatically sending a single frame
> > from the CPU port to both redundant ports. This is required because the
> > inserted HSR/PRP header/trailer must contain the same sequence number
> > on the frames sent out both redundant ports.
> >
> > Signed-off-by: George McCollister <george.mccollister@...il.com>
>
> This is just a high level overview for now, but I would split this into two:
>
> - a patch that adds HSR offload to the existing HSR stack and introduces
> the new netdev_features_t bits to support HSR offload, more on that below
>
> - a patch that does the plumbing between HSR and within the DSA framework

Okay. I think I could just move everything that touches DSA into a
second commit. I kind of pondered this to begin but settled on it
being unnecessary.

>
> > ---
>
> Do you think we can start with a hsr-hw-offload feature and create new
> bits to described how challenged a device may be with HSR offload? Is it
>  reasonable assumption that functional hardware should be able to
> offload all of these functions or none of them?

I'm fine with either way. I see the ksz9477 supports HSR (not sure
which version) and it looks like it doesn't do header insertion or
removal.

I expect the insertion and removal would always come as a pair but I
suppose someone could always do something weird and just do one.
Forwarding and duplication seem like a given for anything claiming
HSR/PRP hardware support.

Knowing this do you think I should go ahead and just change it to
hsr-hw-offload and if someone adds ksz9477 support or whatever later
they can add a new bit?

>
> It may be a good idea to know what the platform that Murali is working
> on or has worked on is capable of doing, too.

Yeah, I copied him but it bounced as did yours.

>
> [snip]
>
> >
> > +static inline bool netif_is_hsr_master(const struct net_device *dev)
> > +{
> > +     return dev->flags & IFF_MASTER && dev->priv_flags & IFF_HSR;
> > +}
> > +
> > +static inline bool netif_is_hsr_slave(const struct net_device *dev)
> > +{
> > +     return dev->flags & IFF_SLAVE && dev->priv_flags & IFF_HSR;
> > +}
>
> I believe the kernel community is now trying to eliminate the use of the
> terms master and slave, can you replace these with some HSR specific
> naming maybe?

Understood however these both already exist and are referenced in ~40
places. Are you saying I should create different macros that wrap them
or change everywhere they are used?

> --
> Florian