Re: Being more anal about iospace accesses..

[Linus Torvalds]

On Wed, 15 Sep 2004, Jörn Engel wrote:
> 
> But it still leaves me confused.  Before I had this code:
> 
> 	struct regs {
> 		uint32_t status;
> 		...
> 	}
> 
> 	...
> 
> 	struct regs *regs = ioremap(...);
> 	uint32_t status = regs->status;
> 	...
> 
> So now I should do it like this:
> 
> 	#define REG_STATUS 0
> 
> 	...
> 
> 	void __iomem *regs = ioremap(...);
> 	uint32_t status = readl(regs + REG_STATUS);

No, you can certainly continue to use non-void pointers. The "void __iomem
*" case is just the typeless one, exactly equivalent to regular void
pointer usage.

So let me clarify my original post with two points:

 - if your device only supports MMIO, you might as well just use the old 
   interfaces. The new interface will _also_ work, but there is no real 
   advantage, unless you count the "pci_iomap()" as a simpler interface.

   The new interface is really only meaningful for things that want to 
   support _both_ PIO and MMIO. It's also, perhaps, a bit syntactically 
   easier to work with, so some people might prefer that for that 
   reason. See my comments further down on the auto-sizing. BUT it doesn't 
   make the old interfaces go away by any means, and I'm not even
   suggesting that people should re-write drivers just for the hell of it.

   In short: if you don't go "ooh, that will simplify XXX", then you 
   should just ignore the new interfaces.

 - you can _absolutely_ use other pointers than "void *". You should 
   annotate them with "__iomem" if you want to be sparse-clean (and it 
   often helps visually to clarify the issue), but gcc won't care, the 
   "__iomem" annotation is purely a extended check.

So you can absolutely still continue with

	struct mydev_iolayout {
		__u32 status;
		__u32 irqmask;
		...

	struct mydev_iolayout __iomem *regs = pci_map(...);
	status = ioread32(&regs.status);

which is often a lot more readable, and thus in fact _preferred_. It also
adds another level of type-checking, so I applaud drivers that do this.

Now, I'm _contemplating_ also allowing the "get_user()" kind of "unsized" 
access function for the new interface. Right now all the old (and the new) 
access functions are all explicitly sized. But for the "struct layout" 
case, it's actually often nice to just say

	status = ioread(&regs.status);

and the compiler can certainly figure out the size of the register on its
own. This was impossible with the old interface, because the old 
interfaces didn't even take a _pointer_, much less one that could be sized 
up automatically.

(The auto-sizing is something that "get_user()" and "put_user()" have
always done, and it makes them very easy to use. It involved a few pretty
ugly macros, but hey, that's all hidden away, and is actually pretty
simple in the end).

		Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/