add PCI ROMs to sysfs

Per a big thread earlier today, this patch adds a 'rom' file to sysfs in the same place where we currently have 'config', 'irq', etc.

Note that the ROM is *not* cached, so userspace will have to save the ROM when a device is first detected if the card doesn't support address cycles to ROM space at the same time as other memory accesses. (I'm still open to changing this if people feel strongly, but as it stands, the 'rom' file is only slightly more dangerous than the 'config' file.)

Jon, am I missing something or is it possible for us to cache the ROM in userspace when it receives the hotplug event? I saw your DRM code, and for the case of ROMs at a nonstandard address, we can fixup the address for pci_dev->resource[PCI_ROM_RESOURCE] in pci-quirks, can't we?

Thoughts? I've tried to add cleanup code, but I'm not sure how acceptable it is and I don't have any way of testing it.

Thanks,
Jesse

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

@@ -164,6 +164,86 @@
 	return count;
 }
 
+/**
+ * pci_enable_rom - enable ROM decoding for a PCI device
+ * @dev: PCI device to enable
+ *
+ * Enable ROM decoding on @dev.  This involves simply turning on the last
+ * bit of the PCI ROM BAR.  Note that some cards may share address decoders
+ * between the ROM and other resources, so enabling it may disable access
+ * to MMIO registers or other card memory.
+ */
+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);
+}
+
+/**
+ * pci_disable_rom - disable ROM decoding for a PCI device
+ * @dev: PCI device to disable
+ *
+ * Disable ROM decoding on a PCI device by turning off the last bit in the
+ * ROM BAR.
+ */
+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);
+}
+
+/**
+ * pci_read_rom - read a PCI ROM
+ * @kobj: kernel object handle
+ * @buf: where to put the data we read from the ROM
+ * @off: file offset
+ * @count: number of bytes to read
+ *
+ * Put @count bytes starting at @off into @buf from the ROM in the PCI
+ * device corresponding to @kobj.
+ */
+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",

@@ -193,6 +273,39 @@
 	else
 		sysfs_create_bin_file(&pdev->dev.kobj, &pcie_config_attr);
 
+	/* If the device has a ROM, try to expose it in sysfs. */
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE)) {
+		struct bin_attribute *rom_attr;
+		rom_attr = kmalloc(sizeof(*rom_attr), GFP_ATOMIC);
+
+		if (!rom_attr)
+			goto out;
+
+		pdev->rom_attr = rom_attr;
+		rom_attr->attr.name = "rom";
+		rom_attr->attr.mode = S_IRUSR;
+		rom_attr->attr.owner = THIS_MODULE;
+		rom_attr->read = pci_read_rom;
+		rom_attr->size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+		sysfs_create_bin_file(&pdev->dev.kobj, rom_attr);
+	}
+ out:
 	/* add platform-specific attributes */
 	pcibios_add_platform_entries(pdev);
+}
+
+/**
+ * pci_remove_sysfs_dev_files - cleanup PCI specific sysfs files
+ * @pdev: device whose entries we should free
+ *
+ * Cleanup when @pdev is removed from sysfs.
+ */
+void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
+{
+	/* Don't need to free config entries since they're static & global */
+
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE) && pdev->rom_attr) {
+		sysfs_remove_bin_file(&pdev->dev.kobj, pdev->rom_attr);
+		kfree(pdev->rom_attr);
+	}
 }

===== drivers/pci/remove.c 1.3 vs edited =====

@@ -26,6 +26,7 @@
 static void pci_destroy_dev(struct pci_dev *dev)
 {
 	pci_proc_detach_device(dev);
+	pci_remove_sysfs_dev_files(dev);
 	device_unregister(&dev->dev);
 
 	/* Remove the device from the device lists, and prevent any further

===== include/linux/pci.h 1.130 vs edited =====

@@ -537,6 +537,7 @@
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	
 	unsigned int 	saved_config_space[16]; /* config space saved at suspend time */
+	struct bin_attribute *rom_attr; /* ROM attribute (if ROM is present) */
 #ifdef CONFIG_PCI_NAMES
 #define PCI_NAME_SIZE	96
 #define PCI_NAME_HALF	__stringify(43)	/* less than half to handle slop */

You don't have access to a cardbus machine? Or how about using the fakephp driver to "remove" pci devices? You should be able to test this code path with either one of those methods...

I've never tried fakephp, I'll look into it. And I do have a laptop at home that I could mess around on too, though I don't think I have any devices with ROMs...

What will happen if we don't? They didn't used to be removed, and there's only one global copy afaict. I'll add code to kill them at any rate.

Thanks,
Jesse

Ok, I tested it with fakephp and things seem to work ok (no panics, all files are removed).

Does it look ok otherwise?

Thanks,
Jesse

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

@@ -164,6 +164,86 @@
 	return count;
 }
 
+/**
+ * pci_enable_rom - enable ROM decoding for a PCI device
+ * @dev: PCI device to enable
+ *
+ * Enable ROM decoding on @dev.  This involves simply turning on the last
+ * bit of the PCI ROM BAR.  Note that some cards may share address decoders
+ * between the ROM and other resources, so enabling it may disable access
+ * to MMIO registers or other card memory.
+ */
+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);
+}
+
+/**
+ * pci_disable_rom - disable ROM decoding for a PCI device
+ * @dev: PCI device to disable
+ *
+ * Disable ROM decoding on a PCI device by turning off the last bit in the
+ * ROM BAR.
+ */
+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);
+}
+
+/**
+ * pci_read_rom - read a PCI ROM
+ * @kobj: kernel object handle
+ * @buf: where to put the data we read from the ROM
+ * @off: file offset
+ * @count: number of bytes to read
+ *
+ * Put @count bytes starting at @off into @buf from the ROM in the PCI
+ * device corresponding to @kobj.
+ */
+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",

@@ -193,6 +273,43 @@
 	else
 		sysfs_create_bin_file(&pdev->dev.kobj, &pcie_config_attr);
 
+	/* If the device has a ROM, try to expose it in sysfs. */
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE)) {
+		struct bin_attribute *rom_attr;
+		rom_attr = kmalloc(sizeof(*rom_attr), GFP_ATOMIC);
+
+		pdev->rom_attr = NULL;
+		if (!rom_attr)
+			goto out;
+
+		pdev->rom_attr = rom_attr;
+		rom_attr->attr.name = "rom";
+		rom_attr->attr.mode = S_IRUSR;
+		rom_attr->attr.owner = THIS_MODULE;
+		rom_attr->read = pci_read_rom;
+		rom_attr->size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+		sysfs_create_bin_file(&pdev->dev.kobj, rom_attr);
+	}
+ out:
 	/* add platform-specific attributes */
 	pcibios_add_platform_entries(pdev);
+}
+
+/**
+ * pci_remove_sysfs_dev_files - cleanup PCI specific sysfs files
+ * @pdev: device whose entries we should free
+ *
+ * Cleanup when @pdev is removed from sysfs.
+ */
+void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
+{
+	if (pdev->cfg_size < 4096)
+		sysfs_remove_bin_file(&pdev->dev.kobj, &pci_config_attr);
+	else
+		sysfs_remove_bin_file(&pdev->dev.kobj, &pcie_config_attr);
+
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE) && pdev->rom_attr) {
+		sysfs_remove_bin_file(&pdev->dev.kobj, pdev->rom_attr);
+		kfree(pdev->rom_attr);
+	}
 }

