Messages in this thread | | | Date | Wed, 8 Dec 2021 13:54:31 +0200 | From | Vladimir Oltean <> | Subject | Re: [net-next RFC PATCH 0/6] Add support for qca8k mdio rw in Ethernet packet |
| |
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: > > On Wed, Dec 08, 2021 at 01:42:59AM +0100, Ansuel Smith wrote: > > > On Wed, Dec 08, 2021 at 02:40:51AM +0200, Vladimir Oltean wrote: > > > > On Wed, Dec 08, 2021 at 02:04:32AM +0200, Vladimir Oltean wrote: > > > > > On Wed, Dec 08, 2021 at 01:47:36AM +0200, Vladimir Oltean wrote: > > > > > > > 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; }
| |