Re: [PATCH 1/3] make sparse keep its promise about context tracking

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]


> >     static void *dev_mc_seq_start(struct seq_file *seq, loff_t * pos)
> >             __acquires(dev_base_lock)
> >     {
> >             [...]
> >             __acquire__(dev_base_lock);
> >             read_lock(&dev_base_lock);
> >             [...]
> >     }
> 
> I don't understand why you count this as wrong.  read_lock should
> handle acquiring the context itself, either via an __acquires
> annotation if written as a function, or via an __acquire__ statement
> if written as a macro.  Callers of read_lock shouldn't need to
> explicitly call __acquire__.

Well, the question is how you want to name things. What I did is that
the context identifier is just a name and bears no other relation to the
code. Hence, read_lock() can't say that it acquires 'dev_base_lock'
because it doesn't know what the name should be.

With a slightly different sparse patch than mine you could declare
read_lock() something like this:

#define read_lock(x) do { \
	__acquire__(x); \
	__read_lock((x)); \
} while (0)

but then you'd have different names everywhere, say somebody passed
'&dev_base_lock' and somebody else used

readlock_t *dbl = &dev_base_lock;
read_lock(dbl)

and you'd end up with one context named "dbl" and another one named
"&dev_base_lock".

If you have suggestions on how to solve this I'm all ears, so far I
decided it wasn't worth it and opted for explicitly naming all the
contexts.

(with my patch the above would just create a context called "x")

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]