===== drivers/pci/remove.c 1.3 vs edited =====

@@ -26,6 +26,7 @@
 static void pci_destroy_dev(struct pci_dev *dev)
 {
 	pci_proc_detach_device(dev);
+	pci_remove_sysfs_dev_files(dev);
 	device_unregister(&dev->dev);
 
 	/* Remove the device from the device lists, and prevent any further

===== include/linux/pci.h 1.130 vs edited =====

@@ -537,6 +537,7 @@
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	
 	unsigned int 	saved_config_space[16]; /* config space saved at suspend time */
+	struct bin_attribute *rom_attr; /* ROM attribute (if ROM is present) */
 #ifdef CONFIG_PCI_NAMES
 #define PCI_NAME_SIZE	96
 #define PCI_NAME_HALF	__stringify(43)	/* less than half to handle slop */

code,

When I was done with the ROM I did this:

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

Then when I accessed them I did this:

/* assign the ROM an address if it doesn't have one */ if (r->parent == NULL)

	pci_assign_resource(dev->pdev, PCI_ROM_RESOURCE);

I believe I needed to do this because Xfree was enabling the ROMs without telling the kernel. This caused the kernel resource struct to get out of sync with the actual state of the ROM.

If you set pci_dev->resource[PCI_ROM_RESOURCE] to C000:0 won't this mess up pci_assign_resource()/release_resource()?

I wrote this code a while ago so I'm forgetting exactly why I did things. There is a variation on this code in drivers/video/aty/radeon_base.c

=====
Jon Smirl
[email protected]

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

Yeah, you're right, that wouldn't be a good thing to do. I guess I'll have to hang a different structure off of the pci_dev so we can tell the sysfs rom handling code where to get the rom. Doing it that way would allow us to deal with cards that really need a copy made too, though the default behavior would be to read it directly.

How does that sound?

Jesse

Here's a new patch that implements that suggestion, though without any special cases for the various cards that might need the ROM copy (e.g. those with shared decoders or whose ROMs are in the system ROM somewhere). How does it look, Greg? It it suitable for the mainline yet? I expect those familiar with the various cards to add the necessary quirks code as needed.

Thanks,
Jesse

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

@@ -164,6 +164,92 @@
 	return count;
 }
 
+/**
+ * pci_enable_rom - enable ROM decoding for a PCI device
+ * @dev: PCI device to enable
+ *
+ * Enable ROM decoding on @dev.  This involves simply turning on the last
+ * bit of the PCI ROM BAR.  Note that some cards may share address decoders
+ * between the ROM and other resources, so enabling it may disable access
+ * to MMIO registers or other card memory.
+ */
+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);
+}
+
+/**
+ * pci_disable_rom - disable ROM decoding for a PCI device
+ * @dev: PCI device to disable
+ *
+ * Disable ROM decoding on a PCI device by turning off the last bit in the
+ * ROM BAR.
+ */
+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);
+}
+
+/**
+ * pci_read_rom - read a PCI ROM
+ * @kobj: kernel object handle
+ * @buf: where to put the data we read from the ROM
+ * @off: file offset
+ * @count: number of bytes to read
+ *
+ * Put @count bytes starting at @off into @buf from the ROM in the PCI
+ * device corresponding to @kobj.
+ */
+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);
+	char direct_access = dev->rom_info.rom ? 0 : 1;
+
+	if (off > size)
+		return 0;
+	if (off + count > size) {
+		size -= off;
+		count = size;
+	} else {
+		size = count;
+	}
+
+	/* Enable ROM space decodes and do the reads */
+	if (direct_access)
+		pci_enable_rom(dev);
+
+	while (size > 0) {
+		unsigned char val;
+		if (direct_access)
+			val = readb(start + off);
+		else
+			val = *(dev->rom_info.rom + off);
+		buf[off - init_off] = val;
+		off++;
+		--size;
+	}
+
+	/* Disable again before continuing */
+	if (direct_access)
+		pci_disable_rom(dev);
+
+	return count;
+}
+
 static struct bin_attribute pci_config_attr = {
 	.attr =	{
 		.name = "config",

@@ -186,13 +272,50 @@
 	.write = pci_write_config,
 };
 
-void pci_create_sysfs_dev_files (struct pci_dev *pdev)
+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, try to expose it in sysfs. */
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE)) {
+		struct bin_attribute *rom_attr;
+		rom_attr = kmalloc(sizeof(*rom_attr), GFP_ATOMIC);
+
+		pdev->rom_info.rom_attr = NULL;
+		if (!rom_attr)
+			goto out;
+
+		pdev->rom_info.rom_attr = rom_attr;
+		rom_attr->attr.name = "rom";
+		rom_attr->attr.mode = S_IRUSR;
+		rom_attr->attr.owner = THIS_MODULE;
+		rom_attr->read = pci_read_rom;
+		rom_attr->size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+		sysfs_create_bin_file(&pdev->dev.kobj, rom_attr);
+	}
+ out:
 	/* add platform-specific attributes */
 	pcibios_add_platform_entries(pdev);
+}
+
+/**
+ * pci_remove_sysfs_dev_files - cleanup PCI specific sysfs files
+ * @pdev: device whose entries we should free
+ *
+ * Cleanup when @pdev is removed from sysfs.
+ */
+void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
+{
+	if (pdev->cfg_size < 4096)
+		sysfs_remove_bin_file(&pdev->dev.kobj, &pci_config_attr);
+	else
+		sysfs_remove_bin_file(&pdev->dev.kobj, &pcie_config_attr);
+
+	if (pdev->rom_info.rom_attr) {
+		sysfs_remove_bin_file(&pdev->dev.kobj, pdev->rom_info.rom_attr);
+		kfree(pdev->rom_info.rom_attr);
+	}
 }

===== drivers/pci/pci.h 1.12 vs edited =====

@@ -3,6 +3,7 @@
 extern int pci_hotplug (struct device *dev, char **envp, int num_envp,
 			 char *buffer, int buffer_size);
 extern void pci_create_sysfs_dev_files(struct pci_dev *pdev);
+extern void pci_remove_sysfs_dev_files(struct pci_dev *pdev);
 extern int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
 				  unsigned long size, unsigned long align,
 				  unsigned long min, unsigned int type_mask,

===== drivers/pci/probe.c 1.65 vs edited =====

@@ -157,6 +157,8 @@
 #endif
 		}
 	}
