[PATCH] macb: Add support of the netpoll API

[PATCH] macb: Add support of the netpoll API

[Thomas Petazzoni]

macb: Add support of the netpoll API

With this patch in place, I'm successfully able to use the netconsole
mechanism with the Calao USB-A9263 board, which uses the AT91SAM9263
CPU, which in terms of Ethernet controller is supported by the macb
driver.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/net/macb.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Index: linux/drivers/net/macb.c
===================================================================
--- linux.orig/drivers/net/macb.c
+++ linux/drivers/net/macb.c
@@ -616,6 +616,19 @@
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+/*
+ * Polling receive - used by netconsole and other diagnostic tools
+ * to allow network i/o with interrupts disabled.
+ */
+static void macb_poll_controller(struct net_device *dev)
+{
+	disable_irq(dev->irq);
+	macb_interrupt(dev->irq, dev);
+	enable_irq(dev->irq);
+}
+#endif
+
 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);
@@ -1184,6 +1197,9 @@
 	dev->do_ioctl = macb_ioctl;
 	netif_napi_add(dev, &bp->napi, macb_poll, 64);
 	dev->ethtool_ops = &macb_ethtool_ops;
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	dev->poll_controller = macb_poll_controller;
+#endif
 
 	dev->base_addr = regs->start;
 


-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [PATCH] macb: Add support of the netpoll API

[Haavard Skinnemoen]

