All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller
@ 2015-05-05  7:55 Sonic Zhang
  2015-05-05 23:36 ` David Miller
  0 siblings, 1 reply; 4+ messages in thread
From: Sonic Zhang @ 2015-05-05  7:55 UTC (permalink / raw)
  To: Giuseppe Cavallaro, David S. Miller
  Cc: netdev, adi-buildroot-devel, 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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller
  2015-05-05  7:55 [PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller Sonic Zhang
@ 2015-05-05 23:36 ` David Miller
  2015-05-06  3:53   ` Sonic Zhang
  0 siblings, 1 reply; 4+ messages in thread
From: David Miller @ 2015-05-05 23:36 UTC (permalink / raw)
  To: sonic.adi; +Cc: peppe.cavallaro, netdev, adi-buildroot-devel, sonic.zhang

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller
  2015-05-05 23:36 ` David Miller
@ 2015-05-06  3:53   ` Sonic Zhang
  2015-05-06 14:02     ` David Miller
  0 siblings, 1 reply; 4+ messages in thread
From: Sonic Zhang @ 2015-05-06  3:53 UTC (permalink / raw)
  To: David Miller
  Cc: Giuseppe CAVALLARO, netdev, adi-buildroot-devel, Zhang, Sonic

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller
  2015-05-06  3:53   ` Sonic Zhang
@ 2015-05-06 14:02     ` David Miller
  0 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2015-05-06 14:02 UTC (permalink / raw)
  To: sonic.adi; +Cc: peppe.cavallaro, netdev, adi-buildroot-devel, sonic.zhang

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-05-06 14:02 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-05  7:55 [PATCH] stmmac: call local_irq_save other than disable_irq in poll_controller Sonic Zhang
2015-05-05 23:36 ` David Miller
2015-05-06  3:53   ` Sonic Zhang
2015-05-06 14:02     ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.