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

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

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),
hence the reason why I did not believe it would make such a huge
difference.

I realize now it puts you in a bad light since it suggests you didn't
test it with lockdep, and I apologize.

quoted

Only hardware access should need the register lock, and this in itself
is for the mv88e6xxx_smi_indirect_ops to work properly and nothing more,
unless I'm misunderstanding something

Historically, reg_lock has been used to serialize all access to the
hardware across entries points into the driver. Not everything takes
rtnl lock. Clearly, interrupts don't. I don't know if PTP takes it. In
the past there was been hwmon code, etc. The reg_lock is used to
serialize all this. The patterns of all entry points into the driver
taking the lock has in general worked well. Just interrupt code is a
pain.

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. IMHO, proper locking would need to find
what are the sequences of SMI reads/writes that need to be indivisible,
and only lock around those, and lock per topic if possible.

quoted

Fixes: dc30c35be720 ("net: dsa: mv88e6xxx: Implement interrupt support.")

As i said, i suspect this is the wrong commit. You need to look at
changes to the interrupt core. There is even a danger that if this
gets backported too far, it could add deadlocks.

Ok, retarget to "net-next"?