From: Andrew Lunn <andrew@lunn•ch>
To: Vladimir Oltean <olteanv@gmail•com>
Cc: Vladimir Oltean <vladimir.oltean@nxp•com>,
netdev@vger•kernel.org, Vivien Didelot <vivien.didelot@gmail•com>,
Florian Fainelli <f.fainelli@gmail•com>
Subject: Re: [PATCH net] net: dsa: mv88e6xxx: stop calling irq_domain_add_simple with the reg_lock held
Date: Fri, 27 Aug 2021 22:04:14 +0200 [thread overview]
Message-ID: <YSlFPhtmcI116ciO@lunn.ch> (raw)
In-Reply-To: <20210827184525.p44pir5or4h5nwgk@skbuf>
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
next prev parent reply other threads:[~2021-08-27 20:04 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-27 18:01 [PATCH net] net: dsa: mv88e6xxx: stop calling irq_domain_add_simple with the reg_lock held Vladimir Oltean
2021-08-27 18:34 ` Andrew Lunn
2021-08-27 18:45 ` Vladimir Oltean
2021-08-27 20:04 ` Andrew Lunn [this message]
2021-08-27 20:13 ` Vladimir Oltean
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YSlFPhtmcI116ciO@lunn.ch \
--to=andrew@lunn$(echo .)ch \
--cc=f.fainelli@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=olteanv@gmail$(echo .)com \
--cc=vivien.didelot@gmail$(echo .)com \
--cc=vladimir.oltean@nxp$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox