[PATCH] add PCI ROMs to sysfs

[PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

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

[-- Attachment #2: pci-sysfs-rom-4.patch --]
[-- Type: text/plain, Size: 4456 bytes --]

===== drivers/pci/pci-sysfs.c 1.10 vs edited =====
--- 1.10/drivers/pci/pci-sysfs.c	2004-06-04 06:23:04 -07:00
+++ edited/drivers/pci/pci-sysfs.c	2004-07-30 14:04:40 -07:00
@@ -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 =====
--- 1.3/drivers/pci/remove.c	2004-02-03 09:17:30 -08:00
+++ edited/drivers/pci/remove.c	2004-07-30 13:35:18 -07:00
@@ -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 =====
--- 1.130/include/linux/pci.h	2004-06-30 11:21:27 -07:00
+++ edited/include/linux/pci.h	2004-07-30 13:37:09 -07:00
@@ -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 */

Re: [PATCH] add PCI ROMs to sysfs

[Greg KH]

On Fri, Jul 30, 2004 at 02:09:05PM -0700, Jesse Barnes wrote:
> 
> 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.

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...

> +void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
> +{
> +	/* Don't need to free config entries since they're static & global */

What do you mean by this?  You should still remove all files we added in
the pci_create_sysfs_dev_files() function here, not just the rom file.

thanks,

greg k-h

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Friday, July 30, 2004 2:29 pm, Greg KH wrote:
> On Fri, Jul 30, 2004 at 02:09:05PM -0700, Jesse Barnes wrote:
> > 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.
>
> 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...

> > +void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
> > +{
> > +	/* Don't need to free config entries since they're static & global */
>
> What do you mean by this?  You should still remove all files we added in
> the pci_create_sysfs_dev_files() function here, not just the rom file.

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

Re: [PATCH] add PCI ROMs to sysfs

[Greg KH]

On Fri, Jul 30, 2004 at 02:34:50PM -0700, Jesse Barnes wrote:
> > > +void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
> > > +{
> > > +	/* Don't need to free config entries since they're static & global */
> >
> > What do you mean by this?  You should still remove all files we added in
> > the pci_create_sysfs_dev_files() function here, not just the rom file.
> 
> What will happen if we don't?

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

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

On Friday, July 30, 2004 2:34 pm, Jesse Barnes wrote:
> On Friday, July 30, 2004 2:29 pm, Greg KH wrote:
> > On Fri, Jul 30, 2004 at 02:09:05PM -0700, Jesse Barnes wrote:
> > > 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.
> >
> > 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...

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

[-- Attachment #2: pci-sysfs-rom-6.patch --]
[-- Type: text/plain, Size: 4569 bytes --]

===== drivers/pci/pci-sysfs.c 1.10 vs edited =====
--- 1.10/drivers/pci/pci-sysfs.c	2004-06-04 06:23:04 -07:00
+++ edited/drivers/pci/pci-sysfs.c	2004-07-30 14:37:04 -07:00
@@ -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 =====
--- 1.3/drivers/pci/remove.c	2004-02-03 09:17:30 -08:00
+++ edited/drivers/pci/remove.c	2004-07-30 13:35:18 -07:00
@@ -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 =====
--- 1.130/include/linux/pci.h	2004-06-30 11:21:27 -07:00
+++ edited/include/linux/pci.h	2004-07-30 13:37:09 -07:00
@@ -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 */

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

--- Jesse Barnes <jbarnes@engr.sgi.com> wrote:
> 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?

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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Vojtech Pavlik]

On Fri, Jul 30, 2004 at 02:53:25PM -0700, Jon Smirl wrote:
> 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);
> 	}
> }

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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

--- Vojtech Pavlik <vojtech@suse.cz> wrote:
> On Fri, Jul 30, 2004 at 02:53:25PM -0700, Jon Smirl wrote:
> > Here's another grungy thing I needed to do to PCI. Multi-headed
> video
> 
> 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.

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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Greg KH]

On Sat, Jul 31, 2004 at 06:28:28AM -0700, Jon Smirl wrote:
> --- Vojtech Pavlik <vojtech@suse.cz> wrote:
> > On Fri, Jul 30, 2004 at 02:53:25PM -0700, Jon Smirl wrote:
> > > Here's another grungy thing I needed to do to PCI. Multi-headed
> > video
> > 
> > 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.
> 
> 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. 

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.

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

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

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Friday, July 30, 2004 3:15 pm, Jon Smirl wrote:
> If you set pci_dev->resource[PCI_ROM_RESOURCE] to C000:0 won't this
> mess up pci_assign_resource()/release_resource()?

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

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

On Saturday, July 31, 2004 8:59 am, Jesse Barnes wrote:
> On Friday, July 30, 2004 3:15 pm, Jon Smirl wrote:
> > If you set pci_dev->resource[PCI_ROM_RESOURCE] to C000:0 won't this
> > mess up pci_assign_resource()/release_resource()?
>
> 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?

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

[-- Attachment #2: pci-sysfs-rom-7.patch --]
[-- Type: text/plain, Size: 6230 bytes --]

===== drivers/pci/pci-sysfs.c 1.10 vs edited =====
--- 1.10/drivers/pci/pci-sysfs.c	2004-06-04 06:23:04 -07:00
+++ edited/drivers/pci/pci-sysfs.c	2004-08-02 09:57:11 -07:00
@@ -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 =====
--- 1.12/drivers/pci/pci.h	2004-06-04 06:23:04 -07:00
+++ edited/drivers/pci/pci.h	2004-08-02 09:41:02 -07:00
@@ -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 =====
--- 1.65/drivers/pci/probe.c	2004-05-21 11:45:27 -07:00
+++ edited/drivers/pci/probe.c	2004-08-02 09:43:21 -07:00
@@ -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 =====
--- 1.3/drivers/pci/remove.c	2004-02-03 09:17:30 -08:00
+++ edited/drivers/pci/remove.c	2004-08-02 09:41:52 -07:00
@@ -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 =====
--- 1.130/include/linux/pci.h	2004-06-30 11:21:27 -07:00
+++ edited/include/linux/pci.h	2004-08-02 09:32:08 -07:00
@@ -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 */

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

--- Jesse Barnes <jbarnes@engr.sgi.com> wrote:
> 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.

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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

--- Jesse Barnes <jbarnes@engr.sgi.com> wrote:
> It it suitable for the mainline yet?  I expect those familiar with 
> the various cards to add the necessary quirks code as needed.

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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Monday, August 2, 2004 2:00 pm, Jon Smirl wrote:
> --- Jesse Barnes <jbarnes@engr.sgi.com> wrote:
> > It it suitable for the mainline yet?  I expect those familiar with
> > the various cards to add the necessary quirks code as needed.
>
> 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?

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

Re: [PATCH] add PCI ROMs to sysfs

[Alan Cox]

What guarantees the ROM already has an assigned PCI address ?

Re: [PATCH] add PCI ROMs to sysfs

[Alan Cox]

On Llu, 2004-08-02 at 22:05, Jesse Barnes wrote:
> > 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?
> 
> I think that would be a quirk.  You'd copy ROMs like that into the rom pointer 
> in the pci_dev structure I guess.

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.

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Monday, August 2, 2004 4:30 pm, Alan Cox wrote:
> 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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

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.


--- Jesse Barnes <jbarnes@engr.sgi.com> wrote:

> On Monday, August 2, 2004 4:30 pm, Alan Cox wrote:
> > 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
> 

=====
Jon Smirl
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Monday, August 2, 2004 7:32 pm, Jon Smirl wrote:
> 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.

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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Tuesday, August 3, 2004 2:19 pm, Jon Smirl wrote:
> 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?

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

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Tuesday, August 3, 2004 2:28 pm, Jesse Barnes wrote:
> > Both of the video ROMs are at 00020000, won't they end up on top of
> > each other when enabled?
>
> 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.

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

Re: [PATCH] add PCI ROMs to sysfs

[Martin Mares]

> 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.

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

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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

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

--- Martin Mares <mj@ucw.cz> wrote:

> > 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.
> 
> ... or try `lspci -b', it will dump the actual registers, not the
> kernel's
> view.
> 
> 				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
> A jury consists of 12 persons chosen to decide who has the better
> lawyer.
> 


=====
Jon Smirl
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Martin Mares]

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

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

The following quick hack should cure it:

--- orig/lib/generic.c
+++ mod/lib/generic.c
@@ -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   <mj@ucw.cz>   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 and ROM on bridge, was: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

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
jonsmirl@yahoo.com


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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pci-sysfs-rom-8.patch --]
[-- Type: text/x-patch; name="pci-sysfs-rom-8.patch", Size: 6830 bytes --]

===== drivers/pci/pci-sysfs.c 1.10 vs edited =====
--- 1.10/drivers/pci/pci-sysfs.c	Fri Jun  4 09:23:04 2004
+++ edited/drivers/pci/pci-sysfs.c	Wed Aug  4 01:37:15 2004
@@ -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 =====
--- 1.12/drivers/pci/pci.h	Fri Jun  4 09:23:04 2004
+++ edited/drivers/pci/pci.h	Tue Aug  3 17:05:19 2004
@@ -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 =====
--- 1.65/drivers/pci/probe.c	Fri May 21 14:45:27 2004
+++ edited/drivers/pci/probe.c	Tue Aug  3 17:05:19 2004
@@ -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 =====
--- 1.3/drivers/pci/remove.c	Tue Feb  3 12:17:30 2004
+++ edited/drivers/pci/remove.c	Tue Aug  3 17:05:20 2004
@@ -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 =====
--- 1.132/include/linux/pci.h	Mon Aug  2 04:00:43 2004
+++ edited/include/linux/pci.h	Tue Aug  3 17:05:21 2004
@@ -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 */

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Tuesday, August 3, 2004 11:08 pm, Jon Smirl wrote:
> 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.

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

Jesse

Re: lspci and ROM on bridge, was: [PATCH] add PCI ROMs to sysfs

[Martin Mares]

> 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.

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

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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

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
jonsmirl@yahoo.com


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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pci-sysfs-rom-10.patch --]
[-- Type: text/x-patch; name="pci-sysfs-rom-10.patch", Size: 7275 bytes --]

===== arch/i386/pci/fixup.c 1.19 vs edited =====
--- 1.19/arch/i386/pci/fixup.c	Thu Jun  3 10:58:17 2004
+++ edited/arch/i386/pci/fixup.c	Thu Aug  5 00:20:08 2004
@@ -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 =====
--- 1.10/drivers/pci/pci-sysfs.c	Fri Jun  4 09:23:04 2004
+++ edited/drivers/pci/pci-sysfs.c	Thu Aug  5 00:45:04 2004
@@ -164,6 +164,95 @@
 	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));
+	loff_t start;
+	size_t size;
+	unsigned char *rom;
+	struct resource *res = &dev->resource[PCI_ROM_RESOURCE];
+
+	if (res->flags & PCI_ROM_SHADOW) {	/* PCI_ROM_SHADOW only set on x86 */
+		start = (unsigned char *)0xC0000; /* primary video rom always starts here */
+		size = 0x20000;			/* cover C000:0 through E000:0 */
+	} else {
+		/* assign the ROM an address if it doesn't have one */
+		if (res->parent == NULL)
+			pci_assign_resource(dev, PCI_ROM_RESOURCE);
+			
+		start = pci_resource_start(dev, PCI_ROM_RESOURCE);
+		size = pci_resource_len(dev, PCI_ROM_RESOURCE);
+		
+		/* Enable ROM space decodes */
+		pci_enable_rom(dev);
+	}
+	if (off >= size)
+		return 0;
+		
+	if (off + count > size)
+		count = size - off;
+	
+	rom = ioremap(start, size);
+	if (rom) {
+		memcpy_fromio(buf, rom + off, count);
+		iounmap(rom);
+	} else
+		count = 0;
+		
+	/* Disable again before continuing, leave enabled if pci=rom */
+	if (!(res->flags & (PCI_ROM_ADDRESS_ENABLE|PCI_ROM_SHADOW)))
+		pci_disable_rom(dev);
+		
+	return count;
+}
+
 static struct bin_attribute pci_config_attr = {
 	.attr =	{
 		.name = "config",
@@ -186,13 +275,49 @@
 	.write = pci_write_config,
 };
 
-void pci_create_sysfs_dev_files (struct pci_dev *pdev)
+static struct bin_attribute rom_attr = {
+	.attr =	{
+		.name = "rom",
+		.mode = S_IRUSR,
+		.owner = THIS_MODULE,
+	},
+	/* .size is set individually for each device, sysfs copies it into dentry */
+	.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, try to expose it in sysfs. */
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE)) {
+		struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
+		if (res->flags & PCI_ROM_SHADOW) {
+			rom_attr.size = 0x20000;	/* cover C000:0 through E000:0 */
+		} else
+			rom_attr.size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+		sysfs_create_bin_file(&pdev->dev.kobj, &rom_attr);
+	}
 	/* 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))
+		sysfs_remove_bin_file(&pdev->dev.kobj, &rom_attr);
 }
===== drivers/pci/pci.h 1.12 vs edited =====
--- 1.12/drivers/pci/pci.h	Fri Jun  4 09:23:04 2004
+++ edited/drivers/pci/pci.h	Tue Aug  3 17:05:19 2004
@@ -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 =====
--- 1.65/drivers/pci/probe.c	Fri May 21 14:45:27 2004
+++ edited/drivers/pci/probe.c	Thu Aug  5 00:10:06 2004
@@ -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 =====
--- 1.3/drivers/pci/remove.c	Tue Feb  3 12:17:30 2004
+++ edited/drivers/pci/remove.c	Thu Aug  5 00:11:05 2004
@@ -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 =====
--- 1.132/include/linux/pci.h	Mon Aug  2 04:00:43 2004
+++ edited/include/linux/pci.h	Thu Aug  5 00:13:54 2004
@@ -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 */

Re: [PATCH] add PCI ROMs to sysfs

[Benjamin Herrenschmidt]

On Thu, 2004-08-05 at 15:05, Jon Smirl wrote:
> 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.

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 ?

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

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.

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

--- Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 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 ?

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.

> > I did the x86 quirk, what do the quirks on ia64, ppc, x86_64 need?
> Can
> > they just copy the x86 one?
> 
> 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...

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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Wednesday, August 4, 2004 10:05 pm, Jon Smirl wrote:
> Version 10

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

Jesse

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Thursday, August 5, 2004 8:54 am, Jesse Barnes wrote:
> On Wednesday, August 4, 2004 10:05 pm, Jon Smirl wrote:
> > Version 10
>
> For some reason this version doesn't work on ia64.  It just returns bytes
> containing 0 when I try to dump the ROM.

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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

--- Jesse Barnes <jbarnes@engr.sgi.com> wrote:
> 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?

/* 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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Thursday, August 5, 2004 1:45 pm, Jon Smirl wrote:
> 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.

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.

> 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.

Right.

Thanks,
Jesse

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]

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
jonsmirl@yahoo.com


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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pci-sysfs-rom-11.patch --]
[-- Type: text/x-patch; name="pci-sysfs-rom-11.patch", Size: 10553 bytes --]

===== arch/i386/pci/fixup.c 1.19 vs edited =====
--- 1.19/arch/i386/pci/fixup.c	Thu Jun  3 10:58:17 2004
+++ edited/arch/i386/pci/fixup.c	Thu Aug  5 00:20:08 2004
@@ -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 =====
--- 1.10/drivers/pci/pci-sysfs.c	Fri Jun  4 09:23:04 2004
+++ edited/drivers/pci/pci-sysfs.c	Fri Aug  6 16:38:00 2004
@@ -5,6 +5,7 @@
  * (C) Copyright 2002-2004 IBM Corp.
  * (C) Copyright 2003 Matthew Wilcox
  * (C) Copyright 2003 Hewlett-Packard
+ * (C) Copyright 2004 Jon Smirl
  *
  * File attributes for PCI devices
  *
@@ -164,6 +165,176 @@
 	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_map_rom - map a PCI ROM to kernel space
+ * @dev: pointer to pci device struct
+ * @size: pointer to receive size of pci window over ROM
+ * @return: kernel virtual pointer to image of ROM
+ *
+ * Map a PCI ROM into kernel space. If ROM is boot video ROM,
+ * the shadow BIOS copy will be returned instead of the 
+ * actual ROM.
+ */
+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 */
+	} 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);
+	}
+	
+	rom = ioremap(start, *size);
+	if (!rom) {
+		/* restore enable if ioremap fails */
+		if (!(res->flags & (PCI_ROM_ADDRESS_ENABLE | PCI_ROM_SHADOW | PCI_ROM_COPY)))
+			pci_disable_rom(dev);
+		return NULL;
+	}		
+	/* Standard PCI ROMs start out with these three bytes 55 AA size/512 */
+	if ((*rom == 0x55) && (*(rom + 1) == 0xAA))
+		*size = *(rom + 2) * 512;	/* return true ROM size, not PCI window size */
+		
+	return rom;
+}
+
+/**
+ * pci_map_rom_copy - map a PCI ROM to kernel space, create a copy
+ * @dev: pointer to pci device struct
+ * @size: pointer to receive size of pci window over ROM
+ * @return: kernel virtual pointer to image of ROM
+ *
+ * Map a PCI ROM into kernel space. If ROM is boot video ROM,
+ * the shadow BIOS copy will be returned instead of the 
+ * actual ROM.
+ */
+unsigned char *
+pci_map_rom_copy(struct pci_dev *dev, size_t *size) {
+	struct resource *res = &dev->resource[PCI_ROM_RESOURCE];
+	unsigned char *rom;
+	
+	rom = pci_map_rom(dev, size);
+	if (!rom)
+		return NULL;
+		
+	if (res->flags & (PCI_ROM_COPY | PCI_ROM_SHADOW))
+		return rom;
+		
+	res->start = (unsigned long)kmalloc(*size, GFP_KERNEL);
+	if (!res->start) 
+		return rom;
+
+	res->end = res->start + *size; 
+	memcpy((void*)res->start, rom, *size);
+	pci_unmap_rom(dev, rom);
+	res->flags |= PCI_ROM_COPY;
+	
+	return (unsigned char *)res->start;
+}
+
+/**
+ * pci_unmap_rom - unmap the ROM from kernel space
+ * @dev: pointer to pci device struct
+ * @rom: virtual address of the previous mapping
+ *
+ * Remove a mapping of a previously mapped ROM
+ */
+void 
+pci_unmap_rom(struct pci_dev *dev, unsigned char *rom) {
+	struct resource *res = &dev->resource[PCI_ROM_RESOURCE];
+
+	if (res->flags & PCI_ROM_COPY)
+		return;
+		
+	iounmap(rom);
+		
+	/* Disable again before continuing, leave enabled if pci=rom */
+	if (!(res->flags & (PCI_ROM_ADDRESS_ENABLE | PCI_ROM_SHADOW)))
+		pci_disable_rom(dev);
+}
+
+/**
+ * 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 char *rom;
+	size_t size;
+	
+	rom = pci_map_rom(dev, &size);	/* size starts out as PCI window size */
+	if (!rom)
+		return 0;
+		
+	if (off >= size)
+		count = 0;
+	else {
+		if (off + count > size)
+			count = size - off;
+		
+		memcpy_fromio(buf, rom + off, count);
+	}
+	pci_unmap_rom(dev, rom);
+		
+	return count;
+}
+
 static struct bin_attribute pci_config_attr = {
 	.attr =	{
 		.name = "config",
@@ -186,13 +357,53 @@
 	.write = pci_write_config,
 };
 
-void pci_create_sysfs_dev_files (struct pci_dev *pdev)
+static struct bin_attribute rom_attr = {
+	.attr =	{
+		.name = "rom",
+		.mode = S_IRUSR,
+		.owner = THIS_MODULE,
+	},
+	/* .size is set individually for each device, sysfs copies it into dentry */
+	.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, try to expose it in sysfs. */
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE)) {
+		unsigned char *rom;
+		rom = pci_map_rom(pdev, &rom_attr.size);
+		if (rom) {
+			pci_unmap_rom(pdev, rom);
+			sysfs_create_bin_file(&pdev->dev.kobj, &rom_attr);
+		}
+	}
 	/* 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))
+		sysfs_remove_bin_file(&pdev->dev.kobj, &rom_attr);
+}
+
+EXPORT_SYMBOL(pci_map_rom);
+EXPORT_SYMBOL(pci_map_rom_copy);
+EXPORT_SYMBOL(pci_unmap_rom);
===== drivers/pci/pci.h 1.12 vs edited =====
--- 1.12/drivers/pci/pci.h	Fri Jun  4 09:23:04 2004
+++ edited/drivers/pci/pci.h	Tue Aug  3 17:05:19 2004
@@ -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 =====
--- 1.3/drivers/pci/remove.c	Tue Feb  3 12:17:30 2004
+++ edited/drivers/pci/remove.c	Fri Aug  6 15:36:18 2004
@@ -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 =====
--- 1.132/include/linux/pci.h	Mon Aug  2 04:00:43 2004
+++ edited/include/linux/pci.h	Fri Aug  6 00:34:27 2004
@@ -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);

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Friday, August 6, 2004 2:14 pm, Jon Smirl wrote:
> attributes after pcibios_init() then I can put the alloc call back.

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.

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

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

Thanks,
Jesse

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Friday, August 6, 2004 2:14 pm, Jon Smirl wrote:
> Please check the code out and give it some testing. It will probably
> needs some adjustment for other platforms.

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 <jbarnes@sgi.com>

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

Re: [PATCH] add PCI ROMs to sysfs

[Greg KH]

On Wed, Aug 11, 2004 at 10:04:02AM -0700, Jesse Barnes wrote:
> On Friday, August 6, 2004 2:14 pm, Jon Smirl wrote:
> > Please check the code out and give it some testing. It will probably
> > needs some adjustment for other platforms.
> 
> 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 <jbarnes@sgi.com>
> 
> to pci-sysfs.c if you do?

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

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

> Greg was a little worried that your comment
> 	/* .size is set individually for each device, sysfs copies it into dentry */
> might not be correct.

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

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Wednesday, August 11, 2004 10:28 am, Greg KH wrote:
> On Wed, Aug 11, 2004 at 10:04:02AM -0700, Jesse Barnes wrote:
> > On Friday, August 6, 2004 2:14 pm, Jon Smirl wrote:
> > > Please check the code out and give it some testing. It will probably
> > > needs some adjustment for other platforms.
> >
> > 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 <jbarnes@sgi.com>
> >
> > to pci-sysfs.c if you do?
>
> Care to send me a new patch?  Oh, and that copyright line needs to look
> like:
> * Copyright (c) 2004 Silicon Graphics, Inc. Jesse Barnes <jbarnes@sgi.com>
>
> to make it legal, or so my lawyers say :)

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

> > Greg was a little worried that your comment
> > 	/* .size is set individually for each device, sysfs copies it into
> > dentry */ might not be correct.
>
> 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.)

Ok.  Jon?

Thanks,
Jesse

Re: [PATCH] add PCI ROMs to sysfs

[Greg KH]

On Wed, Aug 11, 2004 at 11:02:10AM -0700, Jesse Barnes wrote:
> On Wednesday, August 11, 2004 10:28 am, Greg KH wrote:
> > On Wed, Aug 11, 2004 at 10:04:02AM -0700, Jesse Barnes wrote:
> > > On Friday, August 6, 2004 2:14 pm, Jon Smirl wrote:
> > > > Please check the code out and give it some testing. It will probably
> > > > needs some adjustment for other platforms.
> > >
> > > 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 <jbarnes@sgi.com>
> > >
> > > to pci-sysfs.c if you do?
> >
> > Care to send me a new patch?  Oh, and that copyright line needs to look
> > like:
> > * Copyright (c) 2004 Silicon Graphics, Inc. Jesse Barnes <jbarnes@sgi.com>
> >
> > to make it legal, or so my lawyers say :)
> 
> But I'm not the copyright holder, Silicon Graphics is, I just wanted people to 
> know who to harass if something breaks :).

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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]

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.

 
--- Greg KH <greg@kroah.com> wrote:

> On Wed, Aug 11, 2004 at 10:04:02AM -0700, Jesse Barnes wrote:
> > On Friday, August 6, 2004 2:14 pm, Jon Smirl wrote:
> > > Please check the code out and give it some testing. It will
> probably
> > > needs some adjustment for other platforms.
> > 
> > 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 <jbarnes@sgi.com>
> > 
> > to pci-sysfs.c if you do?

I'll add this

> Care to send me a new patch?  Oh, and that copyright line needs to
> look
> like:
> * Copyright (c) 2004 Silicon Graphics, Inc. Jesse Barnes
> <jbarnes@sgi.com>
> 
> to make it legal, or so my lawyers say :)
> 
> > Greg was a little worried that your comment
> > 	/* .size is set individually for each device, sysfs copies it into
> dentry */
> > might not be correct.
> 
> 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.)

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

> 
> thanks,
> 
> greg k-h
> 

=====
Jon Smirl
jonsmirl@yahoo.com


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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pci-sysfs-rom-13.patch --]
[-- Type: text/x-patch; name="pci-sysfs-rom-13.patch", Size: 11116 bytes --]

===== arch/i386/pci/fixup.c 1.19 vs edited =====
--- 1.19/arch/i386/pci/fixup.c	Thu Jun  3 10:58:17 2004
+++ edited/arch/i386/pci/fixup.c	Thu Aug  5 00:20:08 2004
@@ -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 =====
--- 1.10/drivers/pci/pci-sysfs.c	Fri Jun  4 09:23:04 2004
+++ edited/drivers/pci/pci-sysfs.c	Sat Aug  7 13:37:14 2004
@@ -5,6 +5,7 @@
  * (C) Copyright 2002-2004 IBM Corp.
  * (C) Copyright 2003 Matthew Wilcox
  * (C) Copyright 2003 Hewlett-Packard
+ * (C) Copyright 2004 Jon Smirl
  *
  * File attributes for PCI devices
  *
@@ -164,6 +165,193 @@
 	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_map_rom - map a PCI ROM to kernel space
+ * @dev: pointer to pci device struct
+ * @size: pointer to receive size of pci window over ROM
+ * @return: kernel virtual pointer to image of ROM
+ *
+ * Map a PCI ROM into kernel space. If ROM is boot video ROM,
+ * the shadow BIOS copy will be returned instead of the 
+ * actual ROM.
+ */
+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 */
+	} 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);
+	}
+	
+	rom = ioremap(start, *size);
+	if (!rom) {
+		/* restore enable if ioremap fails */
+		if (!(res->flags & (PCI_ROM_ADDRESS_ENABLE | PCI_ROM_SHADOW | PCI_ROM_COPY)))
+			pci_disable_rom(dev);
+		return NULL;
+	}		
+	/* Standard PCI ROMs start out with these three bytes 55 AA size/512 */
+	if ((*rom == 0x55) && (*(rom + 1) == 0xAA))
+		*size = *(rom + 2) * 512;	/* return true ROM size, not PCI window size */
+		
+	return rom;
+}
+
+/**
+ * pci_map_rom_copy - map a PCI ROM to kernel space, create a copy
+ * @dev: pointer to pci device struct
+ * @size: pointer to receive size of pci window over ROM
+ * @return: kernel virtual pointer to image of ROM
+ *
+ * Map a PCI ROM into kernel space. If ROM is boot video ROM,
+ * the shadow BIOS copy will be returned instead of the 
+ * actual ROM.
+ */
+unsigned char *
+pci_map_rom_copy(struct pci_dev *dev, size_t *size) {
+	struct resource *res = &dev->resource[PCI_ROM_RESOURCE];
+	unsigned char *rom;
+	
+	rom = pci_map_rom(dev, size);
+	if (!rom)
+		return NULL;
+		
+	if (res->flags & (PCI_ROM_COPY | PCI_ROM_SHADOW))
+		return rom;
+		
+	res->start = (unsigned long)kmalloc(*size, GFP_KERNEL);
+	if (!res->start) 
+		return rom;
+
+	res->end = res->start + *size; 
+	memcpy((void*)res->start, rom, *size);
+	pci_unmap_rom(dev, rom);
+	res->flags |= PCI_ROM_COPY;
+	
+	return (unsigned char *)res->start;
+}
+
+/**
+ * pci_unmap_rom - unmap the ROM from kernel space
+ * @dev: pointer to pci device struct
+ * @rom: virtual address of the previous mapping
+ *
+ * Remove a mapping of a previously mapped ROM
+ */
+void 
+pci_unmap_rom(struct pci_dev *dev, unsigned char *rom) {
+	struct resource *res = &dev->resource[PCI_ROM_RESOURCE];
+
+	if (res->flags & PCI_ROM_COPY)
+		return;
+		
+	iounmap(rom);
+		
+	/* Disable again before continuing, leave enabled if pci=rom */
+	if (!(res->flags & (PCI_ROM_ADDRESS_ENABLE | PCI_ROM_SHADOW)))
+		pci_disable_rom(dev);
+}
+
+static struct bin_attribute rom_attr;
+
+/**
+ * pci_remove_rom - disable the ROM and remove it's sysfs attribute
+ * @dev: pointer to pci device struct
+ *
+ */
+void 
+pci_remove_rom(struct pci_dev *dev) {
+	struct resource *res = &dev->resource[PCI_ROM_RESOURCE];
+	
+	if (pci_resource_len(dev, PCI_ROM_RESOURCE))
+		sysfs_remove_bin_file(&dev->dev.kobj, &rom_attr);
+	if (!(res->flags & (PCI_ROM_ADDRESS_ENABLE | PCI_ROM_SHADOW | PCI_ROM_COPY)))
+		pci_disable_rom(dev);
+}
+
+/**
+ * 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 char *rom;
+	size_t size;
+	
+	rom = pci_map_rom(dev, &size);	/* size starts out as PCI window size */
+	if (!rom)
+		return 0;
+		
+	if (off >= size)
+		count = 0;
+	else {
+		if (off + count > size)
+			count = size - off;
+		
+		memcpy_fromio(buf, rom + off, count);
+	}
+	pci_unmap_rom(dev, rom);
+		
+	return count;
+}
+
 static struct bin_attribute pci_config_attr = {
 	.attr =	{
 		.name = "config",
@@ -186,13 +374,53 @@
 	.write = pci_write_config,
 };
 
-void pci_create_sysfs_dev_files (struct pci_dev *pdev)
+static struct bin_attribute rom_attr = {
+	.attr =	{
+		.name = "rom",
+		.mode = S_IRUSR,
+		.owner = THIS_MODULE,
+	},
+	/* .size is set individually for each device, sysfs copies it into dentry */
+	.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, try to expose it in sysfs. */
+	if (pci_resource_len(pdev, PCI_ROM_RESOURCE)) {
+		unsigned char *rom;
+		rom = pci_map_rom(pdev, &rom_attr.size);
+		if (rom) {
+			pci_unmap_rom(pdev, rom);
+			sysfs_create_bin_file(&pdev->dev.kobj, &rom_attr);
+		}
+	}
 	/* 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))
+		sysfs_remove_bin_file(&pdev->dev.kobj, &rom_attr);
+}
+
+EXPORT_SYMBOL(pci_map_rom);
+EXPORT_SYMBOL(pci_map_rom_copy);
+EXPORT_SYMBOL(pci_unmap_rom);
===== drivers/pci/pci.h 1.12 vs edited =====
--- 1.12/drivers/pci/pci.h	Fri Jun  4 09:23:04 2004
+++ edited/drivers/pci/pci.h	Tue Aug  3 17:05:19 2004
@@ -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 =====
--- 1.3/drivers/pci/remove.c	Tue Feb  3 12:17:30 2004
+++ edited/drivers/pci/remove.c	Fri Aug  6 15:36:18 2004
@@ -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 =====
--- 1.132/include/linux/pci.h	Mon Aug  2 04:00:43 2004
+++ edited/include/linux/pci.h	Sat Aug  7 13:33:34 2004
@@ -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);

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Wednesday, August 11, 2004 12:24 pm, Jon Smirl wrote:
> 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.

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

Re: [PATCH] add PCI ROMs to sysfs

[Alan Cox]

On Mer, 2004-08-11 at 20:24, Jon Smirl wrote:
> 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.

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.

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

--- Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Mer, 2004-08-11 at 20:24, Jon Smirl wrote:
> > 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.
> 
> 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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

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?

--- Greg KH <greg@kroah.com> wrote:
> > Greg was a little worried that your comment
> > 	/* .size is set individually for each device, sysfs copies it into
> dentry */
> > might not be correct.
> 
> 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
> 


=====
Jon Smirl
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Marcelo Tosatti]

On Wed, Aug 11, 2004 at 11:12:36AM -0700, Greg KH wrote:
> On Wed, Aug 11, 2004 at 11:02:10AM -0700, Jesse Barnes wrote:
> > On Wednesday, August 11, 2004 10:28 am, Greg KH wrote:
> > > On Wed, Aug 11, 2004 at 10:04:02AM -0700, Jesse Barnes wrote:
> > > > On Friday, August 6, 2004 2:14 pm, Jon Smirl wrote:
> > > > > Please check the code out and give it some testing. It will probably
> > > > > needs some adjustment for other platforms.
> > > >
> > > > 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 <jbarnes@sgi.com>
> > > >
> > > > to pci-sysfs.c if you do?
> > >
> > > Care to send me a new patch?  Oh, and that copyright line needs to look
> > > like:
> > > * Copyright (c) 2004 Silicon Graphics, Inc. Jesse Barnes <jbarnes@sgi.com>
> > >
> > > to make it legal, or so my lawyers say :)
> > 
> > But I'm not the copyright holder, Silicon Graphics is, I just wanted people to 
> > know who to harass if something breaks :).
> 
> That's fine.  It's the "Copyright (c) 2004" order and exact "(c)" that
> really matters, from what I have been told to do.

Greg,

That made me curious, what is the rationale behind 

(C) Copyright 2004 Silicon Graphics, Inc. Jesse Barnes <jbarnes@sgi.com> 

works and the previous suggested

(C) Copyright 2004 Silicon Graphics, Inc. 
	Jesse Barnes <jbarnes@sgi.com> 

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!

Re: [PATCH] add PCI ROMs to sysfs

[Miles Bader]

Greg KH <greg@kroah.com> writes:
> That's fine.  It's the "Copyright (c) 2004" order and exact "(c)" that
> really matters, from what I have been told to do.

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

Re: [PATCH] add PCI ROMs to sysfs

[Greg KH]

On Wed, Aug 11, 2004 at 05:45:25PM -0700, Jon Smirl wrote:
> 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?

Adding a pointer is fine for now.

thanks,

greg k-h

Re: [PATCH] add PCI ROMs to sysfs

[Greg KH]

On Thu, Aug 12, 2004 at 11:25:27AM +0900, Miles Bader wrote:
> Greg KH <greg@kroah.com> writes:
> > That's fine.  It's the "Copyright (c) 2004" order and exact "(c)" that
> > really matters, from what I have been told to do.
> 
> Is that true now?

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

Re: [PATCH] add PCI ROMs to sysfs

[Geert Uytterhoeven]

On Wed, 11 Aug 2004, Greg KH wrote:
> On Thu, Aug 12, 2004 at 11:25:27AM +0900, Miles Bader wrote:
> > Greg KH <greg@kroah.com> writes:
> > > That's fine.  It's the "Copyright (c) 2004" order and exact "(c)" that
> > > really matters, from what I have been told to do.
> >
> > Is that true now?
>
> 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 :)

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 -- geert@linux-m68k.org

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

Re: [PATCH] add PCI ROMs to sysfs

[Alan Cox]

On Iau, 2004-08-12 at 00:31, Jon Smirl wrote:
> 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.

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.

> 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.

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

> 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.

It would be better to just to blacklist those corner cases

Re: [PATCH] add PCI ROMs to sysfs

[Jesse Barnes]

On Wednesday, August 11, 2004 6:28 pm, Marcelo Tosatti wrote:
> That made me curious, what is the rationale behind
>
> (C) Copyright 2004 Silicon Graphics, Inc. Jesse Barnes <jbarnes@sgi.com>
>
> works and the previous suggested
>
> (C) Copyright 2004 Silicon Graphics, Inc.
> 	Jesse Barnes <jbarnes@sgi.com>
>
> 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.

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

Re: [PATCH] add PCI ROMs to sysfs

[Greg KH]

On Wed, Aug 11, 2004 at 10:28:43PM -0300, Marcelo Tosatti wrote:
> > > > > 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 <jbarnes@sgi.com>
> > > > >
> > > > > to pci-sysfs.c if you do?
> > > >
> > > > Care to send me a new patch?  Oh, and that copyright line needs to look
> > > > like:
> > > > * Copyright (c) 2004 Silicon Graphics, Inc. Jesse Barnes <jbarnes@sgi.com>
> > > >
> > > > to make it legal, or so my lawyers say :)
> > > 
> > > But I'm not the copyright holder, Silicon Graphics is, I just wanted people to 
> > > know who to harass if something breaks :).
> > 
> > That's fine.  It's the "Copyright (c) 2004" order and exact "(c)" that
> > really matters, from what I have been told to do.
> 
> Greg,
> 
> That made me curious, what is the rationale behind 
> 
> (C) Copyright 2004 Silicon Graphics, Inc. Jesse Barnes <jbarnes@sgi.com> 
> 
> works and the previous suggested
> 
> (C) Copyright 2004 Silicon Graphics, Inc. 
> 	Jesse Barnes <jbarnes@sgi.com> 
> 
> 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. 

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

Re: [PATCH] add PCI ROMs to sysfs

[Jon Smirl]

--- Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Iau, 2004-08-12 at 00:31, Jon Smirl wrote:
> > 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.
> 
> 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.

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
jonsmirl@yahoo.com


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

Re: [PATCH] add PCI ROMs to sysfs

[Matthew Wilcox]

On Wed, Aug 11, 2004 at 09:38:18PM -0700, Greg KH wrote:
> 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 :)

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