Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: Being more careful about iospace accesses..
Original-Message-ID: <Pine.LNX.4.58.0409151546400.2333@ppc970.osdl.org>
Date: Wed, 15 Sep 2004 23:37:49 GMT
Message-ID: <fa.gue1uua.828rpm@ifi.uio.no>

[ Subject line changed to avoid getting caught as spam ;]

On Wed, 15 Sep 2004, Roland Dreier wrote:
>
> Linus, while we're on the subject of new sparse checks, could you give
> a quick recap of the semantics of the new __leXX types (and what
> __bitwise means to sparse)?  I don't think I've ever seen this stuff
> described on LKML.

[ The bitwise checks are actually by Al Viro, but I'll explain the basic
  idea. Al is Cc'd so that he can add any corrections or extensions. ]

Sparse allows a number of extra type qualifiers, including address spaces
and various random extra restrictions on what you can do with them. There
are "context" bits that allow you to use a symbol or type only in certain
contexts, for example, and there are type qualifiers like "noderef" that
just say that a pointer cannot be dereferenced (it looks _exactly_ like a
pointer in all other respects, but trying to actually access anything
through it will cause a sparse warning).

The "bitwise" attribute is very much like the "noderef" one, in that it
restricts how you can use an expression of that type. Unlike "noderef",
it's designed for integer types, though. In fact, sparse will refuse to
apply the bitwise attribute to non-integer types.

As the name suggests, a "bitwise" expression is one that is restricted to
only a certain "bitwise" operations that make sense within that class. In
particular, you can't mix a "bitwise" class with a normal integer
expression (the constant zero happens to be special, since it's "safe"
for all bitwise ops), and in fact you can't even mix it with _another_
bitwise expression of a different type.

And when I say "different", I mean even _slightly_ different. Each typedef
creates a type of it's own, and will thus create a bitwise type that is
not compatible with anything else. So if you declare

	int __bitwise i;
	int __bitwise j;

the two variables "i" and "j" are _not_ compatible, simply because they
were declared separately, while in the case of

	int __bitwise i, j;

they _are_ compatible. The above is a horribly contrived example, as it
shows an extreme case that doesn't make much sense, but it shows how
"bitwise" always creates its own new "class".

Normally you'd always use "__bitwise"  in a typedef, which effectively
makes that particular typedef one single "bitwise class". After that, you
can obviously declare any number of variables in that class.

Now apart from the classes having to match, "bitwise" as it's name
suggests, also restricts all operations within that class to a subset of
"bit-safe" operations. For example, addition isn't "bit-safe", since
clearly the carry-chain moves bits around. But you can do normal bit-wise
operations, and you can compare the values against other values in the
same class, since those are all "bit-safe".

Oh, as an example of something that isn't obviously bit-safe: look out for
things like bit negation: doing a ~ is ok on an bitwise "int" type, but it
is _not_ ok on a bitwise "short" or "char". Why?  Because on a bitwise
"int" you actually stay within the type. But doing the same thing on a
short or char will move "outside" the type by virtue of setting the high
bits (normal C semantics: a short gets promoted to an "int", so doing a
bitwise negation on a short will actually set the high bits).

So as far as sparse is concerned, a "bitwise" type is not really so much
about endianness as it is about making sure bits are never lost or moved
around.

For example, you can use the bitwise operation to verify the __GFP_XXX
mask bits. Right now they are just regular integers, which means that you
can write

	kmalloc(GFP_KERNEL, size);

and the compiler will not notice anything wrong. But something is
_seriously_ wrong: the GFP_KERNEL should be the _second_ argument. If we
mark it to be a "bitwise" type (which it is), that bug would have been
noticed immediately, and you could still do all the operations that are
valid of GFP_xxx values.

See the usage?

In the byte-order case, what we have is:

	typedef __u16 __bitwise __le16;
	typedef __u16 __bitwise __be16;
	typedef __u32 __bitwise __le32;
	typedef __u32 __bitwise __be32;
	typedef __u64 __bitwise __le64;
	typedef __u64 __bitwise __be64;

and if you think about the above rules about what is acceptable for
bitwise types, you'll likely immediately notivce that it automatically
means

 - you can never assign a __le16 type to any other integer type or any
   other bitwise type. You'd get a warning about incompatible types. Makes
   sense, no?
 - you can only do operations that are safe within that byte order. For
   example, it is safe to do a bitwise "&" on two __le16 values. Clearly
   the result is meaningful.
 - if you want to go outside that bitwise type, you have to convert it
   properly first. For example, if you want to add a constant to a __le16
   type, you can do so, but you have to use the proper sequence:

	__le16 sum, a, b;

	sum = a + b;	/* INVALID! "warning: incompatible types for operation (+)" */
	sum = cpu_to_le16(le16_to_cpu(a) + le16_to_cpu(b));	/* Ok */

See?

In short, "bitwise" is about more than just byte-order, but the semantics
of bitwise-restricted ops happen to be the semantics that are valid for
byte-order operations too.

Oh, btw, right now you only get the warnings from sparse if you use
"-Wbitwise" on the command line. Without that, sparse will ignore the
bitwise attribute.

		Linus

Newsgroups: fa.linux.kernel
From: viro@parcelfarce.linux.theplanet.co.uk
Subject: Re: Being more careful about iospace accesses..
Original-Message-ID: <20040916001001.GN23987@parcelfarce.linux.theplanet.co.uk>
Date: Thu, 16 Sep 2004 00:22:13 GMT
Message-ID: <fa.n7t1aqq.1g1upre@ifi.uio.no>

On Wed, Sep 15, 2004 at 04:26:12PM -0700, Linus Torvalds wrote:

>    other bitwise type. You'd get a warning about incompatible types. Makes
>    sense, no?
>  - you can only do operations that are safe within that byte order. For
>    example, it is safe to do a bitwise "&" on two __le16 values. Clearly
>    the result is meaningful.

BTW, so far the most frequent class of endianness bugs had been along the
lines of
	foo->le16_field = cpu_to_le32(12);
and vice versa.  On big-endian it's a guaranteed FUBAR - think carefully about
the value that will end up there.

> Oh, btw, right now you only get the warnings from sparse if you use
> "-Wbitwise" on the command line. Without that, sparse will ignore the
> bitwise attribute.

We probably want __attribute__((opaque)) in addition to bitwise - e.g. for
the handles of all sorts passed in network filesystem protocols.  I'll look
into that when we get the endianness warnings somewhat under control.

For now I'm going to #define __opaque __bitwise and use it for stuff like
	typedef __u32 __opaque cifs_fid;
etc.

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: Being more careful about iospace accesses..
Original-Message-ID: <Pine.LNX.4.58.0409160652460.2333@ppc970.osdl.org>
Date: Thu, 16 Sep 2004 14:05:01 GMT
Message-ID: <fa.gtufv67.8i2rhh@ifi.uio.no>

On Thu, 16 Sep 2004, David Woodhouse wrote:

> On Wed, 2004-09-15 at 16:26 -0700, Linus Torvalds wrote:
> >  - if you want to go outside that bitwise type, you have to convert it
> >    properly first. For example, if you want to add a constant to a __le16
> >    type, you can do so, but you have to use the proper sequence:
> >
> > 	__le16 sum, a, b;
> >
> > 	sum = a + b;	/* INVALID! "warning: incompatible types for operation (+)" */
> > 	sum = cpu_to_le16(le16_to_cpu(a) + le16_to_cpu(b));	/* Ok */
> >
> > See?
>
> Yeah right, that latter case is _so_ much more readable

It's not about readability.

It's about the first case being WRONG!

You can't add two values in the wrong byte-order. It's not an operation
that makes sense. You _have_ to convert them to CPU byte order first.

I certainly agree that the first version "looks nicer".

> It's even nicer when it ends up as:
>
> 	sum = cpu_to_le16(le16_to_cpu(a) + le16_to_cpu(b));	/* Ok */
> 	sum |= c;
> 	sum = cpu_to_le16(le16_to_cpu(sum) + le16_to_cpu(d));

This is actually the strongest argument _against_ hiding endianness in the
compiler, or hiding it behind macros. Make it very explicit, and just make
sure there are tools (ie 'sparse') that can tell you when you do something
wrong.

Any programmer who sees the above will go "well that's stupid", and
rewrite it as something saner instead. You can certainly rewrite it as

	cpu_sum = le16_to_cpu(a) + le16_to_cpu(b);
	cpu_sum |= le16_to_cpu(c);
	cpu_sum += le16_to_cpu(d);
	sum = cpu_to_le16(d);

which gets rid of the double conversions.

But if you hide the endianness in macro's, you'll never see the mess at
all, and won't be able to fix it.

> I'd really quite like to see the real compiler know about endianness,
> too.

I would have agreed with you some time ago. Having been bitten by too damn
many compiler bugs I've become convinced that the compiler doing things
behind your back to "help" you just isn't worth it. Not in a kernel, at
least. It's much better to build up good typechecking and the
infrastructure to help you get the job done.

Expressions like the above might happen once or twice in a project with
several million lines of code. It's just not worth compiler infrastructure
for - that just makes people use it as if it is free, and impossible to
find the bugs when they _do_ happen. Much better to have a type system
that can warn about the bad uses, but that doesn't actually change any of
the code generated..

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: Add sparse "__iomem" infrastructure to check PCI address usage
Original-Message-ID: <Pine.LNX.4.58.0409121922450.13491@ppc970.osdl.org>
Date: Mon, 13 Sep 2004 09:57:00 GMT
Message-ID: <fa.iro5mg6.14kcbbs@ifi.uio.no>

On Sun, 12 Sep 2004, Jeff Garzik wrote:
>
> Linux Kernel Mailing List wrote:
> > --- a/include/linux/compiler.h	2004-09-11 00:26:40 -07:00
> > +++ b/include/linux/compiler.h	2004-09-11 00:26:40 -07:00
> > @@ -6,13 +6,17 @@
> >  # define __kernel	/* default address space */
> >  # define __safe		__attribute__((safe))
> >  # define __force	__attribute__((force))
> > +# define __iomem	__attribute__((noderef, address_space(2)))
>
> Dumb gcc attribute questions:
>
> 1) what does force do? it doesn't appear to be in gcc 3.3.3 docs.

