Re: Circular dependency between DSA switch driver and tagging protocol driver

[Vladimir Oltean]

On Wed, Sep 08, 2021 at 05:49:17PM -0700, Florian Fainelli wrote:
> On 9/8/2021 5:26 PM, Vladimir Oltean wrote:
> > On Wed, Sep 08, 2021 at 03:14:51PM -0700, Florian Fainelli wrote:
> > > > Where is the problem?
> > >
> > > I'd say with 994d2cbb08ca, since the tagger now requires visibility into
> > > sja1105_switch_ops which is not great, to say the least. You could solve
> > > this by:
> > >
> > > - splitting up the sja1150 between a library that contains
> > > sja1105_switch_ops and does not contain the driver registration code
> >
> > I've posted patches which more or less cheat the dependency by creating
> > a third module, as you suggest. The tagging protocol still depends on
> > the main module, now sans the call to dsa_register_switch, that is
> > provided by the third driver, sja1105_probe.ko, which as the name
> > suggests probes the hardware. The sja1105_probe.ko also depends on
> > sja1105.ko, so the insmod order needs to be:
> >
> > insmod sja1105.ko
> > insmod tag_sja1105.ko
> > insmod sja1105_probe.ko
> >
> > I am not really convinced that this change contributes to the overall
> > code organization and structure.
>
> Yes, I don't really like it either, maybe we do need to resolve the other
> dependency created with 566b18c8b752 with a function pointer/indirect call
> that gets resolved at run-time, assuming the overhead is acceptable.

The overhead is acceptable, but maybe I'm not very clear where to put
the function pointer? In struct sja1105_tagger_data I assume?
Also, a function pointer with a single implementation and no intention
of adding a second one is a pretty strange construct, too.

> > > - finding a different way to do a dsa_switch_ops pointer comparison, by
> > > e.g.: maintaining a boolean in dsa_port that tracks whether a particular
> > > driver is backing that port
> >
> > Maybe I just don't see how this would scale. So to clarify, are you
> > suggesting to add a struct dsa_port :: bool is_sja1105, which the
> > sja1105 driver would set to true in sja1105_setup?
>
> Not necessarily something that is sja1105 specific, but something that
> indicates whether the tagger is operating with its intended switch driver,
> or with a "foreign" switch driver (say: dsa_loop for instance).

So that's the other thing. How would we set this "dp->tagger_running_on_foreign_switch_driver" bit?
Which switch/tagging driver pair that gets to run using the mainline code today
will ever cause this to be set?
The patch to make tag_sja1105 safe with dsa_loop was effectively solving
a non-problem from this perspective, since you'd have to modify
dsa_loop_get_protocol to report DSA_TAG_PROTO_SJA1105.
Instead of over-engineering things, how about making dsa_port_is_sja1105
return true? We could set that 'foreign switch driver' bit from the
dsa_loop module itself, when the time comes to make it officially
support multiple tagging protocols and changing between them.