On Sat, 2008-05-10 at 20:18 -0700, David Brownell wrote:
> On Saturday 10 May 2008, Johannes Berg wrote:
> > This is probably my mistake.
> >
> > However, I took __releases and __acquires to mean that this function
> > *changed* the context, doing both doesn't really make much sense. I
> > think the function should actually be declared
> >
> > static void
> > finish_urb(...)
> > __requires(ohci->lock)
> > {...}
> >
> > where __requires is (for sparse) defined as
> >
> > #define __requires(x) __attribute__((context(x,1,1)))
>
> ISTR suggesting special syntax for this to Linus (this was way
> back when "sparse" was just starting) and he wanted to just do
> it by having those two attributes.
>
> So at this point, I'd want to see the regression fixed (and the
> tests updated to avoid this in the future) before exploring any
> alternative syntax for kernel annotations.
Yeah, true, on the other hand, if/when Josh merges my other patches then
most kernel annotations will have to be changed anyway because up to
before the patch that is already in sparse wouldn't flag this:
spin_lock(&lock);
rcu_read_unlock();
because it didn't care *at all* about the expression inside __acquire()
and thus a lot of usage crept in that isn't really usable.
With my future patches, I'm even binding the symbols, so that
void spin_lock(spinlock_t l) __acquire(l);
will flag errors with
spin_lock(&a);
spin_unlock(&b);
while right now it would just treat them both as the context "l".
So I expect that it will not be possible to not "regress" in that sense
because the current annotations are simply messed up since sparse didn't
care about the name inside __acquire()/__release() at all!
However, it's probably fairly easy right now to treat both of them as
being merged together which seems the only sensible things to do anyway
if such a situation is encountered. I'll look into it.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
