lkml.org 
[lkml]   [1999]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: pci_scan, tulip, 3c59x, multiple detections and others
"Petr Vandrovec Ing. VTEI" wrote:
>
> On 28 Sep 99 at 16:12, Alan Cox wrote:
> > > Using physbase instead of ioaddr is important part of change... But adding
> > > this to all Donalds drivers does not look like intelligent solution to me...
> > > pci_scan now does check_{mem_,}region, so why it could not do
> > > request_{mem_,}region too? When we can add five lines into pci's scan.c
> > > and remove 8 lines from each network driver, benefit looks clear to me.
> > You need to remember to free it - at that point you do need to cover physical
> > addresses properly
> Same way as you have to remember to iounmap it now...
> Now ethernet drivers do request_mem_region with completely meaningless address
> from another address space than /proc/iomem covers. I saw report with this
> request_mem_region failing to register ioremapped memory at 0xd2xxxxxx
> because of BIOS assigned physical range d0000000-d7ffffff to another device.

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/

\
 
 \ /
  Last update: 2005-03-22 13:54    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site