Re: [PATCH] pcnet32 driver NAPI support

[Lennart Sorensen <lsorense@csclub.uwaterloo.ca>]

On Wed, Jun 07, 2006 at 11:20:40AM -0700, Don Fry wrote:
> I am also working on a NAPI version of the pcnet32 driver for many of
> the same reasons, and will compare what you have with my own
> implementation.  I probably won't be able to do much until Friday.  
> 
> Just a couple of comments.  I am adding netdev@vger.kernel.org to the cc
> list, as most network driver discussion is done here rather than lkml.
> linux-kernel (and linux-net) should be deleted in future replies.

I must have picked the wrong place to cc.

> The 2.6.17-rc6 would be the correct source to patch against.  Since this
> is an enhancement it will not come out till 2.6.18.

I thought so.  That is why I did it against both 2.6.17-rc6 and 2.6.16
(since I use it with 2.6.16).

> I would not change the driver name from pcnet32 to pcnet32napi, but I
> would changes the version from 1.32 to 1.33NAPI or something like that.

Hmm, perhaps.  I just wanted something that made it obvious in dmesg
which driver I was running.  I see tulip actually does put it in the
version instead.  I don't remember where I got the driver name change
idea from.

> Some areas of concern that you may have addressed already, I have not
> scanned your changes yet, are what happens if the ring size is changed
> without bringing down the interface (via ethtool), or if the loopback
> test is run in a similar fashion, or a tx timeout occurs.

The same thing as if it was done before enabling napi.  From a few
messages I have seen, it doesn't work right now, and it won't work any
better with my changes.  I have never tried changing the ring size on
the fly, so I don't know.

It appears that the port is stopped before the ring size change is done,
although I can't really tell how it handles things if the queue is not
empty when it stops the port.  Does it try to handle anything left in
the ring first or does it just toss those packets? (That I would
consider wrong).

> The lp->lock MUST be held whenever accessing the csr or bcr registers as
> this is a multi-step process, and has been the source of problems in the
> past.  Even on UP systems.

Hmm, I just followed what appeared to be in pcnet32_rx and how tulip and
a few other drivers had done their napi conversions.  It certainly works
for me the way I did it.  Haven't seen any lockups yet.  I do see that I
am not holding the lock when I acknowledge IRQs in pcnet32_poll, which
pcnet32_rx doesn't need to worry about since it is called from the
interrupt handler which already holds the lock.  That should be fixed
then.

So I can do:
        // Clear RX interrupts
        spin_lock(&lp->lock);
        lp->a.write_csr (ioaddr, 0, 0x1400);
        spin_unlock(&lp->lock);
That part seems simple enough to protect.

Is this safe without holding the lock?
    } while(lp->a.read_csr (ioaddr, 0) & 0x1400);
Not sure how to wrap a lock around that one without holding the lock for
way too long.

perhaps:
        spin_lock(&lp->lock);
	state=lp->a.read_csr (ioaddr, 0) & 0x1400;
        spin_unlock(&lp->lock);
    } while(state);
Does that seem more reasonable?

Len Sorensen
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/