Patchwork [v3,net,bna] Fix call trace when interrupts are disabled while sleeping function kzalloc is called

login
register
mail settings
Submitter Shyam_Iyer@Dell.com
Date July 7, 2011, 9:53 p.m.
Message ID <DBFB1B45AF80394ABD1C807E9F28D157063779959F@BLRX7MCDC203.AMER.DELL.COM>
Download mbox | patch
Permalink /patch/103735/
State RFC
Delegated to: David Miller
Headers show

Comments

Shyam_Iyer@Dell.com - July 7, 2011, 9:53 p.m.
> -----Original Message-----

> From: Ivan Vecera [mailto:ivecera@redhat.com]

> Sent: Thursday, July 07, 2011 11:14 AM

> To: Shyam Iyer

> Cc: netdev@vger.kernel.org; rmody@brocade.com; ddutt@brocade.com;

> huangj@brocade.com; davem@davemloft.net; Iyer, Shyam

> Subject: Re: [PATCH] [v3][net][bna] Fix call trace when interrupts are

> disabled while sleeping function kzalloc is called

> 

> Small note, the root of the problem was that non-atomic allocation was

> requested with IRQs disabled. Your patch description does not contain

> why were the IRQs disabled.

> The function bnad_mbox_irq_alloc incorrectly uses 'flags' var for two

> different things, 1) to save current CPU flags and 2) for request_irq

> call.

> First the spin_lock_irqsave disables the IRQs and saves _all_ CPU flags

> (including one that enables/disables interrupts) to 'flags'. Then the

> 'flags' is overwritten by 0 or 0x80 (IRQF_SHARED). Finally the

> spin_unlock_irqrestore should restore saved flags, but these flags are

> now either 0x00 or 0x80. The interrupt bit value in flags register on

> x86 arch is 0x100.

> This means that the interrupt bit is zero (IRQs disabled) after

> spin_unlock_irqrestore so the request_irq function is called with

> disabled interrupts.


That seems to make a lot more sense.. and that way I don't have to initialize the flags variable outside of the spin_lock_irqsave/restore bracket to preserve the irqs not being disabled when allocating in request_irq

Would the below patch make sense instead...? (Note that since davem has accepted the earlier one this is on top of the already committed patch)

(END)

> 

> Rasesh, this is not a good idea to use one variable for two different

> purposes in parallel.

> 

> Regards,

> Ivan

> 

> On Tue, 2011-06-28 at 14:58 -0400, Shyam Iyer wrote:

> > request_threaded irq will call kzalloc that can sleep. Initializing

> the flags variable outside of spin_lock_irqsave/restore in

> bnad_mbox_irq_alloc will avoid call traces like below.

> >

> > Jun 27 08:15:24 home-t710 kernel: [11735.634550] Brocade 10G Ethernet

> driver

> > Jun 27 08:15:24 home-t710 kernel: [11735.634590] bnad_pci_probe :

> (0xffff880427f3d000, 0xffffffffa020f3e0) PCI Func : (2)

> > Jun 27 08:15:24 home-t710 kernel: [11735.637677] bna 0000:82:00.2:

> PCI INT A -> GSI 66 (level, low) -> IRQ 66

> > Jun 27 08:15:24 home-t710 kernel: [11735.638290] bar0 mapped to

> ffffc90014980000, len 262144

> > Jun 27 08:15:24 home-t710 kernel: [11735.638732] BUG: sleeping

> function called from invalid context at mm/slub.c:847

> > Jun 27 08:15:24 home-t710 kernel: [11735.638736] in_atomic(): 0,

> irqs_disabled(): 1, pid: 11243, name: insmod

> > Jun 27 08:15:24 home-t710 kernel: [11735.638740] Pid: 11243, comm:

> insmod Not tainted 3.0.0-rc4+ #6

> > Jun 27 08:15:24 home-t710 kernel: [11735.638743] Call Trace:

> > Jun 27 08:15:24 home-t710 kernel: [11735.638755]

> [<ffffffff81046427>] __might_sleep+0xeb/0xf0

> > Jun 27 08:15:24 home-t710 kernel: [11735.638766]

> [<ffffffffa01fe469>] ? netif_wake_queue+0x3d/0x3d [bna]

> > Jun 27 08:15:24 home-t710 kernel: [11735.638773]

> [<ffffffff8111201c>] kmem_cache_alloc_trace+0x43/0xd8

> > Jun 27 08:15:24 home-t710 kernel: [11735.638782]

> [<ffffffffa01fe469>] ? netif_wake_queue+0x3d/0x3d [bna]

> > Jun 27 08:15:24 home-t710 kernel: [11735.638787]

> [<ffffffff810ab791>] request_threaded_irq+0xa1/0x113

> > Jun 27 08:15:24 home-t710 kernel: [11735.638798]

