On Fri, Jun 30, 2006 at 12:24:38AM +0200, Francois Romieu wrote:
> Nit below.
>
> Don Fry <[EMAIL PROTECTED]> :
> [...]
> > --- 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 PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
