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
On 8/24/07, Scott Thompson <[email protected]> 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
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
On Fri, 24 Aug 2007 15:15:30 -0400 Alan Cox
<[email protected]> 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 / [email protected]
---------------------------------------
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 ([email protected])
Faculty of Informatics, Masaryk University
On Fri, 24 Aug 2007 22:03:42 +0200
Jiri Slaby <[email protected]> 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
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 ([email protected])
Faculty of Informatics, Masaryk University
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
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 ([email protected])
Faculty of Informatics, Masaryk University
On Sat, 25 Aug 2007 04:56:19 -0400
Jeff Garzik <[email protected]> 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