> [<ffffffffa020f0c0>] bnad_pci_probe+0x612/0x8e5 [bna]

> > Jun 27 08:15:24 home-t710 kernel: [11735.638807]

> [<ffffffffa01fe469>] ? netif_wake_queue+0x3d/0x3d [bna]

> > Jun 27 08:15:24 home-t710 kernel: [11735.638816]

> [<ffffffff81482ef4>] ? _raw_spin_unlock_irqrestore+0x17/0x19

> > Jun 27 08:15:24 home-t710 kernel: [11735.638822]

> [<ffffffff8124d17a>] local_pci_probe+0x44/0x75

> > Jun 27 08:15:24 home-t710 kernel: [11735.638826]

> [<ffffffff8124dc06>] pci_device_probe+0xd0/0xff

> > Jun 27 08:15:24 home-t710 kernel: [11735.638832]

> [<ffffffff812ef8ab>] driver_probe_device+0x131/0x213

> > Jun 27 08:15:24 home-t710 kernel: [11735.638836]

> [<ffffffff812ef9e7>] __driver_attach+0x5a/0x7e

> > Jun 27 08:15:24 home-t710 kernel: [11735.638840]

> [<ffffffff812ef98d>] ? driver_probe_device+0x213/0x213

> > Jun 27 08:15:24 home-t710 kernel: [11735.638844]

> [<ffffffff812ee933>] bus_for_each_dev+0x53/0x89

> > Jun 27 08:15:24 home-t710 kernel: [11735.638848]

> [<ffffffff812ef48a>] driver_attach+0x1e/0x20

> > Jun 27 08:15:24 home-t710 kernel: [11735.638852]

> [<ffffffff812ef0ae>] bus_add_driver+0xd1/0x224

> > Jun 27 08:15:24 home-t710 kernel: [11735.638858]

> [<ffffffffa01b8000>] ? 0xffffffffa01b7fff

> > Jun 27 08:15:24 home-t710 kernel: [11735.638862]

> [<ffffffff812efe57>] driver_register+0x98/0x105

> > Jun 27 08:15:24 home-t710 kernel: [11735.638866]

> [<ffffffffa01b8000>] ? 0xffffffffa01b7fff

> > Jun 27 08:15:24 home-t710 kernel: [11735.638871]

> [<ffffffff8124e4c9>] __pci_register_driver+0x56/0xc1

> > Jun 27 08:15:24 home-t710 kernel: [11735.638875]

> [<ffffffffa01b8000>] ? 0xffffffffa01b7fff

> > Jun 27 08:15:24 home-t710 kernel: [11735.638884]

> [<ffffffffa01b8040>] bnad_module_init+0x40/0x60 [bna]

> > Jun 27 08:15:24 home-t710 kernel: [11735.638892]

> [<ffffffff81002099>] do_one_initcall+0x7f/0x136

> > Jun 27 08:15:24 home-t710 kernel: [11735.638899]

> [<ffffffff8108608b>] sys_init_module+0x88/0x1d0

> > Jun 27 08:15:24 home-t710 kernel: [11735.638906]

> [<ffffffff81489682>] system_call_fastpath+0x16/0x1b

> > Jun 27 08:15:24 home-t710 kernel: [11735.639642] bnad_pci_probe :

> (0xffff880427f3e000, 0xffffffffa020f3e0) PCI Func : (3)

> > Jun 27 08:15:24 home-t710 kernel: [11735.639665] bna 0000:82:00.3:

> PCI INT A -> GSI 66 (level, low) -> IRQ 66

> > Jun 27 08:15:24 home-t710 kernel: [11735.639735] bar0 mapped to

> ffffc90014400000, len 262144

> >

> > Signed-off-by: Shyam Iyer <shyam_iyer@dell.com>

> > ---

> >  drivers/net/bna/bnad.c |    7 +++----

> >  1 files changed, 3 insertions(+), 4 deletions(-)

> >

> > diff --git a/drivers/net/bna/bnad.c b/drivers/net/bna/bnad.c

> > index 7d25a97..44e219c 100644

> > --- a/drivers/net/bna/bnad.c

> > +++ b/drivers/net/bna/bnad.c

> > @@ -1111,7 +1111,7 @@ bnad_mbox_irq_alloc(struct bnad *bnad,

> >  		    struct bna_intr_info *intr_info)

