On Wed, Apr 16, 2008 at 12:41:44PM -0400, M. Asselstine wrote:
> I definitely see the complexity and respect it. With some careful
> thought and guidance however do you not feel we can finish this up?
I do, now the task is so small ;-)
> > ok ... the three occurrences in nicstar look like they can be deleted.
> > Replacing 'restore_flags(nsdsf);' with local_irq_restore(flags) would be
> > a nice touch, but to be honest the generic lockdep code we have these
> > days should do even better, so killing the nicstar NS_DEBUG_SPINLOCKS
> > code would be a good idea.
> >
> I am thinking this is a good strategy. There is really no need for the
> NS_DEBUG_SPINLOCKS code as far as I can tell even any documentation
> for NS_DEBUG_SPINLOCKS use seems to of been pulled from the header.
We're in agreement then. Please propose a patch.
> > boardergo looks like it needs its locking reviewed. It has the
> > card->hysdn_lock already, so I don't quite understand what the sti()
> > and cli() are doing in ergo_irq_bh()
> >
> It looks like Thomas Gleixner and Amol Lad did the initial legwork
> (2bd7e20e0d24325b0799544bc8105cc57cc8e2aa and
> 0d9ba869e103d91d471146378ad85bf1fb8e74b4) but a bunch of sti() and one
> cli() survived. This is going to be a tough one to finish as I do not
> have any way to test it.
I don't think anyone else does either. It's going to require careful
analysis of the driver to check they're simply obsolete. It might be
worth asking Thomas and Amol why they were left (it may be a simple
oversight).
> > dmascc appears to be calling cli() while it's already in interrupt
> > context. If so, it already has the register_lock and so this is
> > unnecessary. Someone should dig into the driver to verify that this
> > is true.
>
> Upon examination the only existing cli()/sti() is found in
> start_timer() which as you say is called only when we are already in
> an interrupt (scc_isr()) which already has the register_lock with the
> exception of one call to start_timer() in scc_send_packet() which
> again already has the register_lock. So I think these instances will
> be safe to remove.
Great! Please send a patch.
> > xircom_tulip_cb also has a per-device spinlock. Again, someone needs to
> > verify why the various places calling cli() aren't grabbing the spinlock.
>
> This one looks like it was just neglected and the save_flags();cli()
> were just never moved over to
> spin_lock_irqsave()/spin_unlock_irqrestore(). Do you not think this
> would be a straight forward translation? as it looks like many other
> places in the kernel these were done with only compile testing.
Compile testing and a great deal of thinking. For example, outl_CSR6() is
called from xircom_init_one() a few lines before we call spin_lock_init(),
so grabbing the spinlock in outl_CSR6() would cause a lockup. Also, the
device isn't open at this point, so the interrupt routine can't be
called.
But simply deleting 'cli()' might cause trouble when it's called from,
for example xircom_tx_timeout().
> There are a few in sound/oss, would these be of interest or is this
> code maintained outside of the kernel?
I think you're wrong:
$ grep -wr cli sound
sound/oss/dmabuf.c:/* No kernel lock - DMAbuf_activate_recording protected by global cli/sti */
sound/oss/midibuf.c: /* yes, think the same, so I removed the cli() brackets
sound/oss/midibuf.c: /* yes removed the cli() brackets again
sound/oss/midi_synth.c: cli();
Initially, that looks like only one user, but examining the context:
/* save_flags(flags);
cli();
don't know against what irqhandler to protect*/
it's actually commented out. So I don't see any users in sound/oss.
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
