Exposing ROM's though sysfs

We talked at OLS about exposing adapter ROMs via sysfs. I have some time to work on this but I'm not sure about the right places to hook into the kernel.

The first problem is recording the boot video device. This is needed for laptops that compress their video and system ROMs together into a single ROM. When sysfs exposes the ROM for these adapters it needs to know the boot video device so that it can return the ROM image at C000:0 instead of trying to find an actual ROM. What is the right kernel structure for recording the boot video device? Where should this code live? It is probably x86 specific but have non-x86 laptops done the same trick?

What about ISA support. Should we make an attempt to return ROM contents from ISA cards?

Note that not just video cards can have ROMs. Disk adapters commonly have them too. We probably want to expose these ROMs too.

Do we want to expose the system ROM via sysfs? Where should it appear?

Some Radeon cards have a bug where they forgot to clear a latch which makes the ROMs visible. Where should a fix for things like this go? I can put it in the radeon driver but if you try to read the ROM before the driver is loaded, the ROM won't be visible.

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
Yahoo! Mail Address AutoComplete - You start. We finish.
http://promotions.yahoo.com/new_mail

How about this patch?

If we have the pci quirks stuff fill in pci_dev->resource[PCI_ROM_RESOURCE] with the right addresses, we should be ok.

That's probably more trouble than it's worth, but if someone really wants it they could always code it up.

Jesse

===== drivers/pci/pci-sysfs.c 1.10 vs edited =====

@@ -164,6 +164,61 @@
 	return count;
 }
 
