[PATCH] net: pxa168_eth: add netconsole support

[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 <amonakov@ispras.ru>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Florian Fainelli <f.fainelli@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(&pep->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

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

[David Miller]

From: Alexander Monakov <amonakov@ispras.ru>
Date: Sat, 27 Jan 2018 23:29:07 +0300

> @@ -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(&pep->napi);
> +}
> +#endif

This definitely is not sufficient.

Look at what other drivers do.

You have to invoke the interrupt handler with the device's interrupts disabled,
collect the events, and most importantly mask chip interrupts before scheduling
the NAPI instance.

Thank you.

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(&pep->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

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

[David Miller]

From: Alexander Monakov <amonakov@ispras.ru>
Date: Wed, 31 Jan 2018 17:05:47 +0300 (MSK)

> 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?

If you don't mask interrupts properly around scheduling and finishing
NAPI poll, you can lose events so it is a matter of correctness.

[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 <amonakov@ispras.ru>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Florian Fainelli <f.fainelli@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 v2] net: pxa168_eth: add netconsole support

[David Miller]

From: Alexander Monakov <amonakov@ispras.ru>
Date: Thu, 1 Feb 2018 22:45:17 +0300 (MSK)

> This implements ndo_poll_controller callback which is necessary to
> enable netconsole.
> 
> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>

Looks good, applied.