Re: [PATCH net] net: dsa: fix lockdep warning

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Sun, Feb 17, 2019 at 06:55:39PM +0100, Andrew Lunn wrote:
> On Sun, Feb 17, 2019 at 04:27:32PM +0000, Russell King wrote:
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 4.20.0+ #302 Not tainted
> > ------------------------------------------------------
> > systemd-udevd/160 is trying to acquire lock:
> > edea6080 (&chip->reg_lock){+.+.}, at: __setup_irq+0x640/0x704
> > 
> > but task is already holding lock:
> > edff0340 (&desc->request_mutex){+.+.}, at: __setup_irq+0xa0/0x704
> > 
> > which lock already depends on the new lock.
> > 
> > the existing dependency chain (in reverse order) is:
> > 
> > -> #1 (&desc->request_mutex){+.+.}:
> >        mutex_lock_nested+0x1c/0x24
> >        __setup_irq+0xa0/0x704
> >        request_threaded_irq+0xd0/0x150
> >        mv88e6xxx_probe+0x41c/0x694 [mv88e6xxx]
> 
> > -> #0 (&chip->reg_lock){+.+.}:
> >        __mutex_lock+0x50/0x8b8
> >        mutex_lock_nested+0x1c/0x24
> >        __setup_irq+0x640/0x704
> >        request_threaded_irq+0xd0/0x150
> >        mv88e6xxx_g2_irq_setup+0xcc/0x1b4 [mv88e6xxx]
> >        mv88e6xxx_probe+0x44c/0x694 [mv88e6xxx]
> >        mdio_probe+0x2c/0x54
> > 
> > other info that might help us debug this:
> > 
> >  Possible unsafe locking scenario:
> > 
> >        CPU0                    CPU1
> >        ----                    ----
> >   lock(&desc->request_mutex);
> >                                lock(&chip->reg_lock);
> >                                lock(&desc->request_mutex);
> >   lock(&chip->reg_lock);
> 
> Hi Russell
> 
> I failed to reproduce it on a Freescale system. Which made me take a
> closer look at the above. This is a false positive.
> 
> In #1 we are requesting the GPIO interrupt. In #2 we are requesting
> the chained interrupt from the mv88e6xxx global 1 interrupt handler.
> So these are different desc->request_mutex. The Freescale VF610 GPIO
> driver uses gpiochip_irqchip_add(), which creates a lock class for the
> GPIO. The marvell gpio-mvebu driver does not create a lock class.  So
> when i test on Freescale, lockdep can tell they are different mutex,
> but on clearfog it cannot.
> 
> So i think the real fix is probably two fold, although just doing one
> is sufficient:
> 
> 1) Add lock classes to gpio-mvebu, by call irq_set_lockdep_class()
> 2) Add lock classes to chip.c global 1, by calling irq_set_lockdep_class()
> 
> There is probably more value in 1) since the mvebu gpio driver is much
> more widely used than the mv88e6xxx driver.

I'd ask one question: is there any reason to hold chip->reg_lock while
calling request_threaded_irq()?  If not, then surely it would be best
not to do so?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up