Thomas Petazzoni wrote:
> macb: Add support of the netpoll API
> 
> With this patch in place, I'm successfully able to use the netconsole
> mechanism with the Calao USB-A9263 board, which uses the AT91SAM9263
> CPU, which in terms of Ethernet controller is supported by the macb
> driver.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/net/macb.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> Index: linux/drivers/net/macb.c
> ===================================================================
> --- linux.orig/drivers/net/macb.c
> +++ linux/drivers/net/macb.c
> @@ -616,6 +616,19 @@
>  	return IRQ_HANDLED;
>  }
>  
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> +/*
> + * Polling receive - used by netconsole and other diagnostic tools
> + * to allow network i/o with interrupts disabled.
> + */
> +static void macb_poll_controller(struct net_device *dev)
> +{
> +	disable_irq(dev->irq);

Hmm...is this safe? What if printk() is called from the macb interrupt
handler?

Haavard

Re: [PATCH] macb: Add support of the netpoll API

[Thomas Petazzoni]

Le Thu, 16 Apr 2009 11:07:53 +0200,
Haavard Skinnemoen <haavard.skinnemoen@atmel.com> a écrit :

> Hmm...is this safe? What if printk() is called from the macb interrupt
> handler?

I'm not sure, but that strategy is used in most drivers supporting the
netpoll API (3c509.c, 8139cp.c, 8139too.c, b44.c, bfin_mac.c,
bnx2x_main.c, etc.). It also seems to be the way suggested by netpoll
author, http://oss.sgi.com/archives/netdev/2003-10/msg00800.html.

disable_irq() only disables the macb IRQ line. Is that an issue for
printk() execution ?

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [PATCH] macb: Add support of the netpoll API

[Haavard Skinnemoen]

Thomas Petazzoni wrote:
> Le Thu, 16 Apr 2009 11:07:53 +0200,
> Haavard Skinnemoen <haavard.skinnemoen@atmel.com> a écrit :
> 
> > Hmm...is this safe? What if printk() is called from the macb interrupt
> > handler?  
> 
> I'm not sure, but that strategy is used in most drivers supporting the
> netpoll API (3c509.c, 8139cp.c, 8139too.c, b44.c, bfin_mac.c,
> bnx2x_main.c, etc.). It also seems to be the way suggested by netpoll
> author, http://oss.sgi.com/archives/netdev/2003-10/msg00800.html.
> 
> disable_irq() only disables the macb IRQ line. Is that an issue for
> printk() execution ?

I'm worried about this potential deadlock:
  macb_interrupt()
  calls printk()
  calls macb_poll_controller()
  calls disable_irq()
  calls synchronize_irq()
  waits until macb_interrupt() returns, which will never happen

So I think it's safer to just use local_irq_save(). The interrupt
handler shouldn't run for long anyway since it uses NAPI for RX
processing, and it probably ought to use NAPI for TX processing too,
eventually.

Haavard

Re: [PATCH] macb: Add support of the netpoll API

[David Miller]

From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Date: Thu, 16 Apr 2009 11:24:35 +0200

> Le Thu, 16 Apr 2009 11:07:53 +0200,
> Haavard Skinnemoen <haavard.skinnemoen@atmel.com> a écrit :
> 
>> Hmm...is this safe? What if printk() is called from the macb interrupt
>> handler?
> 
> I'm not sure, but that strategy is used in most drivers supporting the
> netpoll API (3c509.c, 8139cp.c, 8139too.c, b44.c, bfin_mac.c,
> bnx2x_main.c, etc.). It also seems to be the way suggested by netpoll
> author, http://oss.sgi.com/archives/netdev/2003-10/msg00800.html.
> 
> disable_irq() only disables the macb IRQ line. Is that an issue for
> printk() execution ?

And in response to this Haavard explained the potential deadlock
(sorry I deleted that email) and suggested to use local_irq_save()

But that won't work either, because it doesn't prevent the interrupt
handler from running on other cpus, it only prevents that on the
local cpu.

But I do wonder, because a lot of drivers do in fact use that
disable_irq() scheme to implement ->poll_controller().

I suppose one just needs to be careful about using printk from
the interrupt handler when supporting netpoll.

Re: [PATCH] macb: Add support of the netpoll API

[Haavard Skinnemoen]

David Miller wrote:
> And in response to this Haavard explained the potential deadlock
> (sorry I deleted that email) and suggested to use local_irq_save()
> 
> But that won't work either, because it doesn't prevent the interrupt
> handler from running on other cpus, it only prevents that on the
> local cpu.

The interrupt handler takes bp->lock. Isn't that sufficient to prevent
it from running on other cpus?

If you do local_irq_save() around the interrupt handler, the end result
will be identical to spin_lock_irqsave(), which is what the other
(non-interrupt) parts of the driver use.

Or am I misunderstanding something?

Haavard

Re: [PATCH] macb: Add support of the netpoll API

[David Miller]

From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
Date: Fri, 17 Apr 2009 11:07:03 +0200

> David Miller wrote:
>> And in response to this Haavard explained the potential deadlock
>> (sorry I deleted that email) and suggested to use local_irq_save()
>> 
>> But that won't work either, because it doesn't prevent the interrupt
>> handler from running on other cpus, it only prevents that on the
>> local cpu.
> 
> The interrupt handler takes bp->lock. Isn't that sufficient to prevent
> it from running on other cpus?

We don't necessarily hold bp->lock here, we could be called from
the netpoll code.  Another cpu could easily hold bp->lock and be
in the middle of the interrupt handler.

Re: [PATCH] macb: Add support of the netpoll API

[Haavard Skinnemoen]

David Miller wrote:
> From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> Date: Fri, 17 Apr 2009 11:07:03 +0200
> 
> > David Miller wrote:
> >> And in response to this Haavard explained the potential deadlock
> >> (sorry I deleted that email) and suggested to use local_irq_save()
> >> 
> >> But that won't work either, because it doesn't prevent the interrupt
> >> handler from running on other cpus, it only prevents that on the
> >> local cpu.
> > 
> > The interrupt handler takes bp->lock. Isn't that sufficient to prevent
> > it from running on other cpus?
> 
> We don't necessarily hold bp->lock here, we could be called from
> the netpoll code.  Another cpu could easily hold bp->lock and be
> in the middle of the interrupt handler.

Yes, but we're about to take it. poll_controller() disables interrupts
and calls the interrupt handler, which takes the lock. That's
essentially an open-coded spin_lock_irqsave().

Now, that will obviously deadlock too if we're called from within the
interrupt handler or some other place holding bp->lock...

I guess you're right -- we just have to be careful about calling
printk() from within the driver. But it seems a lot easier to just drop
the lock before dumping an error message than it is to work around that
disable_irq() deadlock. And if you're running with verbose debugging
enabled, you probably shouldn't be using netconsole anyway.

Haavard

Re: [PATCH] macb: Add support of the netpoll API

[David Miller]

From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
Date: Fri, 17 Apr 2009 11:25:53 +0200

> I guess you're right -- we just have to be careful about calling
> printk() from within the driver. But it seems a lot easier to just drop
> the lock before dumping an error message than it is to work around that
> disable_irq() deadlock. And if you're running with verbose debugging
> enabled, you probably shouldn't be using netconsole anyway.

Therefore, do you agree to add Thomas's patch as-is?

If so please provide an Acked-by:

Re: [PATCH] macb: Add support of the netpoll API

[Haavard Skinnemoen]

David Miller wrote:
> From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> Date: Fri, 17 Apr 2009 11:25:53 +0200
> 
> > I guess you're right -- we just have to be careful about calling
> > printk() from within the driver. But it seems a lot easier to just drop
> > the lock before dumping an error message than it is to work around that
> > disable_irq() deadlock. And if you're running with verbose debugging
> > enabled, you probably shouldn't be using netconsole anyway.  
> 
> Therefore, do you agree to add Thomas's patch as-is?

No, therefore, I'd prefer local_irq_save() :-)

When using disable_irq(), there's no way we can call printk() from the
interrupt handler ever. When using local_irq_save() we can call
printk() from the interrupt handler provided that we drop the lock
first.

And since poll_controller() ends up taking the lock in both cases, we
need to be careful about calling printk() while holding the lock
anyway. So local_irq_save() means one less thing that might kill the
driver.

Haavard

Re: [PATCH] macb: Add support of the netpoll API

[David Miller]

From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
Date: Fri, 17 Apr 2009 11:50:11 +0200

> David Miller wrote:
>> From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
>> Date: Fri, 17 Apr 2009 11:25:53 +0200
>> 
>> > I guess you're right -- we just have to be careful about calling
>> > printk() from within the driver. But it seems a lot easier to just drop
>> > the lock before dumping an error message than it is to work around that
>> > disable_irq() deadlock. And if you're running with verbose debugging
>> > enabled, you probably shouldn't be using netconsole anyway.  
>> 
>> Therefore, do you agree to add Thomas's patch as-is?
> 
> No, therefore, I'd prefer local_irq_save() :-)

I'm pretty sure there is a specific reason driver's use
disable_irq() rather than disabling local cpu IRQs.

I just can't remember it at the moment.

And there absolutely must be a reason, because disable_irq()
is a lot more expensive.

Re: [PATCH] macb: Add support of the netpoll API

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Fri, 17 Apr 2009 03:44:16 -0700 (PDT)

> From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> Date: Fri, 17 Apr 2009 11:50:11 +0200
> 
>> David Miller wrote:
>>> From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
>>> Date: Fri, 17 Apr 2009 11:25:53 +0200
>>> 
>>> > I guess you're right -- we just have to be careful about calling
>>> > printk() from within the driver. But it seems a lot easier to just drop
>>> > the lock before dumping an error message than it is to work around that
>>> > disable_irq() deadlock. And if you're running with verbose debugging
>>> > enabled, you probably shouldn't be using netconsole anyway.  
>>> 
>>> Therefore, do you agree to add Thomas's patch as-is?
>> 
>> No, therefore, I'd prefer local_irq_save() :-)
> 
> I'm pretty sure there is a specific reason driver's use
> disable_irq() rather than disabling local cpu IRQs.
> 
> I just can't remember it at the moment.
> 
> And there absolutely must be a reason, because disable_irq()
> is a lot more expensive.

Ok, after doing some research, the reason seems to be that
drivers which have lockless IRQ handlers have to do things
this way to guarentee other cpus are not inside of the
interrupt handler.

Since we are using a lock here, local_irq_save(), or disabling
IRQs in the ethernet chip itself, should work fine.

So I'm OK with the local_irq_save() idea, someone please post
the patch.

[PATCH] macb: Add support of the netpoll API

[Haavard Skinnemoen]

From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

With this patch in place, I'm successfully able to use the netconsole
mechanism with the Calao USB-A9263 board, which uses the AT91SAM9263
CPU, which in terms of Ethernet controller is supported by the macb
driver.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
[haavard.skinnemoen@atmel.com: disable_irq() -> local_irq_save()]
Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
 drivers/net/macb.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index f505010..dd7a1c3 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -615,6 +615,21 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+/*
+ * Polling receive - used by netconsole and other diagnostic tools
+ * to allow network i/o with interrupts disabled.
+ */
+static void macb_poll_controller(struct net_device *dev)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	macb_interrupt(dev->irq, dev);
+	local_irq_restore(flags);
+}
+#endif
+
 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);
