Also, the following code from epic100.c worries me:
+#define request_region request_mem_region
+#define release_region release_mem_region
Mixing I/O addresses and MMIO in the same code is a really bad idea
IMHO. It obscures code, possibly hiding flaws when these defines are
active. You can find other code like this too.
Most of the net code seems to be structure with pio in mind, and MMIO
added as an afterthought. This code design might be an underlying cause
of all the problems wrt. MMIO, and physical vs. virtual addresses.
Check over the kernel archives for the recent PCI probing thread, mainly
between myself, Donald, and Alan. There are several issues related to
MMIO and the current scan code.
> So what is correct solution from your point of view? Remove ioremap()
> from drivers/pci/scan.c and move it to all ethernet drivers or add
> request_resource to drivers/pci/scan.c and remove it from all ethernet
> drivers?
I favor the latter solution, but solving it that way probably bloat the
pci scan structure.
> From my point of view it should
> (1) store device physical address into netdevice's base_address. This
> ensures backward compatibility and useful data exported to userspace.
> Reporting kernel virtual address to userspace is useless.
Yes
> (2) store device virtual address into netdevice's priv field, if device
> does not use it priv structure. Maybe specific vbase_addr field is better,
> network device drivers can probably provide better comments on this (fbdev
> has base & vbase... base is exported to userspace & passed to allocation/
> deallocation functions, vbase is really used)
If the driver uses MMIO, it should be using writel() and friends, and
also calling iounmap(). Using 'vbase' or 'virt_base' or similar is best
IMHO.
> (3) drivers/pci/scan.c passes physical address to driver.
> Driver can create virtual mapping by ioremap if it needs it.
Nah, it can be done generically in scan.c... If it uses MMIO, it
_should_ be calling ioremap(). In a thread long past, I believe Alan
said:
Alan> Using ioremap for all cases is almost certainly going to be
required in 2.3.x.
Alan> The fact its not always needed right now is one of the biggest
obstacles
Alan> to supporting a more sane range of memory sizes
Granted it is now 2.3.x post-feature-freeze, but we might as well start
doing things The Right Way(tm) now. That is, religiously following
Documentation/IO-mapping.txt at any rate.
> (4) it is driver task to determine resource type (by checking
> pci_tbl[chip_idx].pci_flags), call appropriate request_{mem,}region
> and ioremap. If probe function fails, it must iounmap virtual
> mapping and release registered regions.
In the PCI probe thread Donald said there was a reason why he didn't do
a request_region in scan.c (but he didn't elaborate)
> (5) on driver unload it's driver's task to release regions and
> iounmap virtual mappings.
Whichever code obtains the mappings should also release them.
> (4) it is driver task to determine resource type and choose
> readb/inb depending on it. If driver needs physical address,
> it can find it by doing
> pdev->resource[(pci_tbl[chip_idx].pci_flags >> 4) & 7].start
> If probe function fails, it must NOT passed virtual mapping
> and must NOT release passed region.
If scan.c sets up virt_base automatically, there need only be a flag
that tells the driver whether or not to use pio.
Regards,
Jeff
-- Custom driver development | Never worry about theory as long Open source programming | as the machinery does what it's | supposed to do. -- R. A. Heinlein- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/