Re: [RFT] pcnet32 NAPI changes

[Jon Mason <jdmason@us.ibm.com>]

On Tue, Jun 20, 2006 at 10:48:07AM -0400, Lennart Sorensen wrote:
> On Tue, Jun 20, 2006 at 08:53:55AM -0500, Jon Mason wrote:
> > The amount of polls per received packet is very low, thus removing the
> > benefit of NAPI.  A compile time option would allow those users who know
> > better to DTRT.
> 
> Well I know on the slow poke system I run on, with the napi polling, the
> system can process packets, and get work done, and not fall over and die
> from handling interrupts.  Without it, even 70Mbit of data on a single
> port will flood the system with packet overruns to the point the
> watchdog times out and the system reboots.  So I don't know if polling
> is slightly more inefficient with little traffic, it is certainly a lot
> more efficient and safer when there is suddenly a lot more traffic.
> Maybe it should be a module option, so that you can pick what you want.
> Heck it could be a per port option even. :)

The point of my comment was CPU utilization.

It appears that a bug is trying to be fixed by adding NAPI. This
sounds a bit hackish to me, and could hide the root cause of the
problem. So I'm not sure that is the best idea, but I will defer to
the maintainer.

> 
> > Yup, but the "everyone else is doing it" argument never worked with my
> > parents. All it takes is one brave soul to determine the reasoning
> > behind the magic numbers and convert them into #define's.  Shouldn't be
> > more than one day's work.
> 
> Is this a magic number in your opinion?
> 
> lp->a.write_csr(ioaddr, 0, 0x0002);          /* Set STRT bit */
> 
> I guess one could do
> #define CSR0_RST 0x0001
> #define CSR0_STRT 0x0002
> #define CSR0_STOP 0x0004
> etc...
> 
> and then
> lp->a.write_csr(ioaddr, 0, CSR0_STRT);         /* Set STRT bit */
> 
> Does that help?  I am not sure.  I think the comment behind it is
> plenty.

But your example is just one instance.  Here is one without a comment:

lp->a.write_csr(ioaddr, 4, 0x0915);

What is it doing?  Is it still needed?  Can it be done anywhere else?  
Who knows, because it is magic.  The 4 can be defined as CSR0_STOP, per
your example above, but what does value 0x0915 do?

My point was that there are certain parts of the code which are
non-intuative and should be commented and there are others which a
good descrptive value would be nice.

> 
> Len Sorensen