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

Hi Paul,

I hate to bring this up again after such a long delay - my usual 
slackness is to blame, sorry.

Geert has reminded me that the IRQ-less ne.c driver is one of the few 
remaining unmerged patches in his queue.
We had resolved the polling from timer interrupt issues in the m68k 
interrupt code, with a little help from Thomas Gleixner (commit 
b39898cd4077f4b6ec706e717c938751c34e1dc4) so changes to ne.c are now 
minimal.

Are you willing to review this minimal patch again?
Regards,

    Michael Schmitz

On 12-04-05 05:28 AM, Geert Uytterhoeven wrote:
  

quoted

On Wed, Apr 4, 2012 at 22:46, Paul Gortmaker
[off-list ref] wrote:
    

quoted

On 12-04-01 04:49 AM, Michael Schmitz wrote:
      

quoted

quoted

And on re-reading the comments in the other part of the patch, i.e.
"...emulates the card interrupt via a timer"  --perhaps the driver
should be just fixed to support generic netpoll, instead of adding
an arch specific thing that amounts to netpoll.  Then anyone can
attempt to limp along and use one of these ancient relics w/o IRQ.

          

Here's take two of my patch to convert the m68k Atari ROM port Ethernet
driver to use mainstream ne.c, with minimal changes to the core NE2000
code.

In particular:

Changes to core net code:
* add a platform specific IRQ flag, so ne.c can share a hardware or
timer interrupt with some other interrupt source.

Changes to arch/m68k code:
* register the 8390 platform device on Atari only if the hardware is present
* retain the old driver (atari_ethernec.c in Geert's tree) under a
different config option, to be removed soon.

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.
      

One other thing we could do is increase CONFIG_HZ to 250.

    

quoted

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.
      

From my point of view, "remove soon" means it will never hit mainline.
    

Can you clarify what "it" is?   It isn't clear to me if you
mean the _removal_ will never hit mainline, or the transient
renamed "old" driver will never hit mainline.

If the former, then there is no point pursuing this any further
as I said above.

If the latter, then the commit sent out for review should have
no instances of this "renaming to old" related changes.

Thanks,
Paul.

  

quoted

quoted

quoted

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
      

Indeed, with a small modification (keep multi-platform kernels in mind):

#ifdef CONFIG_ATARI
#define EI_IRQ_FLAGS    (MACH_IS_ATARI ? IRQF_SHARED : 0)
#else
#define EI_IRQ_FLAGS    0
#endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds