Re: [patch 7/8] lock validator: fix ns83820.c irq-flags bug

[Arjan van de Ven <arjan@linux.intel.com>]

Jeff Garzik wrote:
> Arjan van de Ven wrote:
>> Jeff Garzik wrote:
>>> The driver's locking is definitely wrong, but I don't think this is 
>>> the fix, 
>>
>> it's an obvious correct fix in the correctness sense though...
>>
>>>
>>> Jesus, the locking here is awful.  No wonder there are bugs.
>>
>>
>> ... which given that fact, is for 2.6.17 probably the right thing, 
>> pending
>> a nicer fix for 2.6.18
> 
> I disagree, the patch is wrong too.

wrong as in "not quite optimal", not wrong as in "buggy".

> For normal PCI hardware, the in-ISR paths should all use spin_lock(), 

only for per hardware locks obviously, not for per driver locks ;)


you are right that it's a lot nicer to do what you describe. No argument
from me on that part. But to call it "wrong" or "incorrect" is not quite
ok.  In terms of changing/fixing the approach we did was the simplest one.
Not the "make it look nice" one. Fix it by making the bug go away in the light
of a LOT of fishy locking.

You can demand that we first fix all the fishy locking first, and I can even
in part agree with that, but for -stable and 2.6.17 that is obviously out
of scope while a simple "make the bug go away" fix is not.