[RFC] Changing pci_iounmap to take 'bar' argument

[RFC] Changing pci_iounmap to take 'bar' argument

[Benjamin Herrenschmidt]

Hi !

On ppc and ppc64 platforms, pci_iounmap() currently does nothing, which
is bogus (leak of ioremap space for mmio). It needs to iounmap for MMIOs
and do nothign for IO space.

The problem is that wether it's IO or MMIO cannot be easily deduced from
the virtual address. We _could_ change the whole thing on ppc32 to play
tricks with the top address bits, and we could compare the virtual
address with the known regions containing PHBs IO space, but that sounds
to me like working around a bad API in the first place.

What about, instead, just adding the "int bar" argument to pci_iounmap()
like we pass to pci_iomap() so it can access the resource flags ?

If it's ok with you, I'll send a patch doing it later today.

Ben.

Re: [RFC] Changing pci_iounmap to take 'bar' argument

[Linus Torvalds]

On Thu, 26 May 2005, Benjamin Herrenschmidt wrote:
> 
> If it's ok with you, I'll send a patch doing it later today.

It's ok by me, certainly. There aren't that many users, and it sounds
sane. I'll just prefer the patch going through Greg..

		Linus

Re: [RFC] Changing pci_iounmap to take 'bar' argument

[Benjamin Herrenschmidt]

On Wed, 2005-05-25 at 21:25 -0700, Linus Torvalds wrote:
> 
> On Thu, 26 May 2005, Benjamin Herrenschmidt wrote:
> > 
> > If it's ok with you, I'll send a patch doing it later today.
> 
> It's ok by me, certainly. There aren't that many users, and it sounds
> sane. I'll just prefer the patch going through Greg..

Ok, just wanted some feedback from you. Some people prefer that I whack
some "token" in the vitual address at map time, or that I compare the
vaddr at unmap time with all PCI busses IO ranges or that sort of ugly
thing, it sounds to me simpler to just pass along the bar number, but I
wanted your and Greg's ack first.

Ben.

Re: [RFC] Changing pci_iounmap to take 'bar' argument

[Benjamin Herrenschmidt]

On Thu, 2005-05-26 at 14:27 +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2005-05-25 at 21:25 -0700, Linus Torvalds wrote:
> > 
> > On Thu, 26 May 2005, Benjamin Herrenschmidt wrote:
> > > 
> > > If it's ok with you, I'll send a patch doing it later today.
> > 
> > It's ok by me, certainly. There aren't that many users, and it sounds
> > sane. I'll just prefer the patch going through Greg..
> 
> Ok, just wanted some feedback from you. Some people prefer that I whack
> some "token" in the vitual address at map time, or that I compare the
> vaddr at unmap time with all PCI busses IO ranges or that sort of ugly
> thing, it sounds to me simpler to just pass along the bar number, but I
> wanted your and Greg's ack first.

Oh, and MIPS seems to be broken here ... it's like ppc, it's ioremap'ing
MMIO and just using an existing mapped stuff for IO, but unconditionally
iounmap's on pci_iounmap()... unless there is some arch black magic in
there, that seems broken. Ralph, should I fix it while I'm at it ?

Ben.

Re: [RFC] Changing pci_iounmap to take 'bar' argument

[Benjamin Herrenschmidt]

> > Ok, just wanted some feedback from you. Some people prefer that I whack
> > some "token" in the vitual address at map time, or that I compare the
> > vaddr at unmap time with all PCI busses IO ranges or that sort of ugly
> > thing, it sounds to me simpler to just pass along the bar number, but I
> > wanted your and Greg's ack first.
> 
> Oh, and MIPS seems to be broken here ... it's like ppc, it's ioremap'ing
> MMIO and just using an existing mapped stuff for IO, but unconditionally
> iounmap's on pci_iounmap()... unless there is some arch black magic in
> there, that seems broken. Ralph, should I fix it while I'm at it ?

Hrm... in fact, It complicates life for drivers. There already a few
using it, and there are cases, like sym53c8xx_2, that do something like

	int bar = dodgy_logic_to_find_what_bar_to_use();

	foo = pci_iomap(dev, bar, pci_resource_len(dev, bar));

And in a completely different function, a simple

	pci_iounmap(dev, foo);

To pass the bar in there requires to either add a field to the driver
"instance" structure or to reproduce the "dodgy logic".

I've fixed them all, but I don't like the patch that much.

It may be simpler indeed for me to actually only complicate the ppc &
ppc64 pci_iounmap() implementation and have it compare the virtual
address against known PCI IO spaces...

Ben.

Re: [RFC] Changing pci_iounmap to take 'bar' argument

[Linus Torvalds]

On Thu, 26 May 2005, Benjamin Herrenschmidt wrote:
> 
> 	foo = pci_iomap(dev, bar, pci_resource_len(dev, bar));

