[PATCH] /drivers/char sx.c ioremap -> pci_ioremap api

[PATCH] /drivers/char sx.c ioremap -> pci_ioremap api

[Scott Thompson]

patchset against 2.6.23-rc3.

moves the iomap/iounmap call to pci 'flavor'.  this was a
recommendation from a previously submitted patch that handles
the unchecked iomap (or, now, pci_iomap) return code.

Signed-off-by: Scott Thompson <postfail <at> hushmail.com>
----------------------------------------------------------

diff --git a/drivers/char/sx.c b/drivers/char/sx.c
index 85a2328..481334f 100644
--- a/drivers/char/sx.c
+++ b/drivers/char/sx.c
@@ -2626,14 +2626,14 @@ static void __devinit fix_sx_pci(struct pci_dev *pdev, struct sx_board *board)

  	pci_read_config_dword(pdev, PCI_BASE_ADDRESS_0, &hwbase);
  	hwbase &= PCI_BASE_ADDRESS_MEM_MASK;
-	rebase = ioremap(hwbase, 0x80);
+	rebase = pci_iomap(pdev, 0, 0x80);
  	t = readl(rebase + CNTRL_REG_OFFSET);
  	if (t != CNTRL_REG_GOODVALUE) {
  		printk(KERN_DEBUG "sx: performing cntrl reg fix: %08x -> "
  			"%08x\n", t, CNTRL_REG_GOODVALUE);
  		writel(CNTRL_REG_GOODVALUE, rebase + CNTRL_REG_OFFSET);
  	}
-	iounmap(rebase);
+	pci_iounmap(pdev, rebase);
  }
  #endif

Re: [PATCH] /drivers/char sx.c ioremap -> pci_ioremap api

[Jiri Slaby]

On 8/24/07, Scott Thompson <postfailathushmail@gmail.com> wrote:
>
> patchset against 2.6.23-rc3.
>
> moves the iomap/iounmap call to pci 'flavor'.  this was a
> recommendation from a previously submitted patch that handles
> the unchecked iomap (or, now, pci_iomap) return code.
>
> Signed-off-by: Scott Thompson <postfail <at> hushmail.com>
> ----------------------------------------------------------
>
> diff --git a/drivers/char/sx.c b/drivers/char/sx.c
> index 85a2328..481334f 100644
> --- a/drivers/char/sx.c
> +++ b/drivers/char/sx.c
> @@ -2626,14 +2626,14 @@ static void __devinit fix_sx_pci(struct pci_dev
> *pdev, struct sx_board *board)
>
>   	pci_read_config_dword(pdev, PCI_BASE_ADDRESS_0, &hwbase);
>   	hwbase &= PCI_BASE_ADDRESS_MEM_MASK;
> -	rebase = ioremap(hwbase, 0x80);

remove also the hwbase var.

> +	rebase = pci_iomap(pdev, 0, 0x80);
>   	t = readl(rebase + CNTRL_REG_OFFSET);
>   	if (t != CNTRL_REG_GOODVALUE) {
>   		printk(KERN_DEBUG "sx: performing cntrl reg fix: %08x -> "
>   			"%08x\n", t, CNTRL_REG_GOODVALUE);
>   		writel(CNTRL_REG_GOODVALUE, rebase + CNTRL_REG_OFFSET);
>   	}
> -	iounmap(rebase);
> +	pci_iounmap(pdev, rebase);
>   }
>   #endif
>


-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: [PATCH] /drivers/char sx.c ioremap -> pci_ioremap api

[Alan Cox]

ase, 0x80);
> 
> remove also the hwbase var.
> 
> > +	rebase = pci_iomap(pdev, 0, 0x80);
> >   	t = readl(rebase + CNTRL_REG_OFFSET);

Switch to ioread* if you are using the iomap interface. Its not a trivial
conversion and its slower and bulkier - the original ioremap was much
better

NAK this change therefore

Re: [PATCH] /drivers/char sx.c ioremap -> pci_ioremap api

[Scott Thompson]

On Fri, 24 Aug 2007 15:15:30 -0400 Alan Cox 
<alan@lxorguk.ukuu.org.uk> wrote:
>ase, 0x80);
>> 
>> remove also the hwbase var.
>> 
>> > +	rebase = pci_iomap(pdev, 0, 0x80);
>> >   	t = readl(rebase + CNTRL_REG_OFFSET);
>
>Switch to ioread* if you are using the iomap interface. Its not a 
>trivial
>conversion and its slower and bulkier - the original ioremap was 
>much
>better
>
>NAK this change therefore

Jiri had requested this in relation to previous patch on unchecked 
ioremap return codes, but if the original ioremap is better NAK is 
fine
here and I won't resubmit w/ the hwbase var pulled out or the readl 
-> ioread32 switchover.

---------------------------------------
Scott Thompson / postfail@hushmail.com
---------------------------------------

Re: [PATCH] /drivers/char sx.c ioremap -> pci_ioremap api

[Jiri Slaby]

Alan Cox napsal(a):
> ase, 0x80);
>> remove also the hwbase var.
>>
>>> +	rebase = pci_iomap(pdev, 0, 0x80);
>>>   	t = readl(rebase + CNTRL_REG_OFFSET);
> 
> Switch to ioread* if you are using the iomap interface. Its not a trivial

