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
RTFM.
> >+ 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
combined.
> >+#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,
though :(
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.
> Jeff
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
linux-2.6.9-rc1-sungem-napi-0.02.patch
Description: Text document
signature.asc
Description: Digital signature
|