Re: [PATCH net-next v3 4/4] net: dsa: xrs700x: add HSR offloading support

[Vladimir Oltean <olteanv@gmail.com>]

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@gmail.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@gmail.com>