It doesn't do anything for gcc. You're looking at the sparse-only code.

What "attribute((force))" does for sparse is to mark a type to be
"forced", ie a cast to a forced type will not complain even if the cast
otherwise would be invalid.

For example, "sparse" will warn about explicit casts that drop address
space information:

	void __user *userptr;

	...
	memset((void *)userptr, 0, ...)

will cause a

	warning: cast removes address space of expression

complaint from sparse. But some _internal_ functions want to force the
cast because they know it's safe. For example, you'll find

	#define __addr_ok(addr) ((unsigned long __force)(addr) < (current_thread_info()->addr_limit.seg))

because this internal x86 implementation detail knows that in that
particular case it's safe to remove the address space information (it's
just checking the user pointer against the address limit).

For gcc, none of this means anything, so all the #define's just become
empty.

> 2) is "volatile ... __force" redundant?

No, although it's likely to be a strange combination. If you want to force
a static address space conversion to a volatile pointer, you can do so. I
don't see _why_ you'd want to do it ;)

> 3) can we use 'malloc' attribute on kmalloc?

Since we can't use the gcc alias analysis anyway (it's too broken until
very late gcc versions), the gcc 'malloc' attribute shouldn't make any
difference that I can tell.

But there wouldn't be anything _wrong_ in adding it to kmalloc(), if
that's what you're asking.

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: Add sparse "__iomem" infrastructure to check PCI address usage
Original-Message-ID: <Pine.LNX.4.58.0409121945500.13491@ppc970.osdl.org>
Date: Mon, 13 Sep 2004 09:53:02 GMT
Message-ID: <fa.is7rn09.1446arr@ifi.uio.no>

On Sun, 12 Sep 2004, Jeff Garzik wrote:
>
> > No, although it's likely to be a strange combination. If you want to force
> > a static address space conversion to a volatile pointer, you can do so. I
> > don't see _why_ you'd want to do it ;)
>
> Well the reason I ask....
>
> static inline void writeb(unsigned char b, volatile void __iomem *addr)
> {
>          *(volatile unsigned char __force *) addr = b;
> }

Right. Let's look a bit closer (more of an explanation than you need, but
hey, maybe somebody else is also wondering):

 - for gcc, none of this matters one whit. We're just passing in a
   "volatile void *", and we're storing the value "b" to the byte pointed
   by it. Which is correct on x86, since memory-mapped PCI-space just
   looks like memory on x86.

   This is important to remember: for gcc, the sparse annotations are
   meaningless. They can still be useful just to tell the _programmer_
   that "hey, that pointer you got wasn't a normal pointer" in a fairly
   readable manner, but in the end, unless you use sparse, they don't
   actually _do_ anything.

HOWEVER. When you _do_ use parse, it is another matter entirely. For
"sparse", that "__iomem"  has lots of meaning:

	# define __iomem       __attribute__((noderef, address_space(2)))

ie "iomem" means two separate things: it means that sparse should complain
if the pointer is ever dereferenced (it's a "noderef" pointer) directly,
and it's in "address space 2" as opposed to the normal address space (0).

Now, that means that _sparse_ will complain if such a pointer is ever
passed into a function that wants a regular pointer (because it is _not_ a
normal pointer, and you obviously shouldn't do things like "strcmp()" etc
on it), and sparse will also complain if you try to cast it to another
pointer in another address space.

So if you compile and install sparse, and build with "make C=1", you'll
get warnings like

	drivers/video/aty/radeon_base.c:1725:42: warning: incorrect type in argument 2 (different address spaces)
	drivers/video/aty/radeon_base.c:1725:42:    expected void const *from
	drivers/video/aty/radeon_base.c:1725:42:    got void [noderef] *[assigned] base_addr<asn:2>

which is just another way sparse tells you that there is a bug in the
source code (in this case, we try to copy from PCI memory-mapped space
directly to user space using "copy_to_user()", which is a _bad_ idea).

So not only can you not dereference it by mistake, you can't even _turn_
it into a pointer that you could dereference by mistake. Sparse will
complain. Sparse will complain even if you use an explicit cast to make it
a normal "(void *)".

And that's good, because on some other architectures, if you try to
dereference the pointer, the machine just oopses. You need to do all the
special magic to actually read from memory-mapped PCI space.

HOWEVER. On x86, it just so happens that dereferencing the pointer _is_
actually the right thing to do, as long as you only do it with the proper
interfaces (ie readb/writeb and friends). And so that sparse won't be
upset, we use the "__force" directive - we're telling sparse that "I know
what I'm doing". And so sparse will quietly allow us to dereference that
pointer that was originally not dereferencable.

Generally, you shouldn't ever use __force in a driver or anything like
that. It's usually a valid thing to do only in the code that is defined
for that particular type. By definition, a driver isn't an entity that
should understand how architecture-specific data structures work, so it
shouldn't try to force things.

So a good use of "__force" is exactly the usage you quote: the
arch-specific code that actually really _knows_ what the magic address
space means, and knows what to do about it.

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: Add sparse "__iomem" infrastructure to check PCI address usage
Original-Message-ID: <Pine.LNX.4.58.0409131142370.2378@ppc970.osdl.org>
Date: Mon, 13 Sep 2004 18:51:50 GMT
Message-ID: <fa.gtu7vu9.bieqpv@ifi.uio.no>

On Mon, 13 Sep 2004, Tonnerre wrote:
>
> On Sun, Sep 12, 2004 at 08:00:48PM -0700, Linus Torvalds wrote:
> > Generally, you shouldn't ever use __force in a driver or anything like
> > that.
>
> Why don't we send the __force attribute into some #ifdef that is never
> defined unless  you're in  arch specific code?  This way  we'd prevent
> stupid people from doing stupid things.

Hey, the thing is, that may well prevent smart people from doing smart
things too. Give 'em rope, within reason.

The point behind __force is not so much that you should never use it, but
that you should never use it by _mistake_. It's very easy (and often
_required_) to have a regular typecast in C, and it can often hide bugs
when you cast something in a way that you didn't really think through.

For example, we often have generic "void *" or "unsigned long" things that
are used for passing opaque data around, and they need casts when working
with them. It is quite conceivable that you need an address space cast
too, and that you need to use "__force" to do so. It might be ok, even in
a driver. But hopefully it's something where the action of having to force
the cast makes people think about it more. And the fact that there
probably never will be very _many_ casts like that means that they'll stand
out.

If people start using "__force" to hide their own bugs, we'll just have to
start stringing them up. Hang'em high, I say. But maybe somebody has a
valid reason at times.

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: [PATCH 8/10] Re: [2.6-BK-URL] NTFS: 2.1.19 sparse annotation,
Original-Message-ID: <Pine.LNX.4.58.0409240926580.32117@ppc970.osdl.org>
Date: Fri, 24 Sep 2004 16:45:59 GMT
Message-ID: <fa.ivo9m04.16kkarm@ifi.uio.no>

On Fri, 24 Sep 2004, Anton Altaparmakov wrote:
>
>    - Fix all the sparse bitwise warnings.  Had to change all the enums
>      storing little endian values to #defines because we cannot set enums
>      to be little endian so we had lots of bitwise warnings from sparse.

Btw, Al is fixing this. We'll make enum's properly typed, rather than just
plain integers. It's not traditional C behaviour, but it gives you better
type safety, and Al points out that other C compilers (the Plan 9 one, to
be specific) have done the same thing for similar reasons.

So we'll eventually be able to use enum's instead of #defines without
losing any sparse information.

Of course, the only case where it matters is exactly cases like this,
where the difference between using an enum and a #define is basically a
matter of taste. But since I agree that enum's can look a lot nicer, it's
good to know that it's being worked on.

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: [PATCH 8/10] Re: [2.6-BK-URL] NTFS: 2.1.19 sparse annotation,
Original-Message-ID: <Pine.LNX.4.58.0409241930510.2317@ppc970.osdl.org>
Date: Sat, 25 Sep 2004 02:50:21 GMT
Message-ID: <fa.guubue7.8iiq9l@ifi.uio.no>

On Fri, 24 Sep 2004, Anton Altaparmakov wrote:

> On Fri, 24 Sep 2004, Linus Torvalds wrote:
> >
> > Btw, Al is fixing this. We'll make enum's properly typed, rather than just
> > plain integers. It's not traditional C behaviour, but it gives you better
> > type safety, and Al points out that other C compilers (the Plan 9 one, to
> > be specific) have done the same thing for similar reasons.

Well, when I said "Al is fixing this", I lied.

I just fixed it myself.

> This is good news.  Once that is done I will be very happy to go back to
> using enums as I also agree that they can and in this case do look a
> lot nicer...

Try the current sparse, I think it should work for you.

So if you make an enum where the initializer expression is a little-endian
expression, the type of that (single) enumerator will be little-endian.

HOWEVER, the type of an enum _variable_ will still be just "int". So

	enum myenum {
		one = 1ULL,
		two = 2,
	};

has the strange behaviour that if you use "one" in an expression, it will
have the type "unsigned long long", but if you use a "enum myenum" entry
(even if it has the value "1"), it will be an "int":

	sizeof(one) == 8
	sizeof(enum myenum) == 4

So I would stronly suggest (and I may make sparse warn) against using
non-integertyped enum values with any enum that actually has any backing
store (ie if you ever use a variable of type "enum myenum", that would
result in a warning - you can really just use the values "one" and "two"
directly).

Note that gcc has some similarly strange behaviour wrt enum types that
depends on the _values_ of the enum entries themselves, and that can
result in serious problems if you declare the enum in a different place
from where you defined the values. Again, those problems only happen for
enums that actually get used with backing store (as opposed to just
readable compile-time constants).

		Linus

Newsgroups: fa.linux.kernel
From: viro@parcelfarce.linux.theplanet.co.uk
Subject: Re: [PATCH 8/10] Re: [2.6-BK-URL] NTFS: 2.1.19 sparse annotation, 
	cleanups and a bugfix
Original-Message-ID: <20040925072516.GS23987@parcelfarce.linux.theplanet.co.uk>
Date: Sat, 25 Sep 2004 07:26:42 GMT
Message-ID: <fa.natpbau.1u1upr8@ifi.uio.no>

On Fri, Sep 24, 2004 at 07:46:20PM -0700, Linus Torvalds wrote:
> So I would stronly suggest (and I may make sparse warn) against using
> non-integertyped enum values with any enum that actually has any backing
> store (ie if you ever use a variable of type "enum myenum", that would
> result in a warning - you can really just use the values "one" and "two"
> directly).

Linus, backing store is irrelevant here (and BTW, variables are no better
or worse than arguments / structure fields / return values / argument of
sizeof / etc.)

a) integer type equivalent to particular enum is up to compiler; anything
that depends on it is at the very least non-portable.

b) __bitwise doesn't break anything; __le32 enum members are just as OK as
int ones.

c) its enum members where we are not doing what gcc does; enum itself is
trivial to deal with.  So that's where the warnings should be.

Anyway, I'll send you sparse patches tomorrow when I rediff that stuff to
your current tree...

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: [PATCH 8/10] Re: [2.6-BK-URL] NTFS: 2.1.19 sparse annotation,
Original-Message-ID: <Pine.LNX.4.58.0409250834110.2317@ppc970.osdl.org>
Date: Sat, 25 Sep 2004 15:49:54 GMT
Message-ID: <fa.gse9uec.a2gq9g@ifi.uio.no>

On Sat, 25 Sep 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
>
> Linus, backing store is irrelevant here (and BTW, variables are no better
> or worse than arguments / structure fields / return values / argument of
> sizeof / etc.)

I agree that in the case of NTFS it is irrelevant. I was talking more in
general: if you use enums with "types", you really should only use them as
compile-time constants. Which is, obviously, one very common usage of
enums, but is not the only one.

I personally believe that people use enum's largely in two (independent)
ways:

 - a convenient compile-time constant:

	enum {
		DevEnableMask = 1UL << 0,
		DevIRQMask = 1UL << 5,
		DevError = 1UL << 31
	};

   where you never actually _save_ an enum anywhere. In this case, the
   typing is very convenient indeed.

 - a "type enumerator":

	enum token_type {
		TOKEN_IDENT = 1,
		TOKEN_NUMBER,
		TOKEN_MACRO,
	...

   where the enum actually is used as a variable to distinguish different
   cases. In this case, the per-enum typing ends up being possibly even
   confusing, since using a constant will have a potentially _different_
   type than loading that constant from a variable.

The second case is why I think it's a sane thing to warn if anybody ever
creates a variable (or structure/union member) with an enum that used the
typing features. Not because we can't make the enum fit all the values,
but because the types simply WILL NOT MATCH. They fundamentally cannot,
since we took the approach of having per-entry types.

And for sparse, since the type is _the_ most important part of anything,
we should warn when the types won't match.

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: Totally broken PCI PM calls
Original-Message-ID: <Pine.LNX.4.58.0410150839010.3897@ppc970.osdl.org>
Date: Fri, 15 Oct 2004 16:01:46 GMT
Message-ID: <fa.gsu206h.aikoht@ifi.uio.no>

On Fri, 15 Oct 2004, Pavel Machek wrote:
>
> I'm trying to learn how to work with bitwise on obsolete stuff, but
> checking there is good, too, right?
>
> Is this right way to do it?
>
> +typedef enum pm_request __bitwise {
> +       __bitwise PM_SUSPEND, /* enter D1-D3 */
> +       __bitwise PM_RESUME,  /* enter D0 */
> +} pm_request_t;

No, "__bitwise" is a type attribute, so you'd have to do it something like
this:

	typedef int __bitwise pm_request_t;

	enum pm_request {
		PM_SUSPEND = (__force pm_request_t) 1,
		PM_RESUME = (__force pm_request_t) 2
	};

which makes PM_SUSPEND and PM_RESUME "bitwise" integers (the "__force" is
there because sparse will complain about casting to/from a bitwise type,
but in this case we really _do_ want to force the conversion). And because
the enum values are all the same type, now "enum pm_request" will be that
type too.

And with gcc, all the __bitwise/__force stuff goes away, and it all ends
up looking just like integers to gcc.

Quite frankly, you don't need the enum there. The above all really just
boils down to one special "int __bitwise" type.

So the simpler way is to just do

	typedef int __bitwise pm_request_t;

	#define PM_SUSPEND ((__force pm_request_t) 1)
	#define PM_RESUME ((__force pm_request_t) 2)

and you now have all the infrastructure needed for strict typechecking.

One small note: the constant integer "0" is special. You can use a
constant zero as a bitwise integer type without sparse ever complaining.
This is because "bitwise" (as the name implies) was designed for making
sure that bitwise types don't get mixed up (little-endian vs big-endian
vs cpu-endian vs whatever), and there the constant "0" really _is_
special.

