Skip to contentSkip to conversation

/drivers/char sx.c ioremap -> pci_ioremap api

PATCH In review Scott Thompson 4 participants 9 messages Aug 24, 2007 – Aug 25, 2007

Subsystems (3)
Kernel janitors libata Char and misc drivers
lore
Share & tools
lore

Scott Thompson

author
#0 lore raw

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

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>
----------------------------------------------------------

drivers/char/sx.c +2 -2
index 85a2328..481334f 100644
@@ -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
#1 lore raw
Quoted 15 lines · 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>
----------------------------------------------------------

drivers/char/sx.c +0 -0
index 85a2328..481334f 100644

drivers/char/sx.c static void __devinit fix_sx_pci(struct pci_dev · line 2630

Quoted 5 lines
drivers/char/sx.c +0 -1
@@ -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.

Quoted 12 lines
drivers/char/sx.c +2 -1
+	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
Signature · 4 lines
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
#2 lore raw

ase, 0x80);

Quoted 5 lines

remove also the hwbase var.

drivers/char/sx.c static void __devinit fix_sx_pci(struct pci_dev *pdev, struct sx_board *board) · line 2630

Quoted 2 lines
drivers/char/sx.c +1 -0
+	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

#3 lore raw

Alan Cox napsal(a):

Quoted 7 lines

ase, 0x80);

Quoted 4 lines

remove also the hwbase var.

drivers/char/sx.c static void __devinit fix_sx_pci(struct pci_dev *pdev, struct sx_board *board) · line 2630

Quoted 2 lines
drivers/char/sx.c +1 -0
+	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.

Quoted 2 lines

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.

Signature · 2 lines
Jiri Slaby ([email protected])
Faculty of Informatics, Masaryk University
#4 lore raw
Quoted 4 lines · On Fri, 24 Aug 2007 22:03:42 +0200 Jiri Slaby <[email protected]> wrote:

Alan Cox napsal(a):

Quoted 3 lines

ase, 0x80);

Quoted 2 lines

remove also the hwbase var.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

drivers/char/sx.c static void __devinit fix_sx_pci(struct pci_dev *pdev, struct sx_board *board) · line 2630

Quoted 2 lines
drivers/char/sx.c +1 -0
+	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.

Quoted 6 lines
Quoted 3 lines

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

#5 lore raw

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

Alan Cox napsal(a):

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Quoted 2 lines

drivers/char/sx.c static void __devinit fix_sx_pci(struct pci_dev *pdev, struct sx_board *board) · line 2630

Quoted 2 lines
drivers/char/sx.c +1 -0
+	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,

Signature · 2 lines
Jiri Slaby ([email protected])
Faculty of Informatics, Masaryk University
#6 lore raw

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

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

#7 lore raw

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

Jeff Garzik napsal(a):

Quoted 2 lines

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.

Signature · 2 lines
Jiri Slaby ([email protected])
Faculty of Informatics, Masaryk University
#8 lore raw

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

In reply to Jeff Garzik
Quoted 2 lines · 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