> >  {

> >  	int 		err = 0;

> > -	unsigned long 	flags;

> > +	unsigned long 	irq_flags = 0, flags;

> >  	u32	irq;

> >  	irq_handler_t 	irq_handler;

> >

> > @@ -1125,18 +1125,17 @@ bnad_mbox_irq_alloc(struct bnad *bnad,

> >  	if (bnad->cfg_flags & BNAD_CF_MSIX) {

> >  		irq_handler = (irq_handler_t)bnad_msix_mbox_handler;

> >  		irq = bnad->msix_table[bnad->msix_num - 1].vector;

> > -		flags = 0;

> >  		intr_info->intr_type = BNA_INTR_T_MSIX;

> >  		intr_info->idl[0].vector = bnad->msix_num - 1;

> >  	} else {

> >  		irq_handler = (irq_handler_t)bnad_isr;

> >  		irq = bnad->pcidev->irq;

> > -		flags = IRQF_SHARED;

> > +		irq_flags = IRQF_SHARED;

> >  		intr_info->intr_type = BNA_INTR_T_INTX;

> >  		/* intr_info->idl.vector = 0 ? */

> >  	}

> >  	spin_unlock_irqrestore(&bnad->bna_lock, flags);

> > -

> > +	flags = irq_flags;

> >  	sprintf(bnad->mbox_irq_name, "%s", BNAD_NAME);

> >

> >  	/*

> 

>
Ivan Vecera - July 12, 2011, 12:42 p.m.
On Fri, 2011-07-08 at 03:23 +0530, Shyam_Iyer@Dell.com wrote:
> 
> > -----Original Message-----
> > From: Ivan Vecera [mailto:ivecera@redhat.com]
> > Sent: Thursday, July 07, 2011 11:14 AM
> > To: Shyam Iyer
> > Cc: netdev@vger.kernel.org; rmody@brocade.com; ddutt@brocade.com;
> > huangj@brocade.com; davem@davemloft.net; Iyer, Shyam
> > Subject: Re: [PATCH] [v3][net][bna] Fix call trace when interrupts are
> > disabled while sleeping function kzalloc is called
> > 
> > Small note, the root of the problem was that non-atomic allocation was
> > requested with IRQs disabled. Your patch description does not contain
> > why were the IRQs disabled.
> > The function bnad_mbox_irq_alloc incorrectly uses 'flags' var for two
> > different things, 1) to save current CPU flags and 2) for request_irq
> > call.
> > First the spin_lock_irqsave disables the IRQs and saves _all_ CPU flags
> > (including one that enables/disables interrupts) to 'flags'. Then the
> > 'flags' is overwritten by 0 or 0x80 (IRQF_SHARED). Finally the
> > spin_unlock_irqrestore should restore saved flags, but these flags are
> > now either 0x00 or 0x80. The interrupt bit value in flags register on
> > x86 arch is 0x100.
> > This means that the interrupt bit is zero (IRQs disabled) after
> > spin_unlock_irqrestore so the request_irq function is called with
> > disabled interrupts.
> 
> That seems to make a lot more sense.. and that way I don't have to initialize the flags variable outside of the spin_lock_irqsave/restore bracket to preserve the irqs not being disabled when allocating in request_irq
> 
> Would the below patch make sense instead...? (Note that since davem has accepted the earlier one this is on top of the already committed patch)
> 
Yes, this is much cleaner...

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/bna/bnad.c b/drivers/net/bna/bnad.c

index 44e219c..ea13605 100644

--- a/drivers/net/bna/bnad.c

+++ b/drivers/net/bna/bnad.c

@@ -1111,7 +1111,7 @@  bnad_mbox_irq_alloc(struct bnad *bnad,

                    struct bna_intr_info *intr_info)
 {
        int             err = 0;
-       unsigned long   irq_flags = 0, flags;

+       unsigned long   irq_flags, flags;

        u32     irq;
        irq_handler_t   irq_handler;
 
@@ -1125,6 +1125,7 @@  bnad_mbox_irq_alloc(struct bnad *bnad,

        if (bnad->cfg_flags & BNAD_CF_MSIX) {
                irq_handler = (irq_handler_t)bnad_msix_mbox_handler;
                irq = bnad->msix_table[bnad->msix_num - 1].vector;
+               irq_flags = 0;

                intr_info->intr_type = BNA_INTR_T_MSIX;
                intr_info->idl[0].vector = bnad->msix_num - 1;
        } else {
@@ -1135,7 +1136,6 @@  bnad_mbox_irq_alloc(struct bnad *bnad,

                /* intr_info->idl.vector = 0 ? */
        }
        spin_unlock_irqrestore(&bnad->bna_lock, flags);
-       flags = irq_flags;

        sprintf(bnad->mbox_irq_name, "%s", BNAD_NAME);
 
        /*
@@ -1146,7 +1146,7 @@  bnad_mbox_irq_alloc(struct bnad *bnad,

 
        BNAD_UPDATE_CTR(bnad, mbox_intr_disabled);
 
-       err = request_irq(irq, irq_handler, flags,

+       err = request_irq(irq, irq_handler, irq_flags,

                          bnad->mbox_irq_name, bnad);
 
        if (err) {