Also, because of the "bitwise" nature of bitwise types, you cannot add,
subtract or do a lot of things with bitwise types. But you _can_ use the
bitwise operations on them, and you can test them for equality.

So at some point we might add a separate "__opaque" type that allows no
operations at all (except for assignment and equality comparison), and
where "0" isn't special. But in the meantime, __bitwise gets you most of
the way. Just keep in mind that sparse won't warn about use of the
constant zero.

> Having __bitwise at every line in enum looks quite ugly to my
> eyes.

And in fact you cannot do it that way. "__bitwise" will always create a
_new_ type, so every time you use it you get a _different_ type. So to use
it sanely, you have to create _one_ typedef for each type you want to use,
and make that one __bitwise, and that will be the only __bitwise that
you'll ever see for that particular usage. After that, you use the
typedef, because it is now a unique type, thanks to the __bitwise.

>	 [Where to get sparse? I tried to google for it but "sparse" is
> very common word (as in sparse matrix). And theres no
> kernel/people/linus on kernel.org...]

With BK, you can just get it from

	bk://sparse.bkbits.net/sparse

and I think DaveJ does tar-balls somewhere. If you search for "sparse
checker linux" you'll find a number of hits..

Once you have it, just do

	make
	make install

as your regular user, and it will install sparse in your ~/bin directory.
After that, doing a kernel make with "make C=1" will run sparse on all the
C files that get recompiled, or with "make C=2" will run sparse on the
files whether they need to be recompiled or not (ie the latter is fast way
to check the whole tree if you have already built it).

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Sparse "context" checking..
Original-Message-ID: <Pine.LNX.4.58.0410302005270.28839@ppc970.osdl.org>
Date: Sun, 31 Oct 2004 03:22:13 GMT
Message-ID: <fa.j07fng1.134u8bj@ifi.uio.no>

I just committed the patches to the kernel to start supporting a new
automated correctness check that I added to sparse: the counting of static
"code context" information.

The sparse infrastructure is pretty agnostic, and you can count pretty
much anything you want, but it's designed to test that the entry and exit
contexts match, and that no path through a function is ever entered with
conflicting contexts.

In particular, this is designed for doing things like matching up a "lock"
with the pairing "unlock", and right now that's exactly what the code
does: it makes each spinlock count as "+1" in the context, and each
spinunlock count as "-1", and then hopefully it should all add up.

It doesn't always, of course. Since it's a purely static analyser, it's
unhappy about code like

	int fn(arg)
	{
		if (arg)
			spin_lock(lock);
		...
		if (arg)
			spin_unlock(lock);
	}

