linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: Add sparse "__iomem" infrastructure to check PCI address usage
       [not found] <200409110726.i8B7QTGn009468@hera.kernel.org>
@ 2004-09-13  0:26 ` Jeff Garzik
  2004-09-13  0:50   ` David S. Miller
  2004-09-13  2:31   ` Linus Torvalds
  0 siblings, 2 replies; 10+ messages in thread
From: Jeff Garzik @ 2004-09-13  0:26 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Linux Kernel Mailing List

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.

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

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Add sparse "__iomem" infrastructure to check PCI address usage
  2004-09-13  0:26 ` Add sparse "__iomem" infrastructure to check PCI address usage Jeff Garzik
@ 2004-09-13  0:50   ` David S. Miller
  2004-09-13  2:31   ` Linus Torvalds
  1 sibling, 0 replies; 10+ messages in thread
From: David S. Miller @ 2004-09-13  0:50 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: torvalds, linux-kernel

On Sun, 12 Sep 2004 20:26:38 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Dumb gcc attribute questions:
> 
> 1) what does force do? it doesn't appear to be in gcc 3.3.3 docs.
> 
> 2) is "volatile ... __force" redundant?
> 
> 3) can we use 'malloc' attribute on kmalloc?

Jeff, this code you quoted is in the sparse ifdef block.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Add sparse "__iomem" infrastructure to check PCI address usage
  2004-09-13  0:26 ` Add sparse "__iomem" infrastructure to check PCI address usage Jeff Garzik
  2004-09-13  0:50   ` David S. Miller
@ 2004-09-13  2:31   ` Linus Torvalds
  2004-09-13  2:41     ` Jeff Garzik
  1 sibling, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2004-09-13  2:31 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: Linux Kernel Mailing List



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Add sparse "__iomem" infrastructure to check PCI address usage
  2004-09-13  2:31   ` Linus Torvalds
@ 2004-09-13  2:41     ` Jeff Garzik
  2004-09-13  3:00       ` Linus Torvalds
  0 siblings, 1 reply; 10+ messages in thread
From: Jeff Garzik @ 2004-09-13  2:41 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Linux Kernel Mailing List

Linus Torvalds wrote:
> On Sun, 12 Sep 2004, Jeff Garzik wrote:
>>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.

doh, and thanks for the info.


>>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 ;)

Well the reason I ask....

static inline void writeb(unsigned char b, volatile void __iomem *addr)
{
         *(volatile unsigned char __force *) addr = b;
}
static inline void writew(unsigned short b, volatile void __iomem *addr)
{
         *(volatile unsigned short __force *) addr = b;
}
static inline void writel(unsigned int b, volatile void __iomem *addr)
{
         *(volatile unsigned int __force *) addr = b;
}


>>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.

That's what I'm asking.

Thanks,

	Jeff



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Add sparse "__iomem" infrastructure to check PCI address usage
  2004-09-13  2:41     ` Jeff Garzik
@ 2004-09-13  3:00       ` Linus Torvalds
  2004-09-13 14:22         ` Geert Uytterhoeven
  2004-09-13 18:31         ` Tonnerre
  0 siblings, 2 replies; 10+ messages in thread
From: Linus Torvalds @ 2004-09-13  3:00 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: Linux Kernel Mailing List



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Add sparse "__iomem" infrastructure to check PCI address usage
  2004-09-13  3:00       ` Linus Torvalds
@ 2004-09-13 14:22         ` Geert Uytterhoeven
  2004-09-13 14:33           ` Linus Torvalds
  2004-09-13 18:31         ` Tonnerre
  1 sibling, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2004-09-13 14:22 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Linux Kernel Mailing List


While resuming adding __user annotations to the m68k-specific parts of the
code, I stumbled on

    struct task_struct {
	...
	unsigned long sas_ss_sp;
	...
    }

If I'm not mistaken, sas_ss_sp is always a pointer to user stack space.
Shouldn't it be changed to `void __user *sas_ss_sp', or is an
unsigned long/void * change in generic code a too controversial change for
making sparse happy?

And I guess I can find a few more of these...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Add sparse "__iomem" infrastructure to check PCI address usage
  2004-09-13 14:22         ` Geert Uytterhoeven
@ 2004-09-13 14:33           ` Linus Torvalds
  0 siblings, 0 replies; 10+ messages in thread
From: Linus Torvalds @ 2004-09-13 14:33 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Linux Kernel Mailing List



On Mon, 13 Sep 2004, Geert Uytterhoeven wrote:
> 
> While resuming adding __user annotations to the m68k-specific parts of the
> code, I stumbled on
> 
>     struct task_struct {
> 	...
> 	unsigned long sas_ss_sp;
> 	...
>     }
> 
> If I'm not mistaken, sas_ss_sp is always a pointer to user stack space.
> Shouldn't it be changed to `void __user *sas_ss_sp', or is an
> unsigned long/void * change in generic code a too controversial change for
> making sparse happy?

I don't think it's too controversial per se, and it would certainly remove 
at least two casts from kernel/signal.c. Are you ready to fix up some 
other architectures from the fall-out (x86 at the least..)

		Linus

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Add sparse "__iomem" infrastructure to check PCI address usage
  2004-09-13  3:00       ` Linus Torvalds
  2004-09-13 14:22         ` Geert Uytterhoeven
@ 2004-09-13 18:31         ` Tonnerre
  2004-09-13 18:48           ` Linus Torvalds
  2004-09-13 18:51           ` viro
  1 sibling, 2 replies; 10+ messages in thread
From: Tonnerre @ 2004-09-13 18:31 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Jeff Garzik, Linux Kernel Mailing List

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

Salut,

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.

				Tonnerre

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Add sparse "__iomem" infrastructure to check PCI address usage
  2004-09-13 18:31         ` Tonnerre
@ 2004-09-13 18:48           ` Linus Torvalds
  2004-09-13 18:51           ` viro
  1 sibling, 0 replies; 10+ messages in thread
From: Linus Torvalds @ 2004-09-13 18:48 UTC (permalink / raw)
  To: Tonnerre; +Cc: Jeff Garzik, Linux Kernel Mailing List



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 _man_ 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Add sparse "__iomem" infrastructure to check PCI address usage
  2004-09-13 18:31         ` Tonnerre
  2004-09-13 18:48           ` Linus Torvalds
@ 2004-09-13 18:51           ` viro
  1 sibling, 0 replies; 10+ messages in thread
From: viro @ 2004-09-13 18:51 UTC (permalink / raw)
  To: Tonnerre; +Cc: Linus Torvalds, Jeff Garzik, Linux Kernel Mailing List

On Mon, Sep 13, 2004 at 08:31:26PM +0200, Tonnerre wrote:
> Salut,
> 
> 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.

man grep

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2004-09-13 18:54 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <200409110726.i8B7QTGn009468@hera.kernel.org>
2004-09-13  0:26 ` Add sparse "__iomem" infrastructure to check PCI address usage Jeff Garzik
2004-09-13  0:50   ` David S. Miller
2004-09-13  2:31   ` Linus Torvalds
2004-09-13  2:41     ` Jeff Garzik
2004-09-13  3:00       ` Linus Torvalds
2004-09-13 14:22         ` Geert Uytterhoeven
2004-09-13 14:33           ` Linus Torvalds
2004-09-13 18:31         ` Tonnerre
2004-09-13 18:48           ` Linus Torvalds
2004-09-13 18:51           ` viro

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).