Subject: Re: [PATCH 8/9] pcnet32: NAPI implementation.

From: Don Fry <[email-masked]>
Date: Thu, 29 Jun 2006 16:41:04 -0700

On Fri, Jun 30, 2006 at 12:24:38AM +0200, Francois Romieu wrote:
> Nit below.
> 
> Don Fry <[email-masked]> :
> [...]
> > --- linux-2.6.17-git13/drivers/net/purge.pcnet32.c	Thu Jun 29 13:28:24 2006
> > +++ linux-2.6.17-git13/drivers/net/pcnet32.c	Thu Jun 29 13:28:31 2006
> [...]
> > +#ifdef CONFIG_PCNET32_NAPI
> > +static int pcnet32_poll(struct net_device *dev, int *budget)
> > +{
> [...]
> > +	netif_rx_complete(dev);
> > +
> > +	spin_lock_irqsave(&lp->lock, flags);
> 
> No need to save/restore (it's true in pcnet32_{get_regs/suspend} too).

This lock is taken by the interrupt handler and my reading of
spinlocks.txt says I do need to use spin_lock_irqsave unless I
misunderstand.  The only spin_lock() is in the interrupt handler itself,
all others are spin_lock_irqsave.

> 
> > +
> > +	/* clear interrupt masks */
> > +	val = lp->a.read_csr(ioaddr, CSR3);
> > +	val &= 0x00ff;
> > +	lp->a.write_csr(ioaddr, CSR3, val);
> > +
> > +	/* Set interrupt enable. */
> > +	lp->a.write_csr(ioaddr, CSR0, CSR0_INTEN);
> 
> Insert mmiowb():

Is this required in addition to the two outw() in write_csr?  There are
rmb() and wmb() in the places that need them, but no mmiowb() anywhere.
What are the factors for when mmiowb needs to be inserted?

> 
> > +
> > +	spin_unlock_irqrestore(&lp->lock, flags);
> 
> [...]
> > @@ -2413,183 +2608,41 @@ pcnet32_interrupt(int irq, void *dev_id,
> >  				       dev->name, csr0);
> >  			/* unlike for the lance, there is no restart needed */
> >  		}
> > -
> > -		if (must_restart) {
> > +#ifdef CONFIG_PCNET32_NAPI
> > +		if (netif_rx_schedule_prep(dev)) {
> > +			u16 val;
> > +			/* set interrupt masks */
> > +			val = lp->a.read_csr(ioaddr, CSR3);
> > +			val |= 0x5f00;
> > +			lp->a.write_csr(ioaddr, CSR3, val);
> 
> Insert mmiowb();
> 
> -- 
> Ueimor

-- 
Don Fry
[email-masked]