* [PATCH] net: pxa168_eth: add netconsole support
@ 2018-01-27 20:29 Alexander Monakov
2018-01-29 19:31 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Monakov @ 2018-01-27 20:29 UTC (permalink / raw)
To: netdev
Cc: Alexander Monakov, Russell King, Sebastian Hesselbarth,
Florian Fainelli
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] net: pxa168_eth: add netconsole support
2018-01-27 20:29 [PATCH] net: pxa168_eth: add netconsole support Alexander Monakov
@ 2018-01-29 19:31 ` David Miller
2018-01-31 14:05 ` Alexander Monakov
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2018-01-29 19:31 UTC (permalink / raw)
To: amonakov; +Cc: netdev, rmk+kernel, sebastian.hesselbarth, f.fainelli
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: pxa168_eth: add netconsole support
2018-01-29 19:31 ` David Miller
@ 2018-01-31 14:05 ` Alexander Monakov
2018-01-31 15:20 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Monakov @ 2018-01-31 14:05 UTC (permalink / raw)
To: David Miller; +Cc: netdev, rmk+kernel, sebastian.hesselbarth, f.fainelli
> > +#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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: pxa168_eth: add netconsole support
2018-01-31 14:05 ` Alexander Monakov
@ 2018-01-31 15:20 ` David Miller
2018-02-01 19:45 ` [PATCH v2] " Alexander Monakov
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2018-01-31 15:20 UTC (permalink / raw)
To: amonakov; +Cc: netdev, rmk+kernel, sebastian.hesselbarth, f.fainelli
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] net: pxa168_eth: add netconsole support
2018-01-31 15:20 ` David Miller
@ 2018-02-01 19:45 ` Alexander Monakov
2018-02-01 20:00 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Monakov @ 2018-02-01 19:45 UTC (permalink / raw)
To: David Miller; +Cc: netdev, rmk+kernel, sebastian.hesselbarth, f.fainelli
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] net: pxa168_eth: add netconsole support
2018-02-01 19:45 ` [PATCH v2] " Alexander Monakov
@ 2018-02-01 20:00 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2018-02-01 20:00 UTC (permalink / raw)
To: amonakov; +Cc: netdev, rmk+kernel, sebastian.hesselbarth, f.fainelli
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-01 20:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-27 20:29 [PATCH] net: pxa168_eth: add netconsole support Alexander Monakov
2018-01-29 19:31 ` David Miller
2018-01-31 14:05 ` Alexander Monakov
2018-01-31 15:20 ` David Miller
2018-02-01 19:45 ` [PATCH v2] " Alexander Monakov
2018-02-01 20:00 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).