On Wed, Dec 08, 2021 at 04:32:43AM +0100, Ansuel Smith wrote:

On Wed, Dec 08, 2021 at 03:09:47AM +0200, Vladimir Oltean wrote:

quoted

On Wed, Dec 08, 2021 at 01:42:59AM +0100, Ansuel Smith wrote:

quoted

On Wed, Dec 08, 2021 at 02:40:51AM +0200, Vladimir Oltean wrote:

quoted

On Wed, Dec 08, 2021 at 02:04:32AM +0200, Vladimir Oltean wrote:

quoted

On Wed, Dec 08, 2021 at 01:47:36AM +0200, Vladimir Oltean wrote:

quoted

quoted

2) is harder. But as far as i know, we have an 1:N setup.  One switch
driver can use N tag drivers. So we need the switch driver to be sure
the tag driver is what it expects. We keep the shared state in the tag
driver, so it always has valid data, but when the switch driver wants
to get a pointer to it, it needs to pass a enum dsa_tag_protocol and
if it does not match, the core should return -EINVAL or similar.

In my proposal, the tagger will allocate the memory from its side of the
->connect() call. So regardless of whether the switch driver side
connects or not, the memory inside dp->priv is there for the tagger to
use. The switch can access it or it can ignore it.

I don't think I actually said something useful here.

The goal would be to minimize use of dp->priv inside the switch driver,
outside of the actual ->connect() / ->disconnect() calls.
For example, in the felix driver which supports two tagging protocol
drivers, I think these two methods would be enough, and they would
replace the current felix_port_setup_tagger_data() and
felix_port_teardown_tagger_data() calls.

An additional benefit would be that in ->connect() and ->disconnect() we
get the actual tagging protocol in use. Currently the felix driver lacks
there, because felix_port_setup_tagger_data() just sets dp->priv up
unconditionally for the ocelot-8021q tagging protocol (luckily the
normal ocelot tagger doesn't need dp->priv).

In sja1105 the story is a bit longer, but I believe that can also be
cleaned up to stay within the confines of ->connect()/->disconnect().

So I guess we just need to be careful and push back against dubious use
during review.

I've started working on a prototype for converting sja1105 to this model.
It should be clearer to me by tomorrow whether there is anything missing
from this proposal.

I'm working on your suggestion and I should be able to post another RFC
this night if all works correctly with my switch.

Ok. The key point with my progress so far is that Andrew may be right
and we might need separate tagger priv pointers per port and per switch.
At least for the cleanliness of implementation. In the end I plan to
remove dp->priv and stay with dp->tagger_priv and ds->tagger_priv.

Here's what I have so far. I have more changes locally, but the rest it
isn't ready and overall also a bit irrelevant for the discussion.
I'm going to sleep now.

BTW, I notice we made the same mistake. Don't know if it was the problem
and you didn't notice... The notifier is not ready at times of the first
tagger setup and the tag_proto_connect is never called.
Anyway sending v2 with your suggestion applied.

I didn't go past the compilation stage yesterday. Anyway, now that you
mention it, I remember Tobias hitting this issue as well when he worked
on changing tagging protocol via device tree, and this is why
dsa_switch_setup_tag_protocol() exists.  I believe that's where we'd
need to call ds->ops->connect_tag_proto from, like this:

static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
{
	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
	struct dsa_switch_tree *dst = ds->dst;
	struct dsa_port *cpu_dp;
	int err;

	if (tag_ops->proto == dst->default_proto)
		goto connect;

	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
		rtnl_lock();
		err = ds->ops->change_tag_protocol(ds, cpu_dp->index,
						   tag_ops->proto);
		rtnl_unlock();
		if (err) {
			dev_err(ds->dev, "Unable to use tag protocol \"%s\": %pe\n",
				tag_ops->name, ERR_PTR(err));
			return err;
		}
	}

connect:
	if (ds->ops->connect_tag_protocol) {
		err = ds->ops->connect_tag_protocol(ds, tag_ops->proto);
		if (err) {
			dev_err(ds->dev,
				"Unable to connect to tag protocol \"%s\": %pe\n",
				tag_ops->name, ERR_PTR(err));
			return err;
		}
	}

	return 0;
}