[PATCH v2] net: pxa168_eth: add netconsole support

[Alexander Monakov]

This implements ndo_poll_controller callback which is necessary to
enable netconsole.

Signed-off-by: Alexander Monakov <amona...@ispras.ru>
Cc: Russell King <rmk+ker...@arm.linux.org.uk>
Cc: Sebastian Hesselbarth <sebastian.hesselba...@gmail.com>
Cc: Florian Fainelli <f.faine...@gmail.com>
---

Here's the revised patch that performs interrupt disabling correctly
as far as I can tell. It's still unclear to me why some drivers use
local_irq_save/restore:

1. netconsole must have already disabled local interrupts, and netpoll
   verifies that; so local_irq_save in ndo_poll_controller must be a nop.

2. local disabling does not ensure that we wouldn't race with the irq
   handler in progress on some other cpu (but disable_irq does).

Please let me know if I'm missing something.

Thanks.
Alexander

 drivers/net/ethernet/marvell/pxa168_eth.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c 
b/drivers/net/ethernet/marvell/pxa168_eth.c
index 7bbd86f08e5f..3a9730612a70 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -1362,6 +1362,15 @@ static int pxa168_eth_do_ioctl(struct net_device *dev, 
struct ifreq *ifr,
return -EOPNOTSUPP;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static void pxa168_eth_netpoll(struct net_device *dev)
+{
+   disable_irq(dev->irq);
+   pxa168_eth_int_handler(dev->irq, dev);
+   enable_irq(dev->irq);
+}
+#endif
+
 static void pxa168_get_drvinfo(struct net_device *dev,
   struct ethtool_drvinfo *info)
 {
@@ -1390,6 +1399,9 @@ static const struct net_device_ops pxa168_eth_netdev_ops 
= {
.ndo_do_ioctl   = pxa168_eth_do_ioctl,
.ndo_change_mtu = pxa168_eth_change_mtu,
.ndo_tx_timeout = pxa168_eth_tx_timeout,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+   .ndo_poll_controller= pxa168_eth_netpoll,
+#endif
 };
 
 static int pxa168_eth_probe(struct platform_device *pdev)
-- 
2.11.0

Re: [PATCH] net: pxa168_eth: add netconsole support

[Alexander Monakov]

> > +#ifdef CONFIG_NET_POLL_CONTROLLER
> > +static void pxa168_eth_netpoll(struct net_device *dev)
> > +{
> > +   struct pxa168_eth_private *pep = netdev_priv(dev);
> > +   napi_schedule(>napi);
> > +}
> > +#endif
> 
> This definitely is not sufficient.
> 
> Look at what other drivers do.

Sorry, I did that, and got confused because some drivers bracket the interrupt
handler with disable_irq/enable_irq, some other drivers use local_irq_save/
local_irq_restore (which seems like a no-op because netconsole already uses
spin_lock_irqsave and netpoll_send_udp checks irqs_disabled), and a few drivers
call bare napi_schedule.

> You have to invoke the interrupt handler with the device's interrupts 
> disabled,

Is this required for correctness? I have to admit I'm unsure why.

> collect the events, and most importantly mask chip interrupts before 
> scheduling
> the NAPI instance.

And is this a matter of efficiency (not calling napi_schedule when there's
nothing to do and not keeping interrupts enabled for its duration), or also
a matter of correctness?

Sorry I'm not sending a revised patch yet, but without a firm understanding
of why changes are needed that would be a bit of a sin.

Thanks.
Alexander

[PATCH] net: pxa168_eth: add netconsole support

[Alexander Monakov]

This implements ndo_poll_controller callback which is necessary to
enable netconsole.

Signed-off-by: Alexander Monakov <amona...@ispras.ru>
Cc: Russell King <rmk+ker...@arm.linux.org.uk>
Cc: Sebastian Hesselbarth <sebastian.hesselba...@gmail.com>
Cc: Florian Fainelli <f.faine...@gmail.com>
---
Hello,

I'm using this to enable netconsole on a consumer device built around the
Marvell Berlin BG2CD SoC.

Thanks.
Alexander

 drivers/net/ethernet/marvell/pxa168_eth.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c 
b/drivers/net/ethernet/marvell/pxa168_eth.c
index 7bbd86f08e5f..6a188f7b426a 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -1362,6 +1362,14 @@ static int pxa168_eth_do_ioctl(struct net_device *dev, 
struct ifreq *ifr,
return -EOPNOTSUPP;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static void pxa168_eth_netpoll(struct net_device *dev)
+{
+   struct pxa168_eth_private *pep = netdev_priv(dev);
+   napi_schedule(>napi);
+}
+#endif
+
 static void pxa168_get_drvinfo(struct net_device *dev,
   struct ethtool_drvinfo *info)
 {
@@ -1390,6 +1398,9 @@ static const struct net_device_ops pxa168_eth_netdev_ops 
= {
.ndo_do_ioctl   = pxa168_eth_do_ioctl,
.ndo_change_mtu = pxa168_eth_change_mtu,
.ndo_tx_timeout = pxa168_eth_tx_timeout,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+   .ndo_poll_controller= pxa168_eth_netpoll,
+#endif
 };
 
 static int pxa168_eth_probe(struct platform_device *pdev)
-- 
2.11.0