On Mon, 2009-01-12 at 17:25 +1100, Timothy S. Nelson wrote:
> Hi all. I've attached a patch that combines a previously reviewed
> patch with some extra warning messages, so that people who have the same
> problem that Xorg was having with ROM reading will maybe be able to solve it
> more quickly.
>
> However, I have a feeling I might've missed something. Can people
> have a look at this?
Sure, I'm procrastinating =D
For starters you need to "sign-off" your patch. If you don't know what
that means look at Documentation/SubmittingPatches.
> It seems from the FAQ that the appropriate next step is to get "the
> maintainer" to ok my patch. Does anyone know who that is for PCI? Or is
> there a different way to do things for PCI?
Jesse is the PCI maintainer, as listed in MAINTAINERS under "PCI
SUBSYSTEM", usually you'd CC him, but he'll see this on the list.
Most people like it if you post patches inline, I had to copy/paste to
get yours below.
I'm no authority on the code you're patching, but still I'm not sure I
like the way you're doing it.
> diff -urN linux-2.6.27/drivers/pci/pci-sysfs.c linux-2.6.27/drivers/pci/pci-sysfs.c
> --- linux-2.6.27/drivers/pci/pci-sysfs.c 2008-10-10 09:13:53.000000000 +1100
> +++ linux-2.6.27/drivers/pci/pci-sysfs.c 2009-01-12 10:29:34.000000000 +1100
> @@ -694,8 +694,8 @@
> return -EINVAL;
>
> rom = pci_map_rom(pdev, &size); /* size starts out as PCI window size */
> - if (!rom)
> - return 0;
> + if (!rom || !size)
> + return -EIO;
>
> if (off >= size)
> count = 0;
> diff -urN linux-2.6.27/drivers/pci/rom.c linux-2.6.27/drivers/pci/rom.c
> --- linux-2.6.27/drivers/pci/rom.c 2008-10-10 09:13:53.000000000 +1100
> +++ linux-2.6.27/drivers/pci/rom.c 2009-01-12 17:06:58.000000000 +1100
> @@ -72,8 +72,12 @@
> do {
> void __iomem *pds;
> /* Standard PCI ROMs start out with these bytes 55 AA */
> - if (readb(image) != 0x55)
> + if (readb(image) != 0x55) {
> + printk(KERN_INFO " %s: Invalid ROM contents\n", __FUNCTION__);
> + printk(KERN_INFO " %s: suggest echoing 1 into /sys/bus/pci/<deviceID>/enable\n", __FUNCTION__);
> + printk(KERN_INFO " %s: to enable device before trying to do rom reading\n", __FUNCTION__);
> break;
> + }
I think any checking like this should be done in pci-sysfs.c, for
several reasons. Firstly someone might not have sysfs enabled, in which
case printing a message about sysfs is a bit silly - even though at the
moment it looks like the sysfs code is the only caller.
Secondly if you do the check at a higher level, where you have the
pci_dev, you can actually check if the device is enabled, and so print a
definitive message. eg. "The device _is_ disabled, if you want to read
the ROM you need to enable it".
And as far as the message, is the content really invalid? The comment
says "standard PCI ROMs" start with "55 AA", which makes it sound like
that's a normal value to find.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
Attachment:
signature.asc
Description: This is a digitally signed message part
