[PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller

[PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller

[Sonic Zhang]

From: Sonic Zhang <sonic.zhang@analog.com>

It is possible that poll_controller is called from netpoll.c with irq
already disabled. Unexpected interrupt may be triggered if always
enable irq before returning from poll_controller.

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 05c146f..830c008 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2465,9 +2465,11 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
  */
 static void stmmac_poll_controller(struct net_device *dev)
 {
-	disable_irq(dev->irq);
+	unsigned long flags;
+
+	local_irq_save(flags);
 	stmmac_interrupt(dev->irq, dev);
-	enable_irq(dev->irq);
+	local_irq_restore(flags);
 }
 #endif
 
-- 
1.7.9.5

Re: [PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller

[David Miller]

From: Sonic Zhang <sonic.adi@gmail.com>
Date: Tue, 5 May 2015 15:55:53 +0800

> From: Sonic Zhang <sonic.zhang@analog.com>
> 
> It is possible that poll_controller is called from netpoll.c with irq
> already disabled. Unexpected interrupt may be triggered if always
> enable irq before returning from poll_controller.
> 
> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>

Nope, this is wrong.

If you don't use disable_irq(), the interrupt handler can run in
parallel on another cpu and you definitely do not want that.

There is nothing wrong with the interrupt triggering on the
enable_irq() here.  If that causes some problem, there is a
bug in this driver.

Re: [PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller

[Sonic Zhang]

Hi David,

On Wed, May 6, 2015 at 7:36 AM, David Miller <davem@davemloft.net> wrote:
> From: Sonic Zhang <sonic.adi@gmail.com>
> Date: Tue, 5 May 2015 15:55:53 +0800
>
>> From: Sonic Zhang <sonic.zhang@analog.com>
>>
>> It is possible that poll_controller is called from netpoll.c with irq
>> already disabled. Unexpected interrupt may be triggered if always
>> enable irq before returning from poll_controller.
>>
>> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
>
> Nope, this is wrong.
>
> If you don't use disable_irq(), the interrupt handler can run in
> parallel on another cpu and you definitely do not want that.

How about change local_irq_save(flags) to
spin_lock_irqsave(&priv->lock, flags) ?

>
> There is nothing wrong with the interrupt triggering on the
> enable_irq() here.  If that causes some problem, there is a
> bug in this driver.

When I debug Linux kernel via KGDB over Ethernet, KGDB calls netpoll
API and stmmac_poll_controller to send UDP packets with all interrupts
disabled. KGDB assumes all interrupts keeps disabled when it is active
and doing UDP data transmission. Enabling interrupts in
stmmac_poll_controller causes KGDB re-enter exception and system halt.

Although KGDB over Ethernet is not part of the mainline tree, I feel
this patch is useful in case other kernel functions want to call
netpoll API with interrupts disabled.


Regards,

Sonic

Re: [PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller

[David Miller]

From: Sonic Zhang <sonic.adi@gmail.com>
Date: Wed, 6 May 2015 11:53:53 +0800

> When I debug Linux kernel via KGDB over Ethernet, KGDB calls netpoll
> API and stmmac_poll_controller to send UDP packets with all interrupts
> disabled. KGDB assumes all interrupts keeps disabled when it is active
> and doing UDP data transmission. Enabling interrupts in
> stmmac_poll_controller causes KGDB re-enter exception and system halt.

If KGDB has disabled local cpu interrupts, that cannot happen.

What stmmac_poll_controller() is doing is enabling a specific interupt
source, not "interrupts" generally.  The latter is what KGDB can
specifically control and should care about.