Why, if you know it's surely a mem region (and thus you rely on it and do
ioremap)? There are many places in the kernel, where this approach is used, e.g.
libata piix.

> conversion and its slower and bulkier - the original ioremap was much
> better

at least get rid of the reading the hwbase address from pci conf space, use
pci_resource_start instead.

-- 
Jiri Slaby (jirislaby@gmail.com)
Faculty of Informatics, Masaryk University

Re: [PATCH] /drivers/char sx.c ioremap -> pci_ioremap api

[Alan Cox]

On Fri, 24 Aug 2007 22:03:42 +0200
Jiri Slaby <jirislaby@gmail.com> wrote:

> Alan Cox napsal(a):
> > ase, 0x80);
> >> remove also the hwbase var.
> >>
> >>> +	rebase = pci_iomap(pdev, 0, 0x80);
> >>>   	t = readl(rebase + CNTRL_REG_OFFSET);
> > 
> > Switch to ioread* if you are using the iomap interface. Its not a trivial
> 
> Why, if you know it's surely a mem region (and thus you rely on it and do
> ioremap)? There are many places in the kernel, where this approach is used, e.g.
> libata piix.

Every single one of them is wrong. The encoding of iomap values is
platform dependant. Fixed in my tree.
> 
> > conversion and its slower and bulkier - the original ioremap was much
> > better
> 
> at least get rid of the reading the hwbase address from pci conf space, use
> pci_resource_start instead.

Agreed entirely

readXs on pci*iomap-ped regions [Was: [PATCH] /drivers/char sx.c ioremap -> pci_ioremap api]

[Jiri Slaby]

Alan Cox napsal(a):
>>>>> +	rebase = pci_iomap(pdev, 0, 0x80);
>>>>>   	t = readl(rebase + CNTRL_REG_OFFSET);
>>> Switch to ioread* if you are using the iomap interface. Its not a trivial
>> Why, if you know it's surely a mem region (and thus you rely on it and do
>> ioremap)? There are many places in the kernel, where this approach is used, e.g.
>> libata piix.
> 
> Every single one of them is wrong. The encoding of iomap values is
> platform dependant. Fixed in my tree.

OK, fast search found these suspicious (not considering e.g. arch/):
$ find drivers/ -name '*.c' -exec grep -q pci.*iomap {} \; -a -exec grep -l
'\<readl\>' {} \+
drivers/firewire/fw-ohci.c
drivers/scsi/sym53c8xx_2/sym_glue.c
drivers/media/dvb/pluto2/pluto2.c
drivers/media/dvb/b2c2/flexcop-pci.c
drivers/ssb/scan.c
drivers/char/cyclades.c
drivers/char/sx.c
drivers/ata/ata_piix.c
drivers/ata/sata_inic162x.c
drivers/ata/pata_pdc2027x.c
drivers/ata/sata_sx4.c
drivers/ata/sata_qstor.c
drivers/ata/sata_mv.c
drivers/ata/sata_svw.c
drivers/ata/sata_nv.c
drivers/ata/sata_sil.c
drivers/ata/sata_vsc.c
drivers/ata/sata_promise.c
drivers/ata/ahci.c
drivers/ata/sata_sil24.c
drivers/net/cassini.c
drivers/mtd/nand/cafe_nand.c

How do you imagine a proper fix?
- move to ioreadX/iowriteX
or
- move back to ioremap (unlikely for the most)
or?

thanks,
-- 
Jiri Slaby (jirislaby@gmail.com)
Faculty of Informatics, Masaryk University

Re: readXs on pci*iomap-ped regions [Was: [PATCH] /drivers/char sx.c ioremap -> pci_ioremap api]

[Jeff Garzik]

If the driver knows its MMIO, using readX/writeX after pci_iomap() is 
just fine, for all current implementations, and it makes sense that way.

	Jeff

Re: readXs on pci*iomap-ped regions [Was: [PATCH] /drivers/char sx.c ioremap -> pci_ioremap api]

[Jiri Slaby]

Jeff Garzik napsal(a):
> If the driver knows its MMIO, using readX/writeX after pci_iomap() is
> just fine, for all current implementations, and it makes sense that way.

Hmm, that's what I'm claiming.

-- 
Jiri Slaby (jirislaby@gmail.com)
Faculty of Informatics, Masaryk University

Re: readXs on pci*iomap-ped regions [Was: [PATCH] /drivers/char sx.c ioremap -> pci_ioremap api]

[Alan Cox]

On Sat, 25 Aug 2007 04:56:19 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> If the driver knows its MMIO, using readX/writeX after pci_iomap() is 
> just fine, for all current implementations, and it makes sense that way.

There is nothing that guarantees this is permitted, any more than there
is anything saying not to use outb/outl. Some of the implementations do
quite strange things. It may happen to work but its not in the
documentation or the comments.

If you want to change this then you need to check the existing usages and
update all the docs if its safe, oh and tell the sparc64 pcmcia people to
take a hike, which is probably not a big problem.

Alan