netdev - Re: [PATCH net] net: dsa: mv88e6xxx: stop calling irq_domain_add_simple with the reg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YSlFPhtmcI116ciO@lunn.ch>
Date:   Fri, 27 Aug 2021 22:04:14 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: 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:
> > 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