@@ -1183,6 +1198,9 @@ static int __init macb_probe(struct platform_device *pdev)
 	dev->do_ioctl = macb_ioctl;
 	netif_napi_add(dev, &bp->napi, macb_poll, 64);
 	dev->ethtool_ops = &macb_ethtool_ops;
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	dev->poll_controller = macb_poll_controller;
+#endif
 
 	dev->base_addr = regs->start;
 
-- 
1.6.0.4

Re: [PATCH] macb: Add support of the netpoll API

[David Miller]

From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
Date: Mon, 20 Apr 2009 12:57:07 +0200

> From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> With this patch in place, I'm successfully able to use the netconsole
> mechanism with the Calao USB-A9263 board, which uses the AT91SAM9263
> CPU, which in terms of Ethernet controller is supported by the macb
> driver.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> [haavard.skinnemoen@atmel.com: disable_irq() -> local_irq_save()]
> Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>

This patch does not apply to the current tree.

The macb driver has been converted over to netdev_ops, and therefore
these accesses to ->poll_controller are not correct.

[PATCH v3] macb: Add support of the netpoll API

[Haavard Skinnemoen]

From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

With this patch in place, I'm successfully able to use the netconsole
mechanism with the Calao USB-A9263 board, which uses the AT91SAM9263
CPU, which in terms of Ethernet controller is supported by the macb
driver.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
[haavard.skinnemoen@atmel.com: disable_irq() -> local_irq_save()]
[haavard.skinnemoen@atmel.com: convert to net_device_ops]
Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
This should apply to the current tree with the net_device_ops change
applied. Sorry about the delay.

 drivers/net/macb.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index e82aee4..7222659 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -599,6 +599,21 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+/*
+ * Polling receive - used by netconsole and other diagnostic tools
+ * to allow network i/o with interrupts disabled.
+ */
+static void macb_poll_controller(struct net_device *dev)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	macb_interrupt(dev->irq, dev);
+	local_irq_restore(flags);
+}
+#endif
+
 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);
