Circular dependency between DSA switch driver and tagging protocol driver

Hi,

Since commits 566b18c8b752 ("net: dsa: sja1105: implement TX
timestamping for SJA1110") and 994d2cbb08ca ("net: dsa: tag_sja1105: be
dsa_loop-safe"), net/dsa/tag_sja1105.ko has gained a build and insmod
time dependency on drivers/net/dsa/sja1105.ko, due to several symbols
exported by the latter and used by the former.

So first one needs to insmod sja1105.ko, then insmod tag_sja1105.ko.

But dsa_port_parse_cpu returns -EPROBE_DEFER when dsa_tag_protocol_get
returns -ENOPROTOOPT. It means, there is no DSA_TAG_PROTO_SJA1105 in the
list of tagging protocols known by DSA, try again later. There is a
runtime dependency for DSA to have the tagging protocol loaded. Combined
with the symbol dependency, this is a de facto circular dependency.

So when we first insmod sja1105.ko, nothing happens, probing is deferred.

Then when we insmod tag_sja1105.ko, we expect the DSA probing to kick
off where it left from, and probe the switch too.

However this does not happen because the deferred probing list in the
device core is reconsidered for a new attempt only if a driver is bound
to a new device. But DSA tagging protocols are drivers with no struct
device.

One can of course manually kick the driver after the two insmods:

echo spi0.1 > /sys/bus/spi/drivers/sja1105/bind

and this works, but automatic module loading based on modaliases will be
broken if both tag_sja1105.ko and sja1105.ko are modules, and sja1105 is
the last device to get a driver bound to it.

Where is the problem?

On 9/8/2021 3:08 PM, Vladimir Oltean wrote:

Hi,

Since commits 566b18c8b752 ("net: dsa: sja1105: implement TX
timestamping for SJA1110") and 994d2cbb08ca ("net: dsa: tag_sja1105: be
dsa_loop-safe"), net/dsa/tag_sja1105.ko has gained a build and insmod
time dependency on drivers/net/dsa/sja1105.ko, due to several symbols
exported by the latter and used by the former.

So first one needs to insmod sja1105.ko, then insmod tag_sja1105.ko.

But dsa_port_parse_cpu returns -EPROBE_DEFER when dsa_tag_protocol_get
returns -ENOPROTOOPT. It means, there is no DSA_TAG_PROTO_SJA1105 in the
list of tagging protocols known by DSA, try again later. There is a
runtime dependency for DSA to have the tagging protocol loaded. Combined
with the symbol dependency, this is a de facto circular dependency.

So when we first insmod sja1105.ko, nothing happens, probing is deferred.

Then when we insmod tag_sja1105.ko, we expect the DSA probing to kick
off where it left from, and probe the switch too.

However this does not happen because the deferred probing list in the
device core is reconsidered for a new attempt only if a driver is bound
to a new device. But DSA tagging protocols are drivers with no struct
device.

One can of course manually kick the driver after the two insmods:

echo spi0.1 > /sys/bus/spi/drivers/sja1105/bind

and this works, but automatic module loading based on modaliases will be
broken if both tag_sja1105.ko and sja1105.ko are modules, and sja1105 is
the last device to get a driver bound to it.

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

- 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
-- 
Florian

On Wed, Sep 08, 2021 at 03:14:51PM -0700, Florian Fainelli wrote:

On 9/8/2021 3:08 PM, Vladimir Oltean wrote:

quoted

Hi,

Since commits 566b18c8b752 ("net: dsa: sja1105: implement TX
timestamping for SJA1110") and 994d2cbb08ca ("net: dsa: tag_sja1105: be
dsa_loop-safe"), net/dsa/tag_sja1105.ko has gained a build and insmod
time dependency on drivers/net/dsa/sja1105.ko, due to several symbols
exported by the latter and used by the former.

So first one needs to insmod sja1105.ko, then insmod tag_sja1105.ko.

But dsa_port_parse_cpu returns -EPROBE_DEFER when dsa_tag_protocol_get
returns -ENOPROTOOPT. It means, there is no DSA_TAG_PROTO_SJA1105 in the
list of tagging protocols known by DSA, try again later. There is a
runtime dependency for DSA to have the tagging protocol loaded. Combined
with the symbol dependency, this is a de facto circular dependency.

So when we first insmod sja1105.ko, nothing happens, probing is deferred.

Then when we insmod tag_sja1105.ko, we expect the DSA probing to kick
off where it left from, and probe the switch too.

However this does not happen because the deferred probing list in the
device core is reconsidered for a new attempt only if a driver is bound
to a new device. But DSA tagging protocols are drivers with no struct
device.

One can of course manually kick the driver after the two insmods:

echo spi0.1 > /sys/bus/spi/drivers/sja1105/bind

and this works, but automatic module loading based on modaliases will be
broken if both tag_sja1105.ko and sja1105.ko are modules, and sja1105 is
the last device to get a driver bound to it.

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

- 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

What about 566b18c8b752 ("net: dsa: sja1105: implement TX timestamping for SJA1110")?
It is essentially the same problem from a symbol usage perspective, plus
the fact that an skb queue belonging to the driver is accessed.

On 9/8/2021 3:19 PM, Vladimir Oltean wrote:

On Wed, Sep 08, 2021 at 03:14:51PM -0700, Florian Fainelli wrote:

quoted

On 9/8/2021 3:08 PM, Vladimir Oltean wrote:

quoted

Hi,

Since commits 566b18c8b752 ("net: dsa: sja1105: implement TX
timestamping for SJA1110") and 994d2cbb08ca ("net: dsa: tag_sja1105: be
dsa_loop-safe"), net/dsa/tag_sja1105.ko has gained a build and insmod
time dependency on drivers/net/dsa/sja1105.ko, due to several symbols
exported by the latter and used by the former.

So first one needs to insmod sja1105.ko, then insmod tag_sja1105.ko.

But dsa_port_parse_cpu returns -EPROBE_DEFER when dsa_tag_protocol_get
returns -ENOPROTOOPT. It means, there is no DSA_TAG_PROTO_SJA1105 in the
list of tagging protocols known by DSA, try again later. There is a
runtime dependency for DSA to have the tagging protocol loaded. Combined
with the symbol dependency, this is a de facto circular dependency.

So when we first insmod sja1105.ko, nothing happens, probing is deferred.

Then when we insmod tag_sja1105.ko, we expect the DSA probing to kick
off where it left from, and probe the switch too.

However this does not happen because the deferred probing list in the
device core is reconsidered for a new attempt only if a driver is bound
to a new device. But DSA tagging protocols are drivers with no struct
device.

One can of course manually kick the driver after the two insmods:

echo spi0.1 > /sys/bus/spi/drivers/sja1105/bind

and this works, but automatic module loading based on modaliases will be
broken if both tag_sja1105.ko and sja1105.ko are modules, and sja1105 is
the last device to get a driver bound to it.

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

- 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

What about 566b18c8b752 ("net: dsa: sja1105: implement TX timestamping for SJA1110")?
It is essentially the same problem from a symbol usage perspective, plus
the fact that an skb queue belonging to the driver is accessed.

I believe we will have to accept that another indirect function call 
must be made in order to avoid creating a direct symbol dependency with 
sja1110_rcv_meta() would that be acceptable performance wise?
-- 
Florian

On Wed, Sep 08, 2021 at 03:14:51PM -0700, Florian Fainelli wrote:

quoted

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.

- 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?

If this was not a driver I would be maintaining, just watching as a
reviewer, I believe "no" is what I would say to that.

On 9/8/2021 5:26 PM, Vladimir Oltean wrote:

On Wed, Sep 08, 2021 at 03:14:51PM -0700, Florian Fainelli wrote:

quoted

quoted

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.

quoted

- 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).

If this was not a driver I would be maintaining, just watching as a
reviewer, I believe "no" is what I would say to that.

-- 
Florian

On Wed, Sep 08, 2021 at 05:49:17PM -0700, Florian Fainelli wrote:

On 9/8/2021 5:26 PM, Vladimir Oltean wrote:

quoted

On Wed, Sep 08, 2021 at 03:14:51PM -0700, Florian Fainelli wrote:

quoted

quoted

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.

quoted

quoted

- 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.

On Wed, Sep 08, 2021 at 04:36:00PM -0700, Florian Fainelli wrote:

On 9/8/2021 3:19 PM, Vladimir Oltean wrote:

quoted

On Wed, Sep 08, 2021 at 03:14:51PM -0700, Florian Fainelli wrote:

quoted

On 9/8/2021 3:08 PM, Vladimir Oltean wrote:

quoted

Hi,

Since commits 566b18c8b752 ("net: dsa: sja1105: implement TX
timestamping for SJA1110") and 994d2cbb08ca ("net: dsa: tag_sja1105: be
dsa_loop-safe"), net/dsa/tag_sja1105.ko has gained a build and insmod
time dependency on drivers/net/dsa/sja1105.ko, due to several symbols
exported by the latter and used by the former.

So first one needs to insmod sja1105.ko, then insmod tag_sja1105.ko.

But dsa_port_parse_cpu returns -EPROBE_DEFER when dsa_tag_protocol_get
returns -ENOPROTOOPT. It means, there is no DSA_TAG_PROTO_SJA1105 in the
list of tagging protocols known by DSA, try again later. There is a
runtime dependency for DSA to have the tagging protocol loaded. Combined
with the symbol dependency, this is a de facto circular dependency.

So when we first insmod sja1105.ko, nothing happens, probing is deferred.

Then when we insmod tag_sja1105.ko, we expect the DSA probing to kick
off where it left from, and probe the switch too.

However this does not happen because the deferred probing list in the
device core is reconsidered for a new attempt only if a driver is bound
to a new device. But DSA tagging protocols are drivers with no struct
device.

One can of course manually kick the driver after the two insmods:

echo spi0.1 > /sys/bus/spi/drivers/sja1105/bind

and this works, but automatic module loading based on modaliases will be
broken if both tag_sja1105.ko and sja1105.ko are modules, and sja1105 is
the last device to get a driver bound to it.

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

- 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

What about 566b18c8b752 ("net: dsa: sja1105: implement TX timestamping for SJA1110")?
It is essentially the same problem from a symbol usage perspective, plus
the fact that an skb queue belonging to the driver is accessed.

I believe we will have to accept that another indirect function call must be
made in order to avoid creating a direct symbol dependency with
sja1110_rcv_meta() would that be acceptable performance wise?
--
Florian

The same circular dependency problem exists between net/dsa/tag_ocelot_8021q.c
and drivers/net/ethernet/mscc/ocelot.c, which exports ocelot_port_inject_frame
and co. This one is fundamentally even worse, I could make all of the
ocelot_port_inject_frame related functions static inline inside
include/linux/dsa/ocelot.h, but for that to do anything, I'd also need
to make the I/O functions themselves static inline, like __ocelot_write_ix
which is called by ocelot_write_rix. That doesn't sound too fun, sooner
or later I'm going to make the entire driver static inline :)

The alternative I see would be to just inject the PTP frames from a
deferred worker, a la sja1105 with its magical SPI commands. A positive
side effect of doing that would be that I can even try harder to inject
them. Currently, because DSA uses NETIF_F_LLTX, it can't return
NETDEV_TX_BUSY to ask the qdisc to requeue the packet, because the qdisc
is, well, noqueue (we talked about this before). But "ocelot_can_inject"
can return false, due to nasty stuff like egress congestion. The current
behavior is to immediately drop the PTP frame, which leads to its own
kind of nastiness - we have that skb enqueued in a list of frames
awaiting TX timestamps, but the timestamp for it will never come because
it's been silently dropped. Then we have a timestamp ID that rolls over
after it counts to 4, and since we keep counting, future PTP timestamps
might match on the skb that is still in the TX timestamping queue but
stale. And nobody will match on the real skb. It goes downhill really
fast and stinks.

What do you think, should I go ahead and make a second user of deferred
xmit (first and foremost to break the cyclic dependency between the
tagger and the driver), and try harder to enqueue the PTP packets
through register-based MMIO?