+static ssize_t
+pci_read_rom(struct kobject *kobj, char *buf, loff_t off, size_t count)
+{
+	struct pci_dev *dev = to_pci_dev(container_of(kobj,struct device,kobj));
+	loff_t init_off = off;
+	unsigned long start = dev->resource[PCI_ROM_RESOURCE].start;
+	int size = dev->resource[PCI_ROM_RESOURCE].end -
+		dev->resource[PCI_ROM_RESOURCE].start;
+
+	if (off > size)
+		return 0;
+	if (off + count > size) {
+		size -= off;
+		count = size;
+	} else {
+		size = count;
+	}
+
+	/* Enable ROM space decodes and do the reads */
+	pci_write_config_dword(dev, 1, PCI_ROM_ADDRESS_ENABLE);
+
+	while (off & 3) {
+		unsigned char val;
+		val = readb(start + off);
+		buf[off - init_off] = val;
+		off++;
+		if (--size == 0)
+			break;
+	}
+
+	while (size > 3) {
+		unsigned int val;
+		val = readl(start + off);
+		buf[off - init_off] = val & 0xff;
+		buf[off - init_off + 1] = (val >> 8) & 0xff;
+		buf[off - init_off + 2] = (val >> 16) & 0xff;
+		buf[off - init_off + 3] = (val >> 24) & 0xff;
+		off += 4;
+		size -= 4;
+	}
+
+	while (size > 0) {
+		unsigned char val;
+		val = readb(start + off);
+		buf[off - init_off] = val;
+		off++;
+		--size;
+	}
+
+	/* Disable again before continuing */
+	pci_write_config_dword(dev, 1, ~PCI_ROM_ADDRESS_ENABLE);
+
+	return count;
+}
+
 static struct bin_attribute pci_config_attr = {
 	.attr =	{
 		.name = "config",

@@ -186,12 +241,28 @@
 	.write = pci_write_config,
 };
 
+static struct bin_attribute pci_rom_attr = {
+	.attr =	{
+		.name = "rom",
+		.mode = S_IRUGO | S_IWUSR,
+		.owner = THIS_MODULE,
+	},
+	.read = pci_read_rom,
+};
+
 void pci_create_sysfs_dev_files (struct pci_dev *pdev)
 {
 	if (pdev->cfg_size < 4096)
 		sysfs_create_bin_file(&pdev->dev.kobj, &pci_config_attr);
 	else
 		sysfs_create_bin_file(&pdev->dev.kobj, &pcie_config_attr);
+
+	/* If the device has a ROM, map it */
+	if (pdev->resource[PCI_ROM_RESOURCE].start) {
+		pci_rom_attr.size = pdev->resource[PCI_ROM_RESOURCE].end -
+			pdev->resource[PCI_ROM_RESOURCE].start;
+		sysfs_create_bin_file(&pdev->dev.kobj, &pci_rom_attr);
+	}
 
 	/* add platform-specific attributes */
 	pcibios_add_platform_entries(pdev);

Oops, forgot about those macros. Fixed. I've also fixed the perms, RO by root.

willy brings up a good point though, we may want to disable the rom file entirely after its been read, since it can be a source of trouble. Here's the latest patch.

Jesse

===== drivers/pci/pci-sysfs.c 1.10 vs edited =====

@@ -164,6 +164,73 @@
 	return count;
 }
 
+static void
+pci_enable_rom(struct pci_dev *dev)
+{
+	pci_write_config_dword(dev, PCI_ROM_ADDRESS, PCI_ROM_ADDRESS_ENABLE);
+}
+
+static void
+pci_disable_rom(struct pci_dev *dev)
+{
+	pci_write_config_dword(dev, PCI_ROM_ADDRESS, ~PCI_ROM_ADDRESS_ENABLE);
+}
+
+static ssize_t
+pci_read_rom(struct kobject *kobj, char *buf, loff_t off, size_t count)
+{
+	struct pci_dev *dev = to_pci_dev(container_of(kobj,struct device,kobj));
+	loff_t init_off = off;
+	unsigned long start = pci_resource_start(dev, PCI_ROM_RESOURCE);
+	int size = pci_resource_len(dev, PCI_ROM_RESOURCE);
+	u32 l;
+
+	if (off > size)
+		return 0;
+	if (off + count > size) {
+		size -= off;
+		count = size;
+	} else {
+		size = count;
+	}
+
+	/* Enable ROM space decodes and do the reads */
+	pci_enable_rom(dev);
+
+	while (off & 3) {
+		unsigned char val;
+		val = readb(start + off);
+		buf[off - init_off] = val;
+		off++;
+		if (--size == 0)
+			break;
+	}
+
+	while (size > 3) {
+		unsigned int val;
+		val = readl(start + off);
+		buf[off - init_off] = val & 0xff;
+		buf[off - init_off + 1] = (val >> 8) & 0xff;
+		buf[off - init_off + 2] = (val >> 16) & 0xff;
+		buf[off - init_off + 3] = (val >> 24) & 0xff;
+		off += 4;
+		size -= 4;
+	}
+
+	while (size > 0) {
+		unsigned char val;
+		val = readb(start + off);
+		buf[off - init_off] = val;
+		off++;
+		--size;
+	}
+
+	/* Disable again before continuing */
+	pci_disable_rom(dev);
+
+	return count;
+}
+
 static struct bin_attribute pci_config_attr = {
 	.attr =	{
 		.name = "config",

@@ -186,12 +253,27 @@
 	.write = pci_write_config,
 };
 
+static struct bin_attribute pci_rom_attr = {
+	.attr =	{
+		.name = "rom",
+		.mode = S_IRUSR,
+		.owner = THIS_MODULE,
+	},
+	.read = pci_read_rom,
+};
+
 void pci_create_sysfs_dev_files (struct pci_dev *pdev)
 {
 	if (pdev->cfg_size < 4096)
 		sysfs_create_bin_file(&pdev->dev.kobj, &pci_config_attr);
 	else
 		sysfs_create_bin_file(&pdev->dev.kobj, &pcie_config_attr);
+
+	/* If the device has a ROM, map it */
+	if (pdev->resource[PCI_ROM_RESOURCE].start) {
+		pci_rom_attr.size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+		sysfs_create_bin_file(&pdev->dev.kobj, &pci_rom_attr);
+	}
 
 	/* add platform-specific attributes */
 	pcibios_add_platform_entries(pdev);

+ if (pdev->resource[PCI_ROM_RESOURCE].start) {

And before you say anything, I've already fixed this to use pci_resource_len(pdev, PCI_ROM_RESOURCE) instead.

Jesse

My problem is with this is the following passage from PCI 2.2 and PCI 2.3:

In order to minimize the number of address decoders needed, a device may share a decoder between the Expansion ROM Base Address register and other Base Address registers. When expansion ROM decode is enabled, the decoder is used for accesses to the expansion ROM and device independent software must not access the device through any other Base Address registers.

ie while we're reading the ROM, we have to prevent any other accesses to the contents of the BARs. This patch is a loaded gun, pointed right at your face. Sure, we can say it's root's fault for pulling the trigger, but it'd be nice to have some kind of safety lock.

I don't see a good way of doing this, unfortunately. It'd probably be enough to call ->suspend on the driver while you read the contents of the ROM, but that's pretty ugly.

How about reading the contents of the ROM at pci_scan_bus() time? It'd waste a bunch of memory, but hey, people love sysfs.

"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

That might be a good solution, actually. Then it would be cached for devices that don't want you to look at it after they've been POSTed too.

Jesse

Hello!

There already exists a "pci=rom" switch which tells the PCI core to assign addresses to the ROM BAR's as well. Either we can offer the files in the sysfs only if this switch is enabled, or we can create another switch for making copies of the ROM's during system boot.

Anyway, the devices sharing ROM decoder with another BAR seem to be pretty rare.

Have a nice fortnight

Martin `MJ' Mares   <[email protected]>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
Anti-trust laws should be approached with exactly that attitude.

Here's a version that actually works, but without the caching (i.e. don't access it after a driver starts using the card). If we go that route, should we hang a copy of the ROM off of the pci_dev in a pointer or somewhere else?

Thanks,
Jesse

===== drivers/pci/pci-sysfs.c 1.10 vs edited =====

@@ -164,6 +164,60 @@
 	return count;
 }
 
+static void
+pci_enable_rom(struct pci_dev *dev)
+{
+	u32 rom_addr;
+
+	pci_read_config_dword(dev, PCI_ROM_ADDRESS, &rom_addr);
+	rom_addr |= PCI_ROM_ADDRESS_ENABLE;
+	pci_write_config_dword(dev, PCI_ROM_ADDRESS, rom_addr);
+}
+
+static void
+pci_disable_rom(struct pci_dev *dev)
+{
+	u32 rom_addr;
+
+	pci_read_config_dword(dev, PCI_ROM_ADDRESS, &rom_addr);
+	rom_addr &= ~PCI_ROM_ADDRESS_ENABLE;
+	pci_write_config_dword(dev, PCI_ROM_ADDRESS, rom_addr);
+}
+
+static ssize_t
+pci_read_rom(struct kobject *kobj, char *buf, loff_t off, size_t count)
+{
+	struct pci_dev *dev = to_pci_dev(container_of(kobj,struct device,kobj));
+	loff_t init_off = off;
+	unsigned long start = pci_resource_start(dev, PCI_ROM_RESOURCE);
+	int size = pci_resource_len(dev, PCI_ROM_RESOURCE);
+
+	if (off > size)
+		return 0;
+	if (off + count > size) {
+		size -= off;
+		count = size;
+	} else {
+		size = count;
+	}
+
+	/* Enable ROM space decodes and do the reads */
+	pci_enable_rom(dev);
+
+	while (size > 0) {
+		unsigned char val;
+		val = readb(start + off);
+		buf[off - init_off] = val;
+		off++;
+		--size;
+	}
+
+	/* Disable again before continuing */
+	pci_disable_rom(dev);
+
+	return count;
+}
+
 static struct bin_attribute pci_config_attr = {
 	.attr =	{
 		.name = "config",

@@ -186,12 +240,27 @@
 	.write = pci_write_config,
 };
 
+static struct bin_attribute pci_rom_attr = {
+	.attr =	{
+		.name = "rom",
+		.mode = S_IRUSR,
+		.owner = THIS_MODULE,
+	},
+	.read = pci_read_rom,
+};
+
 void pci_create_sysfs_dev_files (struct pci_dev *pdev)
 {
 	if (pdev->cfg_size < 4096)
 		sysfs_create_bin_file(&pdev->dev.kobj, &pci_config_attr);
 	else
 		sysfs_create_bin_file(&pdev->dev.kobj, &pcie_config_attr);
+
+	/* If the device has a ROM, map it */
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE)) {
+		pci_rom_attr.size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+		sysfs_create_bin_file(&pdev->dev.kobj, &pci_rom_attr);
+	}
 
 	/* add platform-specific attributes */
 	pcibios_add_platform_entries(pdev);

The ROMs are definitely different sizes from 2K minimum to 128KB, maybe even larger. Varying sizes needs to be handled.

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
Yahoo! Mail is new and improved - Check it out!
http://promotions.yahoo.com/new_mail

Yep, that's pretty broken. I guess I need to allocate a pci_rom_attr every time we see a ROM... Where would the cleanup code go though? In one of the hotplug remove paths?

Jesse

We need to create a pci_remove_sysfs_dev_files() call, and call it when the pci device is about to be unregistered (in pci_destroy_dev(), just before the call to device_unregister()).

thanks,

greg k-h

Alan Cox knows more about this, but I believe there is only one PCI card in existence that does this.

For the one or two cards that do this, the device drivers could flag this to the PCI subsystem. In the flagged case the sysfs read code could shut off interrupts, enable the ROM, copy it, and then reenable interrupts.

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
Yahoo! Mail - 50x more storage than other providers!
http://promotions.yahoo.com/new_mail

Strange; he was the one who pointed out this requirement to me in the first place and he hinted that many devices did this.

Shutting off interrupts isn't nearly enough. Any other CPU could access the device, or indeed any device capable of DMA could potentially cause trouble.

"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Alan, what's the answer here?

Another idea, it's ok to read the ROM when there is no device driver loaded. When the driver for one of these card loads it could trigger a copy of the ROM into RAM and cache it in a PCI structure.

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
Yahoo! Mail - Helps protect you from nasty viruses.
http://promotions.yahoo.com/new_mail

Hello!

I really doubt it's worth the RAM wasted by the automatic caching of ROM's which will be probably left unused in 99.9% of cases.

Have a nice fortnight

Martin `MJ' Mares   <[email protected]>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
Never make any mistaeks.

The caching is only going to happen for cards with minimal address decoder implementations. As far as I know there is only one card that does this.

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
Take Yahoo! Mail with you! Get it on your mobile phone.
http://mobile.yahoo.com/maildemo

Hello!

Yes, but ...

(1) it doesn't change the fact that the caching is in the vast majority of cases just wasting of RAM, even if it will happen only with a couple of cards.

(2) not all drivers dwell in the kernel.

I would prefer keeping sysfs access the ROM directly, with a little work-around disabling the sysfs file for the devices known for sharing decoders and to offer a boot-time parameter for forcing the copy in case you really need such feature for that particular device.

Have a nice fortnight

Martin `MJ' Mares   <[email protected]>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
return(EIEIO); /* Here-a-bug, There-a-bug... */

the current hardware does this.

Caching would only occur:
1) when a kernel driver for the card loads
2) and if the kernel driver asks for it

So normal hardware would never ask for the cache since it isn't needed. In the normal hardware case direct ROM access is used. If the minimalistic hardware is using a user space driver it won't ask for the cache either. The only time you get cached is on minimal hardware and when the driver asks for it. Since the driver is asking for the cache you have to assume that it needs it so the memory isn't wasted.

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
Yahoo! Mail - You care about security. So do we.
http://promotions.yahoo.com/new_mail

Hello!

No, we are speaking about sysfs access to the ROM and the driver itself is unable to predict whether anybody will ever want to use that sysfs file, so it would have to cache always.

Do I understand it correctly that the ROM-in-sysfs hack is intended only for debugging? If it is so, I do not see why we should do anything complicated in order to avoid root shooting himself in the foot.

Have a nice fortnight

Martin `MJ' Mares   <[email protected]>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
Immanuel doesn't pun, he Kant.

... for which the config space access code already sets the precedent -- there exist (rare) devices which have configuration registers with side effects on reads, making it possible to produce SCSI errors or even crash the system by just dumping the config space. Even on these devices, the kernel does not attempt to forbid reading of these registers via sysfs.

Have a nice fortnight

Martin `MJ' Mares   <[email protected]>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
A bug in the code is worth two in the documentation.

Well, this is what I initially argued with willy...

I think typical usage will be:
o dri fires off hotplug event
o userland card POSTing tool reads the ROM, saving it off for future use, and POSTs the card
o userland tool calls back into dri saying that the card is ready
o dri driver operates happily

So dealing with users accessing the rom file after a driver is up and running may not be worth the trouble.

Jesse

For the 99.999% of hardware that implements full address decoding the sysfs code will not cache the ROM contents and instead make a copy as requested from the actual ROM.

The only time the ROM will get cached is when you load a kernel device driver for a card that implements minimalistic PCI decoding (very few cards) and the driver asks for it. The driver would ask for caching since it knows that the decoder lines aren't complete.

We also don't have to cache the ROM for boot video devices since they are already cached at C000:0 by the system BIOS.

This cache is only going to impact a few systems in the world that are trying to run a current kernel on ten year old hardware. It also assumes that someone is going to rewrite the device drivers for this old hardware and ask for the caching.

Reasons for ROMs in sysfs:

Secondary video cards need to have code in their ROMs run to reset them. When an x86 PC boots it only reset the primary video device, the secondary ones won't work until their ROMs are run.

Another group needing this is laptop suspend/resume. Some cards won't come back from suspend until their ROM is run to reinitialize them.

A third group is undocumented video hardware where the only way to set the screen mode is by calling INT10 in the video ROMs. (Intel i810,830,915 for example).

Small apps are attached to the hotplug events. These apps then use vm86 or emu86 to run the ROMs. emu86 is needed for ia64 or ppc when running x86 ROMs on them.

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
Yahoo! Mail is new and improved - Check it out!
http://promotions.yahoo.com/new_mail

Hello!

Good ones, although I believe than re-initializing cards after resume should better be in the kernel.

And, even with your list of reasons, it is still very unlikely that anybody will ever need the cached copy :-) I still do not see a device which would have shared decoders AND needed such initialization in userspace.

(Also, while we are speaking about video hardware -- they either have no kernel driver, so nobody can ask for the copy, or they have one, but in that case the BIOS mode switching calls and similar things can be accomplished by the kernel driver with no need of messing with the ROM in the userspace.)

Have a nice fortnight

Martin `MJ' Mares   <[email protected]>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
How an engineer writes a program: Start by debugging an empty file...

I don't think anyone wants an x86 emulator builtin to the kernel for this purpose.

We can get away without caching a copy of the ROM in the kernel if we require userspace to cache it before the driver takes control of the card (i.e. at POST time). Otherwise, the kernel will have to take care of it.

Jesse

Well, most people probably do not want a x86 emulator running random ROMs in the userspace, either :-) But unfortunately the world is ugly (at least these parts of it).

However, point taken. (Although it will not be easy, since you have to avoid kernel drivers touching the device until you can run the ROM in userspace.)

In case of the video cards, it is probably the right path to go.

Have a nice fortnight

Martin `MJ' Mares   <[email protected]>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
Maintenance-free:  When it breaks, it can't be fixed...

Yep. :(

Yeah, it's a pain.

Great, glad we agree!

Thanks,
Jesse

You may also need ROM access when resuming a suspended device so we need to plan for that case too.

=====
Jon Smirl
[email protected]

_______________________________
Do you Yahoo!?
Express yourself with Y! Messenger! Free. Download now.
http://messenger.yahoo.com

The kernel will need it? Or userspace will need it to resume the card? If the latter, then it should be possible to have userspace cache it the first time it's read or something, to be on the safe side for video cards.

Jesse

I think this is a good idea.

It could either be done as a part of pci_enable_device(), or, which I believe would be better, as a separate function, say pci_copy_rom(char *dest), that the driver would call before pci_enable_device().

Of course, in this case the ROM wouldn't be automatically exported through sysfs, although the driver should be able to export it itself rather easily.

Vojtech Pavlik
SuSE Labs, SuSE CR

Just to name an example I know, HP Visualize EG or FXe (can't remember which one right now) PCI cards, have this sort of restriction ...

Thomas.

Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                 [ Alexander Viro on linux-kernel ]

Is the BIOS guaranteed readable in D3 if we call suspend ?

Since we only need the BIOS in some special cases we could just reject the request if the device is in use. If it has a driver - ask the driver, if it doesn't then ask sysfs.

Ok, so it's a little broken, but the idea is to expose the rom as a file in /sys/devices/pciDDDD:BB/DDDD:BB:SS:F/rom just like config space.

I think I need to use pci_config_write_byte to enable option ROM address decoding, but even then I'm taking a machine check on my test machine, so I'm obviously doing something else wrong too.

Jesse

Here's the ROM access code I've been using but it's not in the form that we need.

We do need a standard scheme for the radeon situation of having a bug in the ROM access logic. Is it ok to put the fix for this in the radeon driver? So if you read the ROM before the driver is loaded it won't be there (proabably FFFF's). After the driver loads the fix will run as part of the driver init and the ROM access functions will work right.

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
Yahoo! Mail Address AutoComplete - You start. We finish.
http://promotions.yahoo.com/new_mail

/*
* Return a copy of the VBIOS ROM
*/

int DRM(getvbios)( DRM_IOCTL_ARGS ) {
	DRM_DEVICE;
	drm_file_t *filp_priv;
	drm_get_vbios_t gb;
	struct resource *r;
	unsigned char *rom;
	int sigok;
	drm_map_t *mmio = NULL;

	DRM_DEBUG("\n");

	DRM_GET_PRIV_WITH_RETURN( filp_priv, filp );

	DRM_COPY_FROM_USER_IOCTL( gb, ( drm_get_vbios_t* )data, sizeof( gb ) );
				  
	/* Input length of zero is a request for the length */
	if (gb.length == 0) {
		DRM_DEBUG("get length\n");
		gb.length = pci_resource_len( dev->pdev, PCI_ROM_RESOURCE );
		DRM_COPY_TO_USER_IOCTL( (drm_get_vbios_t *)data, gb, sizeof(gb) );
		DRM_DEBUG("length is %lx\n", gb.length);
		return 0;
	}

	DRM_FIND_MAP( mmio, pci_resource_start( dev->pdev, 2 ) );
	if(!mmio) {
		DRM_ERROR("could not find mmio region map!\n");
		return DRM_ERR(EINVAL);
	}
              
	DRIVER_ROMBUG(mmio); /* used to fix a bug in the ATI BIOS */                    
                                                                                                        
	/* no need to search for the ROM, just ask the card where it is. */
	r = &dev->pdev->resource[PCI_ROM_RESOURCE];
	
	/* assign the ROM an address if it doesn't have one */
	if (r->parent == NULL)
		pci_assign_resource(dev->pdev, PCI_ROM_RESOURCE);
	
	/* enable if needed */
	if (!(r->flags & PCI_ROM_ADDRESS_ENABLE)) {
		u32 loc, size;
		pci_read_config_dword(dev->pdev, PCI_ROM_ADDRESS, &loc);
		pci_write_config_dword(dev->pdev, PCI_ROM_ADDRESS, ~PCI_ROM_ADDRESS_ENABLE);
		pci_read_config_dword(dev->pdev, PCI_ROM_ADDRESS, &size);
		pci_write_config_dword(dev->pdev, PCI_ROM_ADDRESS, loc);
		size = pci_size(loc, size, PCI_ROM_ADDRESS_MASK);
		DRM_DEBUG("VROM base is %08x %08x\n", loc, size );
		pci_write_config_dword(dev->pdev, PCI_ROM_ADDRESS, r->start | PCI_ROM_ADDRESS_ENABLE);
		r->flags |= PCI_ROM_ADDRESS_ENABLE;
		DRM_DEBUG("Address start/end is %08lx %08lx\n", r->start, r->end );
		pci_read_config_dword(dev->pdev, PCI_ROM_ADDRESS, &loc);
		DRM_DEBUG("VROM enable is %08x\n", loc );
	}
	
	gb.length = min( gb.length, pci_resource_len( dev->pdev, PCI_ROM_RESOURCE ));
	rom = ioremap(r->start, r->end - r->start + 1);
	if (rom) {
	
		sigok = ((readb(rom) == 0x55) && (readb(rom + 1) == 0xAA));

		if (sigok)
			DRM_COPY_TO_USER( gb.data, rom, gb.length);
		else
			DRM_DEBUG("VROM signature wrong %02x %02x\n", *(unsigned char *)rom, *((unsigned char *)rom+1) );
		
		iounmap(rom);
		
		if (r->parent) {
			release_resource(r);
			r->flags &= ~PCI_ROM_ADDRESS_ENABLE;
			r->end -= r->start;
			r->start = 0;
		}
		/* This will disable and set address to unassigned */
		pci_write_config_dword(dev->pdev, PCI_ROM_ADDRESS, 0);
		
		if (sigok) {
			DRM_COPY_TO_USER_IOCTL( (drm_get_vbios_t *)data, gb, sizeof(gb) );
			return 0;
		}
	} else 
		DRM_DEBUG("VROM failed to map\n");

	DRM_DEBUG("Using VROM copy at C000\n");
	rom = ioremap(0xC0000, r->end - r->start + 1);
	DRM_COPY_TO_USER( gb.data, rom, gb.length);
	DRM_COPY_TO_USER_IOCTL( (drm_get_vbios_t *)data, gb, sizeof(gb) );
	iounmap(rom);
	return 0;
/*	
	DRM_DEBUG("radeonfb: ROM failed to map\n");
	gb.length = 0;
	DRM_COPY_TO_USER_IOCTL( (drm_get_vbios_t *)data, gb, sizeof(gb) );
	return -1;

*/
}

IMO the fix should go to the PCI quirk logic. That's a place to work around PCI config bugs.

Vojtech Pavlik
SuSE Labs, SuSE CR

I looked at PCI quirks, they all seem to be fixing chipset issues. Do we want to start including adapter specific quirks along with the more general chipset one?

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
New and Improved Yahoo! Mail - 100MB free storage!
http://promotions.yahoo.com/new_mail

It's mostly chipsets, but not just chipsets - take a look at the S3 entries.

Vojtech Pavlik
SuSE Labs, SuSE CR

I think Martin's suggestion of just caching them all at probe time (or optionally at driver attach time) is probably the simplest and easiest to get right. It could be controllable via a boot time parameter. But I'm not entirely opposed to using pci quirks.

Jesse

You guys just have *too* much RAM.