@@ -1094,6 +1109,9 @@ static const struct net_device_ops macb_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_change_mtu		= eth_change_mtu,
 	.ndo_set_mac_address	= eth_mac_addr,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	.ndo_poll_controller	= macb_poll_controller,
+#endif
 };
 
 static int __init macb_probe(struct platform_device *pdev)
-- 
1.6.0.4

Re: [PATCH v3] macb: Add support of the netpoll API

[David Miller]

From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
Date: Mon,  4 May 2009 13:54:44 +0200

> From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> With this patch in place, I'm successfully able to use the netconsole
> mechanism with the Calao USB-A9263 board, which uses the AT91SAM9263
> CPU, which in terms of Ethernet controller is supported by the macb
> driver.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> [haavard.skinnemoen@atmel.com: disable_irq() -> local_irq_save()]
> [haavard.skinnemoen@atmel.com: convert to net_device_ops]
> Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>

Applied to net-next-2.6, thanks!

Re: [PATCH] macb: Add support of the netpoll API

[Haavard Skinnemoen]

David Miller wrote:
> From: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> Date: Fri, 17 Apr 2009 11:50:11 +0200
> 
> > No, therefore, I'd prefer local_irq_save() :-)
> 
> I'm pretty sure there is a specific reason driver's use
> disable_irq() rather than disabling local cpu IRQs.

Looks like most if not all of the ARM drivers use local_irq_save()...I
couldn't figure out what etherh does from a quick scan.

> I just can't remember it at the moment.
> 
> And there absolutely must be a reason, because disable_irq()
> is a lot more expensive.

Sounds like another reason _not_ to use it ;-)

Well, if you're really sure about this, I guess you might as well go
ahead and apply it. But it feels wrong to Ack the patch when I don't
know the reason for using disable_irq() and can think of several
reasons _not_ to use it...

Haavard