Re: [PATCH] Re: Q: natsemi.c spinlocks

[Donald Becker]

On Sun, 21 Jan 2001, Jeff Garzik wrote:
> Andrew Morton wrote:
> > Manfred wrote:
> > > Hi Jeff, Tjeerd,
> > > I spotted the spin_lock in natsemi.c, and I think it's bogus.
> > >
> > > The "simultaneous interrupt entry" is a bug in some 2.0 and 2.1 kernel
> > > (even Alan didn't remember it exactly when I asked him), thus a sane
> > > driver can assume that an interrupt handler is never reentered.
...
> > I think you're right.  2.4's interrupt handling prevents
> > simultaneous entry of the same ISR.

The bug (simultaneous calls to the interrupt handler on SMP) existed in
most 2.0 versions was fixed before 2.2.  A driver that needs to work
with multiple kernel versions must have the check.

> > However, natsemi.c's spinlock needs to be retained, and
> > extended into start_tx(), because this driver has
> > a race which has cropped up in a few others:
> >         ...
> >         if (np->cur_tx - np->dirty_tx >= TX_QUEUE_LEN - 1) {
> >         /* WINDOW HERE */
> >                 np->tx_full = 1;
> >                 netif_stop_queue(dev);
> >         }
> > If the ring is currently full and an interrupt comes in
> > at the indicated window and reaps ALL the packets in the
> > ring, the driver ends up in state `tx_full = 1' and tramsmit
> > disabled, but with no outstanding transmit interrupts.

The better solution, which I've been adding to the drivers, is to check
again for a just-cleared Tx queue after setting tx_full.
That trades an extra comparison on a rarely followed path for a spinlock
that is taken for every transmit and interrupt.

Remember: spinlocks are expensive!

Donald Becker				[e-mail]
Scyld Computing Corporation		http://www.scyld.com
410 Severn Ave. Suite 210		Second Generation Beowulf Clusters
Annapolis MD 21403			410-990-9993