| Message ID | DBFB1B45AF80394ABD1C807E9F28D157063779959F@BLRX7MCDC203.AMER.DELL.COM |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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) {