Re: [PATCH net] net: dsa: mv88e6xxx: stop calling irq_domain_add_simple with the reg_lock held

On Fri, Aug 27, 2021 at 09:45:25PM +0300, Vladimir Oltean wrote:

On Fri, Aug 27, 2021 at 08:34:33PM +0200, Andrew Lunn wrote:

quoted

On Fri, Aug 27, 2021 at 09:01:01PM +0300, Vladimir Oltean wrote:

quoted

The mv88e6xxx IRQ setup code has some pretty horrible locking patterns,
and wrong.

I agree about the patterns. But it has been lockdep clean, i spent a
while testing it, failed probes, unloads etc, and adding comments.

I suspect it is now wrong because of core changes.

It's true, it is lockdep-clean the way it is structured now, but I
suspect that is purely by chance. I had to shift code around a bit to
get lockdep to shout, my bad for not really mentioning it: I moved
mv88e6xxx_mdios_register from mv88e6xxx_probe to mv88e6xxx_setup, all in
all a relatively superficial change (I am trying to test something out),

That move is actually quite interesting. It can cut down the probe
time quite a bit, which is important when you know the first probe is
going to fail anyway with an EPRODE_DEFER. But we have to be careful
with MDIO busses, as you can see from the other discussions.

I empathize with working in the blind w.r.t. locking, when rtnl_mutex
covers everything. As you point out, threaded interrupts do not the
rtnl_lock, so that is a good opportunity to analyze what needs serialization,
which I do not have on sja1105. Nonetheless, my experience is that
hardware is a pretty parallel/reentrant beast, a "register lock" is
almost always the wrong answer.

That lock has been there since forever. And the driver was written by
a Marvell Switch engineer. Maybe it is not needed, but i'm hesitant to
take it out.

Ok, retarget to "net-next"?

I would prefer to wait until you have finished your testing and have
something which builds upon it. If its not broken, don't fix it...

	  Andrew