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

From: Andrew Lunn <[email-masked]>
Date: Fri, 27 Aug 2021 22:04:14 +0200

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:
> > On Fri, Aug 27, 2021 at 09:01:01PM +0300, Vladimir Oltean wrote:
> > > 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