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

Michael Schmitz
2012-04-05T09:44:35Z

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