From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sabrina Dubroca Subject: Re: [RFC PATCH net-next 04/11] atl1c: remove disable_irq from netpoll controller, use netpoll_irq_lock Date: Tue, 9 Dec 2014 18:23:10 +0100 Message-ID: <20141209172310.GA14343@kria> References: <1418135842-21389-1-git-send-email-sd@queasysnail.net> <1418135842-21389-5-git-send-email-sd@queasysnail.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: davem@davemloft.net, netdev@vger.kernel.org, Jay Cliburn To: Chris Snook Return-path: Received: from smtp2-g21.free.fr ([212.27.42.2]:5973 "EHLO smtp2-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750931AbaLIRXR (ORCPT ); Tue, 9 Dec 2014 12:23:17 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 2014-12-09, 16:13:33 +0000, Chris Snook wrote: > Could you explain the bug a little more for us? It's not obvious to me how > sleeping there is a problem. > > -- Chris Sorry for the lack of context. A might_sleep() check in disable_irq() was added in commit e22b886a8a43b ("sched/wait: Add might_sleep() checks") [1], and it triggers when using netconsole: BUG: sleeping function called from invalid context at kernel/irq/manage.c:104 in_atomic(): 1, irqs_disabled(): 1, pid: 1, name: systemd no locks held by systemd/1. irq event stamp: 10102965 hardirqs last enabled at (10102965): [] vprintk_emit+0x2dd/0x6a0 hardirqs last disabled at (10102964): [] vprintk_emit+0x77/0x6a0 softirqs last enabled at (10102342): [] __do_softirq+0x27a/0x6f0 softirqs last disabled at (10102337): [] irq_exit+0x56/0xe0 Preemption disabled at:[] printk_emit+0x31/0x33 CPU: 1 PID: 1 Comm: systemd Not tainted 3.18.0-rc2-next-20141029-dirty #222 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140617_173321-var-lib-archbuild-testing-x86_64-tobias 04/01/2014 ffffffff81a82291 ffff88001e743978 ffffffff817df31d 0000000000000000 0000000000000000 ffff88001e7439a8 ffffffff8108dfa2 ffff88001e7439a8 ffffffff81a82291 0000000000000068 0000000000000000 ffff88001e7439d8 Call Trace: [] dump_stack+0x4f/0x7c [] ___might_sleep+0x182/0x2b0 [] __might_sleep+0x3a/0xc0 [] synchronize_irq+0x38/0xa0 [] ? __disable_irq_nosync+0x43/0x70 [] disable_irq+0x20/0x30 [] e1000_netpoll+0x23/0x60 [] netpoll_poll_dev+0x72/0x3a0 [] ? _raw_spin_trylock+0x73/0x90 [] ? netpoll_send_skb_on_dev+0x1df/0x2e0 [] netpoll_send_skb_on_dev+0x1b7/0x2e0 [] netpoll_send_udp+0x2e3/0x490 The initial discussion of this problem is here: https://lkml.org/lkml/2014/10/29/523 [1] https://lkml.org/lkml/2014/10/28/427 > On Tue, Dec 9, 2014, 06:39 Sabrina Dubroca wrote: > > > disable_irq() may sleep, replace it with a spin_lock in the interrupt > > handler. > > > > No actual testing done, only compiled. > > > > Signed-off-by: Sabrina Dubroca > > Cc: Jay Cliburn > > Cc: Chris Snook > > --- > > drivers/net/ethernet/atheros/atl1c/atl1c.h | 3 +++ > > drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 12 ++++++++---- > > 2 files changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c.h > > b/drivers/net/ethernet/atheros/atl1c/atl1c.h > > index b9203d928938..8d97791e1516 100644 > > --- a/drivers/net/ethernet/atheros/atl1c/atl1c.h > > +++ b/drivers/net/ethernet/atheros/atl1c/atl1c.h > > @@ -49,6 +49,7 @@ > > #include > > #include > > #include > > +#include > > > > #include "atl1c_hw.h" > > > > @@ -555,6 +556,8 @@ struct atl1c_adapter { > > struct atl1c_rfd_ring rfd_ring; > > struct atl1c_rrd_ring rrd_ring; > > u32 bd_number; /* board number;*/ > > + > > + struct netpoll_irq_lock netpoll_lock; > > }; > > > > #define AT_WRITE_REG(a, reg, value) ( \ > > diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c > > b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c > > index 72fb86b9aa24..7a1b11eb8e4e 100644 > > --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c > > +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c > > @@ -826,6 +826,7 @@ static int atl1c_sw_init(struct atl1c_adapter *adapter) > > atomic_set(&adapter->irq_sem, 1); > > spin_lock_init(&adapter->mdio_lock); > > spin_lock_init(&adapter->tx_lock); > > + netpoll_irq_lock_init(&adapter->netpoll_lock); > > set_bit(__AT_DOWN, &adapter->flags); > > > > return 0; > > @@ -1584,10 +1585,11 @@ static irqreturn_t atl1c_intr(int irq, void *data) > > struct pci_dev *pdev = adapter->pdev; > > struct atl1c_hw *hw = &adapter->hw; > > int max_ints = AT_MAX_INT_WORK; > > - int handled = IRQ_NONE; > > + irqreturn_t handled = IRQ_NONE; > > u32 status; > > u32 reg_data; > > > > + netpoll_irq_lock(&adapter->netpoll_lock); > > do { > > AT_READ_REG(hw, REG_ISR, ®_data); > > status = reg_data & hw->intr_mask; > > @@ -1622,7 +1624,8 @@ static irqreturn_t atl1c_intr(int irq, void *data) > > /* reset MAC */ > > set_bit(ATL1C_WORK_EVENT_RESET, > > &adapter->work_event); > > schedule_work(&adapter->common_task); > > - return IRQ_HANDLED; > > + handled = IRQ_HANDLED; > > + goto out; > > } > > > > if (status & ISR_OVER) > > @@ -1641,6 +1644,9 @@ static irqreturn_t atl1c_intr(int irq, void *data) > > } while (--max_ints > 0); > > /* re-enable Interrupt*/ > > AT_WRITE_REG(&adapter->hw, REG_ISR, 0); > > + > > +out: > > + netpoll_irq_unlock(&adapter->netpoll_lock); > > return handled; > > } > > > > @@ -1900,9 +1906,7 @@ static void atl1c_netpoll(struct net_device *netdev) > > { > > struct atl1c_adapter *adapter = netdev_priv(netdev); > > > > - disable_irq(adapter->pdev->irq); > > atl1c_intr(adapter->pdev->irq, netdev); > > - enable_irq(adapter->pdev->irq); > > } > > #endif > > > > -- > > 2.1.3 > > > > -- Sabrina