Re: [PATCH] Revert "arm64: dts: ls1028a: sl28: use ocelot-8021q tagging by default"

[Michael Walle]

Am 2022-10-27 20:04, schrieb Vladimir Oltean:
> On Thu, Oct 27, 2022 at 06:00:04PM +0200, Michael Walle wrote:
> > > - change the MODULE_ALIAS() of all tagging protocol driver modules from
> > >   "dsa_tag-<number" to something containing their string name - what you
> > >   proposed. I don't know why the current MODULE_ALIAS() is formatted the
> > >   way it is. Maybe Andrew can comment on whether this is feasible.
> > >   I think there isn't any backwards compatibility concern, since only
> > >   modules compiled for a certain kernel version are expected to be
> > >   loaded.
> >
> > FWIW, you can have multiple aliases if we somehow need to keep the 
> > IDs,
> > too.
>
> Yeah, that's worth exploring, but it means that we have 2 code paths 
> for
> request_module() with different string formats. To me this is slightly
> undesirable, we should try to consolidate the mechanisms in the core.
>
> > > - put a translation table between string and MODULE_ALIAS() inside
> > >   dsa_core.ko, which potentially duplicates code. Maybe if we
> > >   auto-generate it somehow?
> >
> > Yeah, I also thought of a table with of name to module alias mapping.
> > But then you'd have two places to keep in sync (of not autogenerated).
>
> Well, to be fair, this is not exactly true. As far as I could find
> (grep for "ops->name" in net/dsa), there are only 3 instances of 
> reading
> the "name" field of struct dsa_device_ops, and none of them are from a
> fast path.
>
> I can imagine a table along the lines of:
>
> static const char * const dsa_tag_proto_names[] = {
> 	[DSA_TAG_PROTO_NONE] = "none",
> 	[DSA_TAG_PROTO_BRCM] = "brcm",
> 	....
> };
>
> which is then used to directly replace ops->name
> (becomes dsa_tag_proto_names[ops->proto]).
>
> Then, we could add a new function "dsa_tag_protocol_name_to_id()" or
> something along those lines, and construct the modalias string based on
> that.
>
> No duplication necessary, since we would remove dsa_device_ops :: name.

If one would a new tagger you'd need to add it to
dsa_tag_proto_names[] as well as adding the tagger source file. Thus,
two places to keep in sync. And you don't have all the information in
one place, e.g. the tagger module. The name of the tagger as used in
sysfs or device tree is then in the core. Just wanted to point that out.
After all it's up to you as the maintainer to decide ;)

-michael