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