Hi Jeff, thanks for your comments.
On Tue, Sep 14, 2004 at 11:56:08AM -0400, Jeff Garzik wrote:
> >+static inline void
> >+gem_irq_acknowledge(struct gem *gp, unsigned int mask)
> >+ writel(mask, gp->regs + GREG_IACK);
> PCI posting
I'm not sure whether I need mb() or wmb() ?
Or even better: Can anybody direct me to a document describing the
primitives in more detail than the linux include files? I'm happy to
> >+ gem_irq_acknowledge(gp, GREG_STAT_RXNOBUF);
> >+ gem_irq_acknowledge(gp, GREG_STAT_RXTAGERR);
> >+ gem_irq_acknowledge(gp, GREG_STAT_TXALL|GREG_STAT_TXINTME);
> Are these in separate functions? If not, it's usually best to go ahead
> and ACK all the interrupts you are going to handle, in a single IO write
> (usually _before_ you handle the events).
Not all of them are in the same function, although two of them can be
> >+#ifdef CONFIG_SUNGEM_NAPI
> >+ netif_receive_skb(skb);
> >+ (*work_done)++;
> minor nit: surely it's better code to increment a local variable, then
> store the local to *work_done once all iterations are complete?
That's actually copy&paste from e1000_main.c ;)
Introducing additional local variables would add #ifdef'ed declarations,
Would you prefer a tg3-like rx() function that returns work_done rather
than making it a return argument?
> >- u32 gem_status = readl(gp->regs + GREG_STAT);
> >+ u32 gem_status = readl(gp->regs + GREG_STAT2);
> what does this do?
GREG_STAT implicitly clears event bits 0..6 on read, which according to
NAPI-HOWTO requires that you move all interrupt processing into dev->poll().
GREG_STAT2 is read-only without implicit clear, that's why I've added
the explicit gem_irq_acknowledge() all over the place.
> are all these enable/disables inside locks?
No, enable() is also called from within the dev->poll() function, which
doesn't grab the lock.
Partially revised patch attached
- Harald Welte <laforge@xxxxxxxxxxxx> http://www.gnumonks.org/
Programming is like sex: One mistake and you have to support it your lifetime
Description: Text document
Description: Digital signature