linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@osdl.org>
To: Roland Dreier <roland@topspin.com>
Cc: "Jörn Engel" <joern@wohnheim.fh-wedel.de>,
	"Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: Being more anal about iospace accesses..
Date: Wed, 15 Sep 2004 10:39:02 -0700 (PDT)	[thread overview]
Message-ID: <Pine.LNX.4.58.0409151024510.2333@ppc970.osdl.org> (raw)
In-Reply-To: <524qlzxxka.fsf@topspin.com>



On Wed, 15 Sep 2004, Roland Dreier wrote:
> 
> However, I somewhat agree -- it's ugly for drivers rely on this and do
> arithmetic on void *.  It should be OK for a driver to use 
> char __iomem * for its IO base if it needs to add in offsets, right?

"char __iomem *" will certainly work - all the normal pointer conversions 
are ok. Some people in fact use pointers to structures in MMIO space, and 
this is quite reasonable when working with a chip that uses "mailboxes" 
for commands.

However, I disagree with "void *" arithmetic being ugly. It's a very nice
feature to have a pointer that can be validly cast to any other type, and
that is the whole _point_ of "void *". The fact that C++ got that wrong is
arguably the worst failing of the language, causing tons of unnecessary
casts that can silently hide real bugs (maybe the thing you cast wasn't a
"void *" in the first place, but you'll never know - the compiler will do
the cast for you).

For example, to go back to the mailbox example, let's say that your 
hardware has an IO area that is 8kB in size, with the last 4kB being 
mailboxes.

The _sane_ way to do that is to do

	void __iomem *base_io = ioremap(...);
	struct mailbox __iomem *mbox = base_io + MAILBOX_OFFSET;

and then just work on that.

In contrast, havign to cast to a "char *" in order to do arithmetic, and 
then casting back to the resultant structure type pointer is not only 
ugly and unreadable, it's a lot more prone to errors as a result.

In other words, think of "void *" as a pointer to storage. Not "char"  
(which is the C name for a signed byte), but really, it's the pointer to 
whatever underlying memory there is. And a _fundamental_ part of such 
memory is the fact that it is addressable. Thus "pointer to storage 
arithmetic" really does make sense on a very fundamental level. It has 
nothing to do with C types, and that also explains why "void *" silently 
converts to anything else. It's a very internally consistent world-view.

Now, I disagree with gcc when it comes to actually taking the "size" of 
void. Gcc will silently accept

	void *x;
	x = malloc(sizeof(*x));

which I consider to be an abomination (and the above _can_ happen, quite
easily, as part of macros for doing allocation etc - nobody would write 
it in that form, but if you have an "MEMALLOC(x)" macro that does the 
sizeof, you could end up trying to feed the compiler bogus code).

The fact that you can do arithmetic on typeless storage does _not_ imply
that typeless storage would have a "size" in my book.

So sparse will say:

	warning: cannot size expression

and refuse to look at broken code like the above. But hey, the fact that I 
have better taste than anybody else in the universe is just something I 
have to live with. It's not easy being me.

		Linus

  reply	other threads:[~2004-09-15 17:42 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <Pine.LNX.4.58.0409081543320.5912@ppc970.osdl.org>
     [not found] ` <Pine.LNX.4.58.0409150737260.2333@ppc970.osdl.org>
2004-09-15 16:30   ` Being more anal about iospace accesses Linus Torvalds
2004-09-15 16:54     ` Jörn Engel
2004-09-15 17:07       ` Linus Torvalds
2004-09-15 17:32         ` Jörn Engel
2004-09-15 17:57           ` Linus Torvalds
2004-09-15 18:06             ` Linus Torvalds
2004-09-15 19:34             ` Greg KH
2004-09-15 19:53               ` Linus Torvalds
2004-09-15 20:06                 ` Greg KH
2004-09-16 14:58             ` Horst von Brand
2004-09-15 17:45         ` Nikita Danilov
2004-09-15 18:09           ` Linus Torvalds
2004-09-15 18:40         ` Chris Wedgwood
2004-09-15 17:07       ` Jeff Garzik
2004-09-15 17:16       ` Roland Dreier
2004-09-15 17:39         ` Linus Torvalds [this message]
2004-09-15 20:07           ` Russell King
2004-09-15 17:36       ` Horst von Brand
2004-09-15 17:40       ` Brian Gerst
2004-09-15 16:56     ` Dave Jones
2004-09-15 17:19     ` Roger Luethi
2004-09-15 17:23     ` Richard B. Johnson
2004-09-15 22:21     ` Deepak Saxena
2004-09-15 22:46       ` Linus Torvalds
2004-09-15 23:09         ` Deepak Saxena
2004-09-16 12:51           ` Geert Uytterhoeven
2004-09-16 22:10             ` Deepak Saxena
2004-09-15 22:29     ` Roland Dreier
2004-09-15 23:26       ` Being more careful " Linus Torvalds
2004-09-16  0:10         ` viro
2004-09-16 11:40           ` David Woodhouse
2004-09-16 12:25         ` David Woodhouse
2004-09-16 14:01           ` Linus Torvalds
2004-09-18  9:46             ` Kai Henningsen
2004-09-15 18:25 Being more anal " linux

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Pine.LNX.4.58.0409151024510.2333@ppc970.osdl.org \
    --to=torvalds@osdl.org \
    --cc=joern@wohnheim.fh-wedel.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=roland@topspin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).