Re: [PATCH] add PCI ROMs to sysfs

[Martin Mares <mj@ucw.cz>]

Hello!

Just a couple of notes about your patch:

+unsigned char *
+pci_map_rom(struct pci_dev *dev, size_t *size) {
+	struct resource *res = &dev->resource[PCI_ROM_RESOURCE];
+	loff_t start;
+	unsigned char *rom;
+	
+	if (res->flags & PCI_ROM_SHADOW) {	/* PCI_ROM_SHADOW only set on x86 */
+		start = (loff_t)0xC0000; 	/* primary video rom always starts here */
+		*size = 0x20000;		/* cover C000:0 through E000:0 */

Shouldn't we do this only if we find that the device has a ROM resource?

+	} else if (res->flags & PCI_ROM_COPY) {
+		*size = pci_resource_len(dev, PCI_ROM_RESOURCE);
+		return (unsigned char *)pci_resource_start(dev, PCI_ROM_RESOURCE);
+	} else {
+		start = pci_resource_start(dev, PCI_ROM_RESOURCE);
+		*size = pci_resource_len(dev, PCI_ROM_RESOURCE);
+		if (*size == 0)
+			return NULL;
+		
+		/* Enable ROM space decodes */
+		pci_enable_rom(dev);
+	}

This seems to be wrong -- before enabling a resource, you must call
request_resource() on it and make sure that the ROM (1) has an address
allocated, and (2) the address is not used by anything else.

+	/* If the device has a ROM, try to expose it in sysfs. */
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE)) {
+		unsigned char *rom;
+		struct bin_attribute *rom_attr;
+		
+		pdev->rom_attr = NULL;
+		rom_attr = kmalloc(sizeof(*rom_attr), GFP_ATOMIC);
+		if (rom_attr) {
+			/* set default size */
+			rom_attr->size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+			/* get true size if possible */
+			rom = pci_map_rom(pdev, &rom_attr->size);

As we can never be sure about the decoder sharing, it is very wise not to
touch the ROM BAR until somebody accesses the sysfs file. Using size according
to the PCI config space (i.e., the resource) does not harm anybody.

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

~, not !

+			res->start = 0;
+			res->end = 0;
+		}
+	}

This should better be handled in a separate function in the same source
file as the rest of the ROM handling code.

Also, what about ROMs in the other header types? Wouldn't it be better to
scan all resources for the COPY flag instead?

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

This does not belong here! This part of pci.h describes the configuration
space, not stuff internal to the kernel. Better introduce a new resource
flag in <linux/ioport.h>.

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
"Oh no, not again!"  -- The bowl of petunias