because the code is not statically deterministic, and the stuff in between
can be called with or without a lock held. That said, this has long been
frowned upon, and there aren't that many cases where it happens.

Right now the counting is only enabled if you use sparse, and add the
"-Wcontext" flag to the sparse command line by hand - and the spinlocks
have only been annotated for the SMP case, so right now it only works for
CONFIG_SMP. Details, details.

Also, since sparse does purely local decisions, if you actually _intend_
to grab a lock in one function and release it in another, you need to tell
sparse so, by annotating the function that acquires the lock (with
"__acquires(lockname)") and the function that releases it (with, surprise
surprise, "__releases(lockname)") in the declaration. That tells sparse to
update the context in the callers appropriately, but it also tells sparse
to expect the proper entry/exit contexts for the annotated functions
themselves.

I haven't done the annotation for any functions yet, so expect warnings.
If you do a checking run, the warnings will look something like:

	  CHECK   kernel/resource.c
	kernel/resource.c:59:13: warning: context imbalance in 'r_start' - wrong count at exit
	kernel/resource.c:69:13: warning: context imbalance in 'r_stop' - unexpected unlock

which just shows that "r_start" acquired a lock, and sparse didn't expect
it to, while "r_stop" released a lock that sparse hadn't realized it had.
In this case, the cause is pretty obvious, and the annotations are equally
so.

A more complicated case is

	  CHECK   kernel/sys.c
	kernel/sys.c:465:2: warning: context imbalance in 'sys_reboot' - different lock contexts for basic block

where that "different lock contexts" warning means that sparse determined
that some code in that function was reachable with two different lock
contexts. In this case it's actually harmless, since what happens in this
case is that the code after rebooting the machine is unreachable, and
sparse just doesn't understand that.

But in other cases it's more fundamental, and the lock imbalance is due to
dynamic data that sparse just can't understand. The warning in that case
can be disabled by hand, but there doesn't seem to be that many of them. A
full kernel build for me has about 200 warnings, and most of them seem to
be the benign kind (ie the kind above where one function acquires the lock
and another releases it, and they just haven't been annotated as such).

The sparse thing could be extended to _any_ context that wants pairing,
and I just wanted to let people know about this in case they find it
interesting..

			Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: Sparse "context" checking..
Original-Message-ID: <Pine.LNX.4.58.0410302158230.28839@ppc970.osdl.org>
Date: Sun, 31 Oct 2004 05:04:31 GMT
Message-ID: <fa.j079oo4.134k93u@ifi.uio.no>

On Sat, 30 Oct 2004, Roland Dreier wrote:
>
>     Linus> In particular, this is designed for doing things like
>     Linus> matching up a "lock" with the pairing "unlock", and right
>     Linus> now that's exactly what the code does: it makes each
>     Linus> spinlock count as "+1" in the context, and each spinunlock
>     Linus> count as "-1", and then hopefully it should all add up.
>
> Do you have a plan for how to handle functions like spin_trylock()?  I
> notice in the current tree you just didn't annotate spin_trylock().

Actually, the _current_ tree does actually annotate spin_trylock() (as of
just before I sent out the email). It looks like

	#define spin_trylock(lock)      __cond_lock(_spin_trylock(lock))

where __cond_lock() for sparse is

	include/linux/compiler.h:# define __cond_lock(x)        ((x) ? ({ __context__(1); 1; }) : 0)

ie we add a "+1" context marker for the success case.

NOTE! This works with sparse only because sparse does immediate constant
folding, so if you do

	if (spin_trylock(lock)) {
		..
		spin_unlock(lock);
	}

sparse linearizes that the right way unconditionally, and even though
there is a data-dependency, the data dependency is constant. However, if
some code does

	success = spin_trylock(lock);
	if (success) {
		..
		spin_unlock(lock);
	}

sparse would complain about it, because sparse doesn't do any _real_ data
flow analysis.

So sparse can follow all the obvious cases, including trylock and
"atomic_dec_and_lock()".

			Linus