netdev - Re: [PATCH net-next v3 4/4] net: dsa: xrs700x: add HSR 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]

Message-ID: <20210210094954.7fi4bhh6dboa6s5i@skbuf>
Date:   Wed, 10 Feb 2021 11:49:54 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     George McCollister <george.mccollister@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Jonathan Corbet <corbet@....net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 4/4] net: dsa: xrs700x: add HSR offloading
 support

On Tue, Feb 09, 2021 at 08:11:15PM -0800, Florian Fainelli wrote:
> 
> 
> On 2/9/2021 5:02 PM, George McCollister wrote:
> > Add offloading for HSR/PRP (IEC 62439-3) tag insertion, tag removal
> > forwarding and duplication supported by the xrs7000 series switches.
> > 
> > Only HSR v1 and PRP v1 are supported by the xrs7000 series switches (HSR
> > v0 is not).
> > 
> > Signed-off-by: George McCollister <george.mccollister@...il.com>
> > ---
> [snip]
> > +	val &= ~BIT(dsa_upstream_port(ds, port));
> > +	regmap_write(priv->regmap, XRS_PORT_FWD_MASK(partner->index), val);
> > +	regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
> > +
> > +	regmap_fields_write(priv->ps_forward, partner->index,
> > +			    XRS_PORT_FORWARDING);
> > +	regmap_fields_write(priv->ps_forward, port, XRS_PORT_FORWARDING);
> > +
> > +	hsr_pair[0] = port;
> > +	hsr_pair[1] = partner->index;
> > +	for (i = 0; i < ARRAY_SIZE(hsr_pair); i++) {
> > +		slave = dsa_to_port(ds, hsr_pair[i])->slave;
> > +		slave->features |= XRS7000X_SUPPORTED_HSR_FEATURES;
> 
> It's a bit weird to change the supported features while joining, usually
> you set them ahead of time to indicate what you are capable of doing and
> those can get toggled by user-space to enable/disable said feature, I
> suppose the goal here is to influence the HSR data path's decisions to
> insert or not tags so this may be okay. This does beg several questions:
> 
> - should slave->vlan_features also include that feature set somehow (can
> I have a VLAN upper?)

hsr_check_dev_ok:
	if (is_vlan_dev(dev)) {
		NL_SET_ERR_MSG_MOD(extack, "HSR on top of VLAN is not yet supported in this driver.");
		return -EINVAL;
	}

> - should there be a notifier running to advertise NETDEV_FEAT_CHANGE?

I felt it's a bit weird too to toggle the netdev flags just like that
instead of just enabling them at probe time or something (or have DSA
set them in dsa_slave_create(), just as it currently checks
ds->ops->port_vlan_add), but since there's no need for anyone to process
that notification, and there don't appear to be any strict guidelines, I
didn't say anything. I guess the current code is just fine for what is
needed at the moment.

Reviewed-by: Vladimir Oltean <olteanv@...il.com>