+
+	dev->rom_info.rom = NULL;
 	if (rom) {
 		dev->rom_base_reg = rom;
 		res = &dev->resource[PCI_ROM_RESOURCE];

===== drivers/pci/remove.c 1.3 vs edited =====

@@ -26,6 +26,9 @@
 static void pci_destroy_dev(struct pci_dev *dev)
 {
 	pci_proc_detach_device(dev);
+	/* Free the copy of the ROM if we made one */
+	kfree(dev->rom_info.rom);
+	pci_remove_sysfs_dev_files(dev);
 	device_unregister(&dev->dev);
 
 	/* Remove the device from the device lists, and prevent any further

===== include/linux/pci.h 1.130 vs edited =====

@@ -471,6 +471,11 @@
 	pci_mmap_mem
 };
 
+struct rom_info {
+	char *rom; /* copy of the ROM if necessary */
+	struct bin_attribute *rom_attr;
+};
+
 /* This defines the direction arg to the DMA mapping routines. */
 #define PCI_DMA_BIDIRECTIONAL	0
 #define PCI_DMA_TODEVICE	1

@@ -537,6 +542,7 @@
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	
 	unsigned int 	saved_config_space[16]; /* config space saved at suspend time */
+	struct rom_info rom_info; /* How and where to get the ROM info for this device */
 #ifdef CONFIG_PCI_NAMES
 #define PCI_NAME_SIZE	96
 #define PCI_NAME_HALF	__stringify(43)	/* less than half to handle slop */

Radeons need this quirk for broken VBIOS's that leave the ROM decoding disabled.

   unsigned int temp;
   temp = DRM_READ32(mmio, RADEON_MPP_TB_CONFIG);
   temp &= 0x00ffffffu;
   temp |= 0x04 << 24;
   DRM_WRITE32(mmio, RADEON_MPP_TB_CONFIG, temp);
   temp = DRM_READ32(mmio, RADEON_MPP_TB_CONFIG);

Shouldn't this go into the radeon driver so that it will work with hotplug? Or should it go in both places, a _devinit pci_quirk and the radeon driver?

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
New and Improved Yahoo! Mail - Send 10MB messages!
http://promotions.yahoo.com/new_mail

Is tracking the boot video device and redirecting to C000:0 going to be a quirk, architecture specific, or what? Where does this little piece of code need to go?

=====
Jon Smirl
[email protected]

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

I think that would be a quirk. You'd copy ROMs like that into the rom pointer in the pci_dev structure I guess.

Jesse

Providing that quirk is scanned at hotplug time it can go into the quirk code which is probably simplest. It may need a tiny hook in sysfs to map the right object but that isnt hard.

What guarantees the ROM already has an assigned PCI address ?

Presumably the PCI core. If that's a bad assumption, then clearly this code won't work as is and will need additional checks/setup code.

Jesse

My original version have this in it:

/* assign the ROM an address if it doesn't have one */ if (r->parent == NULL)
pci_assign_resource(dev->pdev, PCI_ROM_RESOURCE);

if (r->parent) {
    release_resource(r);
    r->flags &= ~PCI_ROM_ADDRESS_ENABLE;
    r->end -= r->start;
    r->start = 0;
}

I was running this code on both 2.4/2.6 but I may have needed to do this for 2.4. Is it consistent to have pci_assign_resource() and then use release_resource()?

My i875P AGP controller has a ROM on it as well as my two video cards.

=====
Jon Smirl
[email protected]

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

So it seems there's some redundancy here--some platforms will map the ROMs at discovery time while on others we have to map them explicitly? Which should we count on? I was assuming the former in the last patch I posted, and would like to keep it that way unless there's some reason not to.

Thanks,
Jesse

This is saying that my AGP bridge chip has a ROM right?

00:01.0 PCI bridge: Intel Corp. 82875P Processor to AGP Controller (rev
02) (prog-if 00 [Normal decode])
Flags: bus master, 66Mhz, fast devsel, latency 64
Bus: primary=00, secondary=01, subordinate=01, sec-latency=64 I/O behind bridge: 0000d000-0000dfff
Memory behind bridge: fe900000-feafffff
Prefetchable memory behind bridge: f0000000-f7ffffff Expansion ROM at 0000d000 [disabled] [size=4K]

Each of my video controllers has one too:

01:00.0 VGA compatible controller: ATI Technologies Inc Radeon RV250 If [Radeon 9000] (rev 01) (prog-if 00 [VGA])
Subsystem: C.P. Technology Co. Ltd RV250 If [Radeon 9000 Pro
"Evil Commando"]
Flags: stepping, 66Mhz, medium devsel, IRQ 177 Memory at f4000000 (32-bit, prefetchable) [disabled]
[size=fea00000]
I/O ports at de00 [disabled] [size=256]
Memory at fe9e0000 (32-bit, non-prefetchable) [disabled]
[size=64K]
Expansion ROM at 00020000 [disabled]
Capabilities: <available only to root>

02:02.0 VGA compatible controller: ATI Technologies Inc Rage 128 PD/PRO TMDS (prog-if 00 [VGA])

        Subsystem: ATI Technologies Inc Rage 128 AIW
        Flags: bus master, stepping, medium devsel, latency 64, IRQ 209
        Memory at f8000000 (32-bit, prefetchable) [size=fe800000]
        I/O ports at ce00 [size=256]
        Memory at fe7dc000 (32-bit, non-prefetchable) [size=16K]
        Expansion ROM at 00020000 [disabled]
        Capabilities: <available only to root>

Both of the video ROMs are at 00020000, won't they end up on top of each other when enabled?

With the patch the video ROMs are in sysfs but the AGP bridge one is not.

=====
Jon Smirl
[email protected]

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

Yeah, it doesn't look like they've been properly assigned addresses. But then I've also seen lspci lie, you can check /sys/devices/.../config for the actual resource values. If they're sane then things are more likely to work.

Jesse

Oops, I mean /sys/devices/.../resource. E.g.

jbarnes@mill:~$ cat /sys/devices/pci0002\:06/0002\:06\:0f.0/resource
0x00000000f5200000 0x00000000f53fffff 0x0000000000000200
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x00000000f5100000 0x00000000f51fffff 0x0000000000007200

Jesse

... or try `lspci -b', it will dump the actual registers, not the kernel's view.

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 jury consists of 12 persons chosen to decide who has the better lawyer.

lspci -b would be great but it doesn't seem to have implemented dumping the ROM location. It dumps everything else.

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
New and Improved Yahoo! Mail - Send 10MB messages!
http://promotions.yahoo.com/new_mail

Ah well, I see the problem -- it doesn't display disabled ROMs.

The following quick hack should cure it:

@@ -160,7 +160,9 @@
       if (reg)
 	{
 	  u32 a = pci_read_long(d, reg);
+#if 0
 	  if (a & PCI_ROM_ADDRESS_ENABLE)
+#endif
 	    d->rom_base_addr = a;
 	}
     }

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 student who changes the course of history is probably taking an exam.

lspci shows my AGP bridge

00:01.0 PCI bridge: Intel Corp. 82875P Processor to AGP Controller (rev
02) (prog-if 00 [Normal decode])
Flags: bus master, 66Mhz, fast devsel, latency 64
Bus: primary=00, secondary=01, subordinate=01, sec-latency=64 I/O behind bridge: 0000d000-0000dfff
Memory behind bridge: fe900000-feafffff
Prefetchable memory behind bridge: f0000000-f7ffffff Expansion ROM at 0000d000 [disabled] [size=4K]

It shows an expansion ROM at d000. d000 is a normal address for a ROM. But d000 also shows as I/O behind bridge.

If I list sysfs resources, the ROM is in slot 7, not 6 which is normal for a ROM. Is this really a ROM or a bug in lspci?

[jonsmirl@smirl 0000:00:01.0]$ cat resource

0  0x0000000000000000 0x0000000000000000 0x0000000000000000
1  0x0000000000000000 0x0000000000000000 0x0000000000000000
2  0x0000000000000000 0x0000000000000000 0x0000000000000000
3  0x0000000000000000 0x0000000000000000 0x0000000000000000
4  0x0000000000000000 0x0000000000000000 0x0000000000000000
5  0x0000000000000000 0x0000000000000000 0x0000000000000000
6  0x0000000000000000 0x0000000000000000 0x0000000000000000
7  0x000000000000d000 0x000000000000dfff 0x0000000000000100
8  0x00000000fe900000 0x00000000feafffff 0x0000000000000200
9  0x00000000f0000000 0x00000000f7ffffff 0x0000000000001200

10 0x0000000000000000 0x0000000000000000 0x0000000000000000 11 0x0000000000000000 0x0000000000000000 0x0000000000000000 [jonsmirl@smirl 0000:00:01.0]$

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
Read only the mail you want - Yahoo! Mail SpamGuard.
http://promotions.yahoo.com/new_mail

Was it with the `-b' option or not?

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
"#define QUESTION ((bb) || !(bb))"  -- Shakespeare

Version 10

implements an x86 quirk to record the boot video device. Is the PCI_ROM_SHADOW flag a safe define? Quirk records boot video device by looking at how the bridges route to the VGA device. It there some other way to tell which video card is the boot one? What if there is more than one VGA card on the PCI bus? I think the BIOS spec is to enable the one in the lowest slot number. Can someone who own multiple PCI video cards test this? I tested with one PCI, one AGP.

BenH, this should solve the problem of which video card owns the ROM copy at C000:0. For the boot device this code returns the shadow copy, else the real ROM on the card.

I did the x86 quirk, what do the quirks on ia64, ppc, x86_64 need? Can they just copy the x86 one?

=====
Jon Smirl
[email protected]

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

===== arch/i386/pci/fixup.c 1.19 vs edited =====

@@ -237,6 +237,29 @@
 	}
 }
 
