Re: [PATCH] m68k/atari: EtherNEC - rewrite to use mainstream ne.c, take two

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


Hi Paul,

(Apologies to all for botching the patch format ...)

Regarding your suggestion that netpoll be used instead of a dedicated timer interrupt: no changes to ne.c or 8390p.c are required to use netpoll, it all works out of the box. All that is needed to use the driver with netpoll is setting the device interrupt to some source that can be registered, and enabling CONFIG_NETPOLL. Interrupt rate and hence throughput is lower with netpoll though, which is why I still prefer the dedicated timer option.

How much lower?  Enough to matter?  Implicit in that question is
the assumption that this is largely a hobbyist platform and nobody
is using it in a closet to route gigabytes of traffic.
I'd say about at least double latency. I can try and measure bulk data rates if it matters. My gut feeling is latency limits data rates even when say behind a DSL modem for downloads. It sure did when my Falcon was still hooked up to a university network, uploading and downloading source and binary packages for Debian/68k.

Of course you're not routing gigabytes of traffic with this (where to - a PPP connection? :). Whoever wants minimum latency better reach for the soldering iron and wire up the interrupt line to some suitable input.
Also, the only advantage to modifying ne.c is to allow dumping
the old driver.  What is the "remove soon" plan?  Any reason
for it to not be synchronous?  That would eliminate the Kconfig
churn and the introduction of the _OLD option.  Modifying ne.c
and then deciding to keep the old driver because it is "faster"
would make this change pointless.
As soon as eventual changes to ne.c get accepted. If you want us to drop the old driver in the same patch, fine by me.
diff --git a/drivers/net/ethernet/8390/8390.h b/drivers/net/ethernet/8390/8390.h
index ef325ff..9416245 100644
--- a/drivers/net/ethernet/8390/8390.h
+++ b/drivers/net/ethernet/8390/8390.h
@@ -32,6 +32,14 @@ extern void ei_poll(struct net_device *dev);
 extern void eip_poll(struct net_device *dev);
 #endif
+/* Some platforms may need special IRQ flags */
+#if IS_ENABLED(CONFIG_ATARI_ETHERNEC)
+#  define EI_IRQ_FLAGS    IRQF_SHARED
+#endif
+
+#ifndef EI_IRQ_FLAGS
+#  define EI_IRQ_FLAGS    0
+#endif

This seems more klunky than it needs to be.  If we assume that anyone
building ne.c on atari is hence trying to drive an ethernec device
than it can just be

#ifdef CONFIG_ATARI
#define EI_IRQ_FLAGS	IRQF_SHARED
#else
#define EI_IRQ_FLAGS	0
#endif

Pretty safe assumption - if we further assume no other arch has reason to resort to such a kludge, we can simplify it this way.
--- a/drivers/net/ethernet/8390/ne.c
+++ b/drivers/net/ethernet/8390/ne.c
@@ -165,7 +165,8 @@ bad_clone_list[] __initdata = {
 #if defined(CONFIG_PLAT_MAPPI)
 #  define DCR_VAL 0x4b
 #elif defined(CONFIG_PLAT_OAKS32R)  || \
-   defined(CONFIG_MACH_TX49XX)
+   defined(CONFIG_MACH_TX49XX) || \
+   IS_ENABLED(CONFIG_ATARI_ETHERNEC)

Rather than use IS_ENABLED on a driver setting, you can follow
the surrounding context and use defined(CONFIG_ATARI) -- i.e.
work off a platform setting.
True as well, point taken. Is the patch acceptable with these changes? If so, would you be OK with this going through Geert's tree?

Cheers,

 Michael

--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Video for Linux]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Video Projectors]     [Linux Kernel]     [Linux SCSI]     [XFree86]

Add to Google Powered by Linux