Btw, this kind of code should be just

	foo = pci_iomap(dev, bar, 0);

because the third argument is _not_ a length, it's a _maximum_ length we 
need to map, with zero meaning "everything" (as does ~0ul, of course, but 
zero is obviously easier).

The only people who want to use non-zero are things like frame-buffers
that might have hundreds of megabytes of memory, but the kernel only needs
to map the actual thing visible on the screen.

> And in a completely different function, a simple
> 
> 	pci_iounmap(dev, foo);
> 
> It may be simpler indeed for me to actually only complicate the ppc &
> ppc64 pci_iounmap() implementation and have it compare the virtual
> address against known PCI IO spaces...

Yeah. It shouldn't be a problem on 64-bit architectures, and even on 
32-bit ones the IO-port range really _should_ be fairly small, and a small 
amount of special casing should hopefully fix it.

A lot of architectures end up having to separate PIO and MMIO anyway,
which is - together with hysterical raisins, of course - why the existing
interfaces just assumed that was possible.

		Linus

Re: [RFC] Changing pci_iounmap to take 'bar' argument

[Russell King]

On Thu, May 26, 2005 at 08:21:07AM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 26 May 2005, Benjamin Herrenschmidt wrote:
> > 
> > 	foo = pci_iomap(dev, bar, pci_resource_len(dev, bar));
> 
> Btw, this kind of code should be just
> 
> 	foo = pci_iomap(dev, bar, 0);
> 
> because the third argument is _not_ a length, it's a _maximum_ length we 
> need to map, with zero meaning "everything" (as does ~0ul, of course, but 
> zero is obviously easier).
> 
> The only people who want to use non-zero are things like frame-buffers
> that might have hundreds of megabytes of memory, but the kernel only needs
> to map the actual thing visible on the screen.

Note also that framebuffer drivers may also wish to map a bar from a
specific offset and length.  Eg, CyberPro has one BAR which is something
like 16MB large, with the framebuffer in the low addresses and the
registers at 8MB in.  (poor example I know.)

I believe the IXP folk have issues similar to this though, but more
extreme.

> Yeah. It shouldn't be a problem on 64-bit architectures, and even on 
> 32-bit ones the IO-port range really _should_ be fairly small, and a small 
> amount of special casing should hopefully fix it.
> 
> A lot of architectures end up having to separate PIO and MMIO anyway,
> which is - together with hysterical raisins, of course - why the existing
> interfaces just assumed that was possible.

Maybe the interface is just wrong.  Maybe it should be:

struct map {
	void __iomem *cpu;
	/* whatever platform data is required */
};

	err = pci_iomap(dev, bar, size, &map);
...
	pci_iounmap(&map);

The reason I suggest this is that we've had problems with the PCI DMA API -
remember that driver people whinged about having to store the dma_addr_t
cookie and we introduced the following crap:

#define DECLARE_PCI_UNMAP_ADDR(ADDR_NAME)       dma_addr_t ADDR_NAME;
#define DECLARE_PCI_UNMAP_LEN(LEN_NAME)         __u32 LEN_NAME;
#define pci_unmap_addr(PTR, ADDR_NAME)          ((PTR)->ADDR_NAME)
#define pci_unmap_addr_set(PTR, ADDR_NAME, VAL) (((PTR)->ADDR_NAME) = (VAL))
#define pci_unmap_len(PTR, LEN_NAME)            ((PTR)->LEN_NAME)
#define pci_unmap_len_set(PTR, LEN_NAME, VAL)   (((PTR)->LEN_NAME) = (VAL))

Let's not make this same mistake again!  (just like we unknowingly
repeated it for the new DMA API... which should have contained the
returned cookies - the platform specific data which may be just the
CPU address for trivial x86 cases - inside a structure with macros
for accessing them.  Magically all the above mess vanishes.)


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [RFC] Changing pci_iounmap to take 'bar' argument

[Greg KH]

On Thu, May 26, 2005 at 02:07:34PM +1000, Benjamin Herrenschmidt wrote:
> Hi !
> 
> On ppc and ppc64 platforms, pci_iounmap() currently does nothing, which
> is bogus (leak of ioremap space for mmio). It needs to iounmap for MMIOs
> and do nothign for IO space.
> 
> The problem is that wether it's IO or MMIO cannot be easily deduced from
> the virtual address. We _could_ change the whole thing on ppc32 to play
> tricks with the top address bits, and we could compare the virtual
> address with the known regions containing PHBs IO space, but that sounds
> to me like working around a bad API in the first place.
> 
> What about, instead, just adding the "int bar" argument to pci_iounmap()
> like we pass to pci_iomap() so it can access the resource flags ?
> 
> If it's ok with you, I'll send a patch doing it later today.

Fine with me.

thanks,

greg k-h