+static void __devinit pci_fixup_video(struct pci_dev *pdev)
+{
+	struct pci_dev *bridge;
+	struct pci_bus *bus;
+	u16 l;
+	
+	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+		return;
+	       
+	/* Is VGA routed to us? */
+	bus = pdev->bus;
+	while (bus) {
+		bridge = bus->self;
+		if (bridge) {
+			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &l);
+			if (!(l & PCI_BRIDGE_CTL_VGA))
+				return;
+		}
+		bus = bus->parent;
+	}
+	pdev->resource[PCI_ROM_RESOURCE].flags |= PCI_ROM_SHADOW;
+}
+
 struct pci_fixup pcibios_fixups[] = {
 	{
 		.pass		= PCI_FIXUP_HEADER,

@@ -345,6 +368,12 @@
 		.vendor		= PCI_VENDOR_ID_NVIDIA,
 		.device		= PCI_DEVICE_ID_NVIDIA_NFORCE2,
 		.hook		= pci_fixup_nforce2
+	},
+	{
+		.pass		= PCI_FIXUP_FINAL,
+		.vendor		= PCI_ANY_ID,
+		.device		= PCI_ANY_ID,
+		.hook		= pci_fixup_video
 	},
 	{ .pass = 0 }
 };

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

===== drivers/pci/pci.h 1.12 vs edited =====

@@ -3,6 +3,7 @@
 extern int pci_hotplug (struct device *dev, char **envp, int num_envp,
 			 char *buffer, int buffer_size);
 extern void pci_create_sysfs_dev_files(struct pci_dev *pdev);
+extern void pci_remove_sysfs_dev_files(struct pci_dev *pdev);
 extern int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
 				  unsigned long size, unsigned long align,
 				  unsigned long min, unsigned int type_mask,

===== drivers/pci/probe.c 1.65 vs edited =====

@@ -157,6 +157,7 @@
 #endif
 		}
 	}
