Message-ID: <20210127173044.65de6aba@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 27 Jan 2021 17:30:44 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v7 net-next 08/11] net: dsa: allow changing the tag
 protocol via the "tagging" device attribute

On Tue, 26 Jan 2021 00:03:30 +0200 Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> Currently DSA exposes the following sysfs:
> $ cat /sys/class/net/eno2/dsa/tagging
> ocelot
> 
> which is a read-only device attribute, introduced in the kernel as
> commit 98cdb4807123 ("net: dsa: Expose tagging protocol to user-space"),
> and used by libpcap since its commit 993db3800d7d ("Add support for DSA
> link-layer types").
> 
> It would be nice if we could extend this device attribute by making it
> writable:
> $ echo ocelot-8021q > /sys/class/net/eno2/dsa/tagging
> 
> This is useful with DSA switches that can make use of more than one
> tagging protocol. It may be useful in dsa_loop in the future too, to
> perform offline testing of various taggers, or for changing between dsa
> and edsa on Marvell switches, if that is desirable.
> 
> In terms of implementation, drivers can now move their tagging protocol
> configuration outside of .setup/.teardown, and into .set_tag_protocol
> and .del_tag_protocol. The calling order is:
> 
> .setup -> [.set_tag_protocol -> .del_tag_protocol]+ -> .teardown
> 
> There was one more contract between the DSA framework and drivers, which
> is that if a CPU port needs to account for the tagger overhead in its
> MTU configuration, it must do that privately. Which means it needs the
> information about what tagger it uses before we call its MTU
> configuration function. That promise is still held.
> 
> Writing to the tagging sysfs will first tear down the tagging protocol
> for all switches in the tree attached to that DSA master, then will
> attempt setup with the new tagger.
> 
> Writing will fail quickly with -EOPNOTSUPP for drivers that don't
> support .set_tag_protocol, since that is checked during the deletion
> phase. It is assumed that all switches within the same DSA tree use the
> same driver, and therefore either all have .set_tag_protocol implemented,
> or none do.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> Reviewed-by: Florian Fainelli <f.fainelli@...il.com>

> +const struct dsa_device_ops *dsa_find_tagger_by_name(const char *buf)
> +{
> +	const struct dsa_device_ops *ops = NULL;
> +	struct dsa_tag_driver *dsa_tag_driver;
> +
> +	mutex_lock(&dsa_tag_drivers_lock);
> +	list_for_each_entry(dsa_tag_driver, &dsa_tag_drivers_list, list) {
> +		const struct dsa_device_ops *tmp = dsa_tag_driver->ops;
> +
> +		if (!sysfs_streq(buf, tmp->name))
> +			continue;
> +
> +		ops = tmp;
> +		break;
> +	}
> +	mutex_unlock(&dsa_tag_drivers_lock);

What's protecting from the tag driver unloading at this very moment?

Some nit picks below if you need to respin.

> +	return ops;
> +}
> +

> +/* Since the dsa/tagging sysfs device attribute is per master, the assumption
> + * is that all DSA switches within a tree share the same tagger, otherwise
> + * they would have formed disjoint trees (different "dsa,member" values).
> + */
> +int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
> +			      struct net_device *master,
> +			      const struct dsa_device_ops *tag_ops,
> +			      const struct dsa_device_ops *old_tag_ops)
> +{
> +	struct dsa_notifier_tag_proto_info info;
> +	struct dsa_port *dp;
> +	int err = -EBUSY;
> +
> +	if (!rtnl_trylock())
> +		return restart_syscall();
> +
> +	/* At the moment we don't allow changing the tag protocol under
> +	 * traffic. The rtnl_mutex also happens to serialize concurrent
> +	 * attempts to change the tagging protocol. If we ever lift the IFF_UP
> +	 * restriction, there needs to be another mutex which serializes this.
> +	 */
> +	if (master->flags & IFF_UP)
> +		goto out_unlock;
> +
> +	list_for_each_entry(dp, &dst->ports, list) {
> +		if (!dsa_is_user_port(dp->ds, dp->index))
> +			continue;
> +
> +		if (dp->slave->flags & IFF_UP)
> +			goto out_unlock;
> +	}
> +
> +	info.tag_ops = old_tag_ops;
> +	err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_DEL, &info);
> +	if (err)
> +		goto out_unlock;
> +
> +	info.tag_ops = tag_ops;
> +	err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_SET, &info);

Not sure I should bother you about this or not, but it looks like
Ocelot does allocations on SET, so there is a chance of not being 
able to return to the previous config, leaving things broken.

There's quite a few examples where we use REPLACE instead of set, 
so that a careful driver can prep its resources before it kills 
the previous config. Although that's not perfect either because 
we'd rather have as much of that logic in the core as possible.

What are your thoughts?

> +	if (err)
> +		goto out_unwind_tagger;
> +
> +	dst->tag_ops = tag_ops;
> +
> +	rtnl_unlock();
> +
> +	return 0;
> +
> +out_unwind_tagger:
> +	info.tag_ops = old_tag_ops;
> +	dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_SET, &info);
> +out_unlock:
> +	rtnl_unlock();
> +	return err;
> +}

> +static bool dsa_switch_tag_proto_match(struct dsa_switch *ds, int port,
> +				       struct dsa_notifier_tag_proto_info *info)
> +{
> +	if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> +		return true;
> +
> +	return false;

return dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ?

> +}
> +
> +static int dsa_switch_tag_proto_del(struct dsa_switch *ds,
> +				    struct dsa_notifier_tag_proto_info *info)
> +{
> +	int port;
> +
> +	/* Check early if we can replace it, so we don't delete it
> +	 * for nothing and leave the switch dangling.
> +	 */
> +	if (!ds->ops->set_tag_protocol)
> +		return -EOPNOTSUPP;
> +
> +	/* The delete method is optional, just the setter is mandatory */
> +	if (!ds->ops->del_tag_protocol)
> +		return 0;
> +
> +	ASSERT_RTNL();
> +
> +	for (port = 0; port < ds->num_ports; port++) {
> +		if (dsa_switch_tag_proto_match(ds, port, info)) {
> +			ds->ops->del_tag_protocol(ds, port,
> +						  info->tag_ops->proto);

invert condition, save indentation

> +		}
> +	}
> +
> +	return 0;
> +}

Powered by blists - more mailing lists