+
 	if (rom) {
 		dev->rom_base_reg = rom;
 		res = &dev->resource[PCI_ROM_RESOURCE];

===== drivers/pci/remove.c 1.3 vs edited =====

@@ -26,6 +26,7 @@
 static void pci_destroy_dev(struct pci_dev *dev)
 {
 	pci_proc_detach_device(dev);
+	pci_remove_sysfs_dev_files(dev);
 	device_unregister(&dev->dev);
 
 	/* Remove the device from the device lists, and prevent any further

===== include/linux/pci.h 1.132 vs edited =====

@@ -102,6 +102,8 @@
 #define PCI_SUBSYSTEM_ID	0x2e  
 #define PCI_ROM_ADDRESS		0x30	/* Bits 31..11 are address, 10..1 reserved */
 #define  PCI_ROM_ADDRESS_ENABLE	0x01
+#define  PCI_ROM_SHADOW		0x02	/* resource flag, ROM is copy at C000:0 */
+#define  PCI_ROM_COPY		0x04	/* resource flag, ROM is alloc'd copy */
 #define PCI_ROM_ADDRESS_MASK	(~0x7ffUL)
 
 #define PCI_CAPABILITY_LIST	0x34	/* Offset of first capability list entry */

Looks ok for me. It would be nice though if the code returning the shadow copy could be "hooked" by the driver, that way, the radeon kernel driver can force-enable the ROM decoding... or do you want to use pci quirks for that too ?

Probably... can't we have arch-independant quirks ? Especially with the new quirk section stuff David just posted, we can have quirks pretty much anywhere...

Ben.

My current plan is for the radeon driver to keep the code for enabling the ROM. Otherwise we have to build all of the radeon PCI IDs somewhere else into the kernel. In my system if you dump if without the driver loaded it is FFFF, load the driver and you see it. First thing the driver does is fix the ROM so that it is visible to the hotplug event.

But isn't this architecture specific? The Mac doesn't shadow ROMs at 0xC0000 does it? How does it shadow ROMs?

I may change the API a little today and expose a map/unmap ROM function to make things easier for the driver to get the right copy. Adding those calls will let me add a pci_map_rom_copy, which the driver can use to trigger the copy for ROMs that aren't fully decoded.

=====
Jon Smirl
[email protected]

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

For some reason this version doesn't work on ia64. It just returns bytes containing 0 when I try to dump the ROM.

Jesse

pci_assign_resource is mucking with the values in pci_dev->resource[PCI_ROM_RESOURCE]. If I remove the call to pci_assign_resource, things work for me. Is that call really necessary? Don't we just need ioremap?

Thanks,
Jesse

/* assign the ROM an address if it doesn't have one */ if (res->parent == NULL)

	pci_assign_resource(dev, PCI_ROM_RESOURCE);

It is protected by the (res->parent == NULL). Looking at the code in kernel/resource.c this is the correct check to see if the resource does not have an address assigned. If (res->parent != NULL) then it is supposed to muck with the addresses.

If you follow the code path of pci_assign_resource() it will program the ROM to appear at the newly assigned address in pci_update_resource(). I'd check these code paths and see if they are 64 bit broken. This process does work on ia32.

If you can read the ROM without a resource assigned it is just luck that everything is still in the same place as boot. If you start hotpluging the original ROM address could get used by another card since is is not actively assigned.

I didn't check the error code from pci_assign_resource(). If it can't match the PREFETCH type it will fail. That may be what is happening. I'll add a check.

=====
Jon Smirl
[email protected]

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

I think just my platform is broken (i.e. pci_assign_resource doesn't do the right thing on sn2). I'll take a look.

Right.

Thanks,
Jesse

Version 11

I removed the call to pci_alloc_resource(). The sysfs attribute code builds the attributes before the pci subsystem is fully initialized. Specifically before arch pcibios_init() has been called. If pci_alloc_resource() is called for the ROM before pcibios_init() the kernel's resource maps have not been built yet. This will result in the ROM being located on top of the framebuffer; as soon as it is enabled the system will lock. Right now the code relies on the BIOS getting things set up right. If we can figure out how to initialize the sysfs attributes after pcibios_init() then I can put the alloc call back.

Changes the API to make it useful from a device driver:

pci_map_rom - map the rom and provide virtual address, transparently return shadow/copy if needed. Normal drivers call this

pci_map_rom_copy - same as pci_map_rom except the ROM will copied, future reads of ROM will use copy. Hardware with minimal decoding calls this

pci_unmap_rom - release the ioremap if there is one

If the ROM code finds the 55 AA signature at the start of the ROM it will use the len/512 from the third byte for the length. The PCI window is often much larger than the ROM really is so this saves a lot of memory for copied ROMs.

I added two new defines in pci.h
#define PCI_ROM_SHADOW 0x02 /* resource flag, ROM is copy at C000:0 */
#define PCI_ROM_COPY 0x04 /* resource flag, ROM is alloc'd copy */
I don't think these conflict with anything, please check to make sure.

Please check the code out and give it some testing. It will probably needs some adjustment for other platforms.

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
Read only the mail you want - Yahoo! Mail SpamGuard.
http://promotions.yahoo.com/new_mail

===== arch/i386/pci/fixup.c 1.19 vs edited =====

@@ -237,6 +237,29 @@
 	}
 }
 
+static void __devinit pci_fixup_video(struct pci_dev *pdev)
+{
+	struct pci_dev *bridge;
+	struct pci_bus *bus;
+	u16 l;
+	
+	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+		return;
+	       
+	/* Is VGA routed to us? */
+	bus = pdev->bus;
+	while (bus) {
+		bridge = bus->self;
+		if (bridge) {
+			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &l);
+			if (!(l & PCI_BRIDGE_CTL_VGA))
+				return;
+		}
+		bus = bus->parent;
+	}
+	pdev->resource[PCI_ROM_RESOURCE].flags |= PCI_ROM_SHADOW;
+}
+
 struct pci_fixup pcibios_fixups[] = {
 	{
 		.pass		= PCI_FIXUP_HEADER,

@@ -345,6 +368,12 @@
 		.vendor		= PCI_VENDOR_ID_NVIDIA,
 		.device		= PCI_DEVICE_ID_NVIDIA_NFORCE2,
 		.hook		= pci_fixup_nforce2
+	},
+	{
+		.pass		= PCI_FIXUP_FINAL,
+		.vendor		= PCI_ANY_ID,
+		.device		= PCI_ANY_ID,
+		.hook		= pci_fixup_video
 	},
 	{ .pass = 0 }
 };

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

===== drivers/pci/pci.h 1.12 vs edited =====

@@ -3,6 +3,7 @@
 extern int pci_hotplug (struct device *dev, char **envp, int num_envp,
 			 char *buffer, int buffer_size);
 extern void pci_create_sysfs_dev_files(struct pci_dev *pdev);
+extern void pci_remove_sysfs_dev_files(struct pci_dev *pdev);
 extern int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
 				  unsigned long size, unsigned long align,
 				  unsigned long min, unsigned int type_mask,

===== drivers/pci/remove.c 1.3 vs edited =====

@@ -12,12 +12,22 @@
 
 static void pci_free_resources(struct pci_dev *dev)
 {
+	struct resource *res;
 	int i;
 
  	msi_remove_pci_irq_vectors(dev);
 
+	if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
+		res = &dev->resource[PCI_ROM_RESOURCE];
+		if (res->flags & PCI_ROM_COPY) {
+			kfree((void*)res->start);
+			res->flags &= !PCI_ROM_COPY;
+			res->start = 0;
+			res->end = 0;
+		}
+	}
 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-		struct resource *res = dev->resource + i;
+		res = dev->resource + i;
 		if (res->parent)
 			release_resource(res);
 	}

@@ -26,6 +36,7 @@
 static void pci_destroy_dev(struct pci_dev *dev)
 {
 	pci_proc_detach_device(dev);
+	pci_remove_sysfs_dev_files(dev);
 	device_unregister(&dev->dev);
 
 	/* Remove the device from the device lists, and prevent any further

===== include/linux/pci.h 1.132 vs edited =====

@@ -102,6 +102,8 @@
 #define PCI_SUBSYSTEM_ID	0x2e  
 #define PCI_ROM_ADDRESS		0x30	/* Bits 31..11 are address, 10..1 reserved */
 #define  PCI_ROM_ADDRESS_ENABLE	0x01
+#define  PCI_ROM_SHADOW		0x02	/* resource flag, ROM is copy at C000:0 */
+#define  PCI_ROM_COPY		0x04	/* resource flag, ROM is alloc'd copy */
 #define PCI_ROM_ADDRESS_MASK	(~0x7ffUL)
 
 #define PCI_CAPABILITY_LIST	0x34	/* Offset of first capability list entry */

@@ -777,6 +779,9 @@
 int pci_dac_set_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_assign_resource(struct pci_dev *dev, int i);
+unsigned char *pci_map_rom(struct pci_dev *dev, size_t *size);
+unsigned char *pci_map_rom_copy(struct pci_dev *dev, size_t *size);
+void pci_unmap_rom(struct pci_dev *dev, unsigned char *rom);
 
 /* Power management related routines */
 int pci_save_state(struct pci_dev *dev, u32 *buffer);

I've fixed (well, hacked) the sn2 code such that your original check for res->parent will work correctly for me too, so it sounds like we're covered there.

Thanks, will do (though probably not until next week, I'm about to head out for the weekend).

Thanks,
Jesse

Jon, this works on my machine too. Greg, if it looks ok can you pull it in? And can you add:

* (C) Copyright 2004 Silicon Graphics, Inc.
* Jesse Barnes <[email protected]>

to pci-sysfs.c if you do?

Greg was a little worried that your comment

	/* .size is set individually for each device, sysfs copies it into dentry */

might not be correct. If not, you can reuse the code from my pci-sysfs-rom-7.patch if you want, otherwise it's probably fine aside from a bunch of trailing whitespace in the file. I added the following to my .emacs to make it obvious so that I could kill it when I saw it:
(defun linux-c-mode ()
"C mode with Linux kernel defaults"
(interactive)
(c-mode)
(c-set-style "K&R")
(setq c-basic-offset 8)
(setq indent-tabs-mode t)
(setq show-trailing-whitespace t))

Thanks,
Jesse

Care to send me a new patch? Oh, and that copyright line needs to look like:
* Copyright (c) 2004 Silicon Graphics, Inc. Jesse Barnes <[email protected]>

to make it legal, or so my lawyers say :)

I looked at the code, and he's right. But it's pretty scary that it works correctly so I'd prefer to do it the way your patch did it (create a new attribute for every entry.)

thnaks,

greg k-h

But I'm not the copyright holder, Silicon Graphics is, I just wanted people to know who to harass if something breaks :).

Ok. Jon?

Thanks,
Jesse

That's fine. It's the "Copyright (c) 2004" order and exact "(c)" that really matters, from what I have been told to do.

thanks,

greg k-h

Is that true now? It used to be said (including by some people who I trust on such matters) that the ascii graphic "(c)" -- as opposed to a real c-in-a-circle character -- was _not_ officially recognized as meaning "Copyright", and so one needed the actual word "Copyright", or a real c-in-a-circle, for such notices to have some meaning. However the word "Copyright" alone is apparently enough.

-Miles

Somebody has to do something, and it's just incredibly pathetic that it
has to be us.  -- Jerry Garcia

That's what the copyright lawyers at a certian three letter company said is needed, and for some reason I tend to believe what they say :)

thanks,

greg k-h

You mean the copyright lawyers at a certian three letter company in a certain three letter country?

IIRC, according to copyright laws in the other part of the world, everything you write is always copyrighted by you, whether ©' or (c)' or `copyright' are present or not.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

On the other hand, you could believe what the US government tells you ...
http://www.copyright.gov/circs/circ1.html#noc

In particular, the (c) has no relevance; the important bit is the word "Copyright":

1. The symbol © (the letter C in a circle), or the word "Copyright," or the abbreviation "Copr."; and

"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

Greg,

That made me curious, what is the rationale behind

(C) Copyright 2004 Silicon Graphics, Inc. Jesse Barnes <[email protected]>

works and the previous suggested

(C) Copyright 2004 Silicon Graphics, Inc.
Jesse Barnes <[email protected]>

doesnt?

I know its a bit offtopic, but still, if you know the reason, would be great to hear :) Bet others will also like to hear that.

Thanks!

My thought was that the second one doesn't list me as the copyright holder, only the author, which is just what I wanted. To me, the first one seems to indicate that the copyright is shared between myself and the entity that owns it, which is incorrect.

Jesse

Ok, this is as per some copyright lawyers who know US Copyright law, and how to defend it in court. To make it legally binding in the US, so you can file for a copyright in the proper government agencies (which is necessary if you want to be able defend your copyright in court in an easier manner, it's not necessary but it sure helps.) you need the phrase to look like:
Copyright (c) <DATE> <COMPANY_NAME>

Note that this is different from the above description of putting a "(C)" and then the word "Copyright".

Now I have no idea if this is the same for all countries, I only know what I was told to do.

Hope this helps, and I really don't want to start a big "copyright" thread, as I'm not a lawyer, I only have to deal with them on a constant basis :)

thanks,

greg k-h

I can put together a new patch later tonight that reverts to the old size scheme. It was more complicated since you need to allocate each attribute.

I'll attach a newer version that incorporates a little feedback about creating a function to remove the attribute for devices that don't want to expose the ROM.

Alan Cox had concerns about copying the ROMs for those devices that don't implement full address decoding. I'm using kmalloc for 40-60KB. Would vmalloc be a better choice? Very few drivers will use the copy option, mostly old hardware.

BenH said he would check it out on ppc but I haven't heard from him yet.

Jesse, did you notice that the quirk for tracking the boot video device is x86 only? I believe this needs to run on ia64 and x86_64 too. How do we want to do that? It will do the wrong thing on architectures that don't shadow video ROMs to C0000.

I'll add this

I can revert back to the old code. The new scheme was just so much simpler.

=====
Jon Smirl
[email protected]

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

===== arch/i386/pci/fixup.c 1.19 vs edited =====

@@ -237,6 +237,29 @@
 	}
 }
 
+static void __devinit pci_fixup_video(struct pci_dev *pdev)
+{
+	struct pci_dev *bridge;
+	struct pci_bus *bus;
+	u16 l;
+	
+	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+		return;
+	       
+	/* Is VGA routed to us? */
+	bus = pdev->bus;
+	while (bus) {
+		bridge = bus->self;
+		if (bridge) {
+			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &l);
+			if (!(l & PCI_BRIDGE_CTL_VGA))
+				return;
+		}
+		bus = bus->parent;
+	}
+	pdev->resource[PCI_ROM_RESOURCE].flags |= PCI_ROM_SHADOW;
+}
+
 struct pci_fixup pcibios_fixups[] = {
 	{
 		.pass		= PCI_FIXUP_HEADER,

@@ -345,6 +368,12 @@
 		.vendor		= PCI_VENDOR_ID_NVIDIA,
 		.device		= PCI_DEVICE_ID_NVIDIA_NFORCE2,
 		.hook		= pci_fixup_nforce2
+	},
+	{
+		.pass		= PCI_FIXUP_FINAL,
+		.vendor		= PCI_ANY_ID,
+		.device		= PCI_ANY_ID,
+		.hook		= pci_fixup_video
 	},
 	{ .pass = 0 }
 };

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

===== drivers/pci/pci.h 1.12 vs edited =====

@@ -3,6 +3,7 @@
 extern int pci_hotplug (struct device *dev, char **envp, int num_envp,
 			 char *buffer, int buffer_size);
 extern void pci_create_sysfs_dev_files(struct pci_dev *pdev);
+extern void pci_remove_sysfs_dev_files(struct pci_dev *pdev);
 extern int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
 				  unsigned long size, unsigned long align,
 				  unsigned long min, unsigned int type_mask,

===== drivers/pci/remove.c 1.3 vs edited =====

@@ -12,12 +12,22 @@
 
 static void pci_free_resources(struct pci_dev *dev)
 {
+	struct resource *res;
 	int i;
 
  	msi_remove_pci_irq_vectors(dev);
 
+	if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
+		res = &dev->resource[PCI_ROM_RESOURCE];
+		if (res->flags & PCI_ROM_COPY) {
+			kfree((void*)res->start);
+			res->flags &= !PCI_ROM_COPY;
+			res->start = 0;
+			res->end = 0;
+		}
+	}
 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-		struct resource *res = dev->resource + i;
+		res = dev->resource + i;
 		if (res->parent)
 			release_resource(res);
 	}

@@ -26,6 +36,7 @@
 static void pci_destroy_dev(struct pci_dev *dev)
 {
 	pci_proc_detach_device(dev);
+	pci_remove_sysfs_dev_files(dev);
 	device_unregister(&dev->dev);
 
 	/* Remove the device from the device lists, and prevent any further

===== include/linux/pci.h 1.132 vs edited =====

@@ -102,6 +102,8 @@
 #define PCI_SUBSYSTEM_ID	0x2e  
 #define PCI_ROM_ADDRESS		0x30	/* Bits 31..11 are address, 10..1 reserved */
 #define  PCI_ROM_ADDRESS_ENABLE	0x01
+#define  PCI_ROM_SHADOW		0x02	/* resource flag, ROM is copy at C000:0 */
+#define  PCI_ROM_COPY		0x04	/* resource flag, ROM is alloc'd copy */
 #define PCI_ROM_ADDRESS_MASK	(~0x7ffUL)
 
 #define PCI_CAPABILITY_LIST	0x34	/* Offset of first capability list entry */

@@ -777,6 +779,12 @@
 int pci_dac_set_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_assign_resource(struct pci_dev *dev, int i);
+
+/* ROM control related routines */
+unsigned char *pci_map_rom(struct pci_dev *dev, size_t *size);
+unsigned char *pci_map_rom_copy(struct pci_dev *dev, size_t *size);
+void pci_unmap_rom(struct pci_dev *dev, unsigned char *rom);
+void pci_remove_rom(struct pci_dev *dev);
 
 /* Power management related routines */
 int pci_save_state(struct pci_dev *dev, u32 *buffer);

Yeah, but I don't know of any ia64 platforms that need the quirk. All of them that I'm aware of use add-on boards.

Jesse

As I said before you don't need to allocate big chunks of kernel memory for this because you don't want to store ROM copies in kernel, you just disallow mmap in such a case and let the user use read().

I am opposed to anything that keeps ROM copies in the kernel.

How are we supposed to implement this without a copy? Once the device driver is loaded there is never a safe way access the ROM again because an interrupt or another CPU might use the PCI decoders to access the other hardware and disrupt the ROM read. You have to copy the ROM when the driver says it is safe.

I provided two calls for the driver to pick from: 1) make an in kernel copy of the ROM and leave it visible from sysfs or 2) remove the sysfs attribute.

Another possible scheme could have a user space daemon that copies the ROMs out of the kernel at boot and then holds them for later access by other apps, but that would break the ROM attribute in sysfs model.

Still another scheme would be to make the drivers for this class of card implement a lock around PCI address decoder use. That would get complex with interrupt routines.

How much trouble do we want to go to handling a case that only applies to very few cards? I believe old QLogic disk controllers have this problem, are there others? I'm not aware of any video cards with this problem. 99%+ of PCI ROMs don't need the copy.

=====
Jon Smirl
[email protected]

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

It's never safe essentially. The only way you can make it safe for this case is to put the knowledge in the device driver for that specific card rather than sysfs.

That would be impossible in fact because of DMA and SMM activity.

It would be better to just to blacklist those corner cases

I added these two calls to the pci API just for the case of partially decoded hardware. The device driver for the hardware needs to make these calls.

unsigned char *pci_map_rom_copy(struct pci_dev *dev, size_t *size);

Call this one from the driver when it is safe to read the ROM. It will copy it and then provide a virtual address so that you can read it. The copy is stored in the kernel so that the sysfs attribute will work right.

void pci_remove_rom(struct pci_dev *dev);

Call this one from the driver to simply remove the ROM attribute.

Before the driver is loaded we have to assume that it safe to read the ROM normally and the sysfs attribute will directly access the ROM.

unsigned char *pci_map_rom(struct pci_dev *dev, size_t *size);

For normal hardware that implements full decoding use this call. It will automatically sort out the need to use the real ROM or a shadow copy.

void pci_unmap_rom(struct pci_dev *dev, unsigned char *rom);

When you are done with a mapping use this

Normal ROMs do not make copies. The only time a copy happens is when a device driver for a partially decoded device calls pci_map_rom_copy().

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
New and Improved Yahoo! Mail - Send 10MB messages!
http://promotions.yahoo.com/new_mail

I have to add a pointer to struct pci_dev to track the attribute copy with the size in it. Would you rather have me add the pointer or or change the sysfs rules to state that a copy of the length is made?

=====
Jon Smirl
[email protected]

__________________________________
Do you Yahoo!?
New and Improved Yahoo! Mail - Send 10MB messages!
http://promotions.yahoo.com/new_mail

Adding a pointer is fine for now.

thanks,

greg k-h

This is a new version of Jesse's PCI ROM patch.

It can read ROMs on x86. Main problem was that the PCI address space is not part of the kernel address space on the x86 so ioremap() is needed. I added the parts about assign/release resource but I am not sure that they are needed.

The patch obviously needs more work for the not_direct case. The quirk to record the boot video card has not been written.

I wasted a lot of time figuring out why it wouldn't work until I remembered that my radeon card needs a kernel quirk to fix it's ROM enabling. Most radeon cards have this problem so if you get FFFF the code is problem working, I just need to write the quirk to enable the ROM.

=====
Jon Smirl
[email protected]

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

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

@@ -164,6 +164,108 @@
 	return count;
 }
 
+/**
+ * pci_enable_rom - enable ROM decoding for a PCI device
+ * @dev: PCI device to enable
+ *
+ * Enable ROM decoding on @dev.  This involves simply turning on the last
+ * bit of the PCI ROM BAR.  Note that some cards may share address decoders
+ * between the ROM and other resources, so enabling it may disable access
+ * to MMIO registers or other card memory.
+ */
+static void
+pci_enable_rom(struct pci_dev *dev)
+{
+	u32 rom_addr;
+	
+	pci_read_config_dword(dev, dev->rom_base_reg, &rom_addr);
+	rom_addr |= PCI_ROM_ADDRESS_ENABLE;
+	pci_write_config_dword(dev, dev->rom_base_reg, rom_addr);
+}
+
+/**
+ * pci_disable_rom - disable ROM decoding for a PCI device
+ * @dev: PCI device to disable
+ *
+ * Disable ROM decoding on a PCI device by turning off the last bit in the
+ * ROM BAR.
+ */
+static void
+pci_disable_rom(struct pci_dev *dev)
+{
+	u32 rom_addr;
+
+	pci_read_config_dword(dev, dev->rom_base_reg, &rom_addr);
+	rom_addr &= ~PCI_ROM_ADDRESS_ENABLE;
+	pci_write_config_dword(dev, dev->rom_base_reg, rom_addr);
+}
+
+/**
+ * pci_read_rom - read a PCI ROM
+ * @kobj: kernel object handle
+ * @buf: where to put the data we read from the ROM
+ * @off: file offset
+ * @count: number of bytes to read
+ *
+ * Put @count bytes starting at @off into @buf from the ROM in the PCI
+ * device corresponding to @kobj.
+ */
+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));
+	unsigned long start;
+	loff_t i, size;
+	char direct_access = dev->rom_info.rom ? 0 : 1;
+	unsigned char *rom = NULL;
+	struct resource *r = &dev->resource[PCI_ROM_RESOURCE];
+
+	if (off > size)
+		return 0;
+		
+	if (direct_access) {
+		/* assign the ROM an address if it doesn't have one */
+		if (r->parent == NULL)
+			pci_assign_resource(dev, PCI_ROM_RESOURCE);
+		/* Enable ROM space decodes and do the reads */
+		pci_enable_rom(dev);
+		start = pci_resource_start(dev, PCI_ROM_RESOURCE);
+		size = pci_resource_len(dev, PCI_ROM_RESOURCE);
+		rom = ioremap(start, size);
+		
+		printk("read_rom start %lx size %x\n", start, size);
+		printk("rom bytes %02x %02x\n", rom, rom + 1);
+	}
+	if (off + count > size) {
+		size -= off;
+		count = size;
+	} else
+		size = count;
+
+	i = 0;
+	while (size > 0) {
+		unsigned char val;
+		if (direct_access)
+			val = readb(rom + off);
+		else
+			val = *(dev->rom_info.rom + off);
+		buf[i++] = val;
+		off++;
+		--size;
+	}
+	if (direct_access) {
+		iounmap(rom);
+		/* Disable again before continuing */
+		pci_disable_rom(dev);
+		if (r->parent) {
+			release_resource(r);
+			r->end -= r->start;
+			r->start = 0;
+		}
+	}
+	return count;
+}
+
 static struct bin_attribute pci_config_attr = {
 	.attr =	{
 		.name = "config",

@@ -186,13 +288,51 @@
 	.write = pci_write_config,
 };
 
-void pci_create_sysfs_dev_files (struct pci_dev *pdev)
+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);
 
+printk("devfn %x len %lx start %lx\n", pdev->devfn, pci_resource_len(pdev, PCI_ROM_RESOURCE), pci_resource_start(pdev, PCI_ROM_RESOURCE));
+	/* If the device has a ROM, try to expose it in sysfs. */
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE)) {
+		struct bin_attribute *rom_attr;
+		rom_attr = kmalloc(sizeof(*rom_attr), GFP_ATOMIC);
+
+		pdev->rom_info.rom_attr = NULL;
+		if (!rom_attr)
+			goto out;
+
+		pdev->rom_info.rom_attr = rom_attr;
+		rom_attr->attr.name = "rom";
+		rom_attr->attr.mode = S_IRUSR;
+		rom_attr->attr.owner = THIS_MODULE;
+		rom_attr->read = pci_read_rom;
+		rom_attr->size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+		sysfs_create_bin_file(&pdev->dev.kobj, rom_attr);
+	}
+ out:
 	/* add platform-specific attributes */
 	pcibios_add_platform_entries(pdev);
+}
+
+/**
+ * pci_remove_sysfs_dev_files - cleanup PCI specific sysfs files
+ * @pdev: device whose entries we should free
+ *
+ * Cleanup when @pdev is removed from sysfs.
+ */
+void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
+{
+	if (pdev->cfg_size < 4096)
+		sysfs_remove_bin_file(&pdev->dev.kobj, &pci_config_attr);
+	else
+		sysfs_remove_bin_file(&pdev->dev.kobj, &pcie_config_attr);
+
+	if (pdev->rom_info.rom_attr) {
+		sysfs_remove_bin_file(&pdev->dev.kobj, pdev->rom_info.rom_attr);
+		kfree(pdev->rom_info.rom_attr);
+	}
 }

===== drivers/pci/pci.h 1.12 vs edited =====

@@ -3,6 +3,7 @@
 extern int pci_hotplug (struct device *dev, char **envp, int num_envp,
 			 char *buffer, int buffer_size);
 extern void pci_create_sysfs_dev_files(struct pci_dev *pdev);
+extern void pci_remove_sysfs_dev_files(struct pci_dev *pdev);
 extern int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
 				  unsigned long size, unsigned long align,
 				  unsigned long min, unsigned int type_mask,

===== drivers/pci/probe.c 1.65 vs edited =====

@@ -157,6 +157,8 @@
 #endif
 		}
 	}
+
+	dev->rom_info.rom = NULL;
 	if (rom) {
 		dev->rom_base_reg = rom;
 		res = &dev->resource[PCI_ROM_RESOURCE];

===== drivers/pci/remove.c 1.3 vs edited =====

@@ -26,6 +26,9 @@
 static void pci_destroy_dev(struct pci_dev *dev)
 {
 	pci_proc_detach_device(dev);
+	/* Free the copy of the ROM if we made one */
+	kfree(dev->rom_info.rom);
+	pci_remove_sysfs_dev_files(dev);
 	device_unregister(&dev->dev);
 
 	/* Remove the device from the device lists, and prevent any further

===== include/linux/pci.h 1.132 vs edited =====

@@ -471,6 +471,11 @@
 	pci_mmap_mem
 };
 
+struct rom_info {
+	char *rom; /* copy of the ROM if necessary */
+	struct bin_attribute *rom_attr;
+};
+
 /* This defines the direction arg to the DMA mapping routines. */
 #define PCI_DMA_BIDIRECTIONAL	0
 #define PCI_DMA_TODEVICE	1

@@ -537,6 +542,7 @@
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	
 	unsigned int 	saved_config_space[16]; /* config space saved at suspend time */
+	struct rom_info rom_info; /* How and where to get the ROM info for this device */
 #ifdef CONFIG_PCI_NAMES
 #define PCI_NAME_SIZE	96
 #define PCI_NAME_HALF	__stringify(43)	/* less than half to handle slop */

Ah, that would explain it. Thanks for fixing it up.

Jesse

Then the driver core will clean them up, like it does today :)

But it's "not nice", and we should clean them up as we now have a function in which we can do that, and based on changes we have planned for the driver core in the future, it will be something that is required to do later on. Might as well do it now, as you are modifying the same code...

thanks,

greg k-h

Here's another grungy thing I needed to do to PCI. Multi-headed video cards don't really implement independent PCI devices even though they look like independent devices. I've heard that this behavior is needed for MS Windows compatibility.

I'm missing a PCI call to claim ownership for the secondary device without also causing a second instance of my driver to be loaded. Here's the code I'm using, what's the right way to do this?

struct pci_dev *secondary;
/* check the next function on the same card */
secondary = pci_find_slot(dev->pdev->bus->number, dev->pdev->devfn + 1);

if (secondary) {
	/* check if class is secondary video */
	if (secondary->class == 0x038000) {
		DRM_DEBUG("registering secondary video head\n");
		/* This code is need to bind the driver to the secondary device */
		/* There is no direct pci call to do this, there should be one */
		secondary->dev.driver = dev->pdev->dev.driver;
		device_bind_driver(&secondary->dev);
		/* dev->pdev->driver is not filled until after probe completes */
		secondary->driver = to_pci_driver(dev->pdev->dev.driver);
		pci_dev_get(secondary);
	}
}

=====
Jon Smirl
[email protected]

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

You can do that, but where is the problem with your probe function being called twice - once for each of the devices? You should be able to sort out which one is which rather easily.

Vojtech Pavlik
SuSE Labs, SuSE CR

I wrote this a while ago, but I believe the problem was that by accepting two probes I get two hotplug ADD events that I can't tell apart. This scheme avoids triggering hotplug. The other problem was that the ADD events get started in parallel and I couldn't figure out how to serialize them. The parallel hotplug programs got into a race to see who could initialize the card first.

Now that I know more about hotplug I could modify the hotplug parameters to indicate primary vs secondary and ignore the secondary one. But this will still cause two apps to be started in parallel.

Another solution would be to modify the kernel API somehow to let me suppress hotplug ADD/REMOVE on the secondary device.

=====
Jon Smirl
[email protected]

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

But as it's easy to tell which one is the secondary, just don't run the userspace app for that device. I don't recommend doing your "grab the other device" hack in the driver, as it's not not a very nice thing to do, and will probably break in the future.

Well, as your driver isn't even loaded at that point in time, it's a bit hard for it to control that :)

thanks,

greg k-h