2005-02-17 00:02:42

by Jesse Barnes

[permalink] [raw]
Subject: pci_map_rom bug?

Jon, it looks like the loop at the bottom of pci_map_rom is busted?

do {
void __iomem *pds;
/* Standard PCI ROMs start out with these bytes 55 AA */
if (readb(image) != 0x55)
break;
if (readb(image + 1) != 0xAA)
break;
/* get the PCI data structure and check its signature */
pds = image + readw(image + 24);
if (readb(pds) != 'P')
break;
if (readb(pds + 1) != 'C')
break;
if (readb(pds + 2) != 'I')
break;
if (readb(pds + 3) != 'R')
break;
last_image = readb(pds + 21) & 0x80;
/* this length is reliable */
image += readw(pds + 16) * 512;
} while (!last_image);

It looks like it's trying to verify all the ROMs on a given PCI device rather
than just the one we just ioremap'd above. Should this check just be inline
and the loop deleted? In that case, all of the breaks would turn into return
NULLs (though the code should probably be refactored to make that a little
clearer) along with an iounmap?

Thanks,
Jesse


2005-02-17 16:29:58

by [email protected]

[permalink] [raw]
Subject: Re: pci_map_rom bug?

On Wed, 16 Feb 2005 16:00:47 -0800, Jesse Barnes <[email protected]> wrote:
> It looks like it's trying to verify all the ROMs on a given PCI device rather
> than just the one we just ioremap'd above. Should this check just be inline
> and the loop deleted? In that case, all of the breaks would turn into return
> NULLs (though the code should probably be refactored to make that a little
> clearer) along with an iounmap?

The ROM experts on linux-pci supplied that code. It is legal to have
multiple images in a ROM for different formats, x86, OpenFirmware,
proprietary. The loop is adding all of the images together. Above we
IO remapped the entire PCI window which may be 64K and it contained
two images each at 20K. The extra loop returns the smaller size. This
lets the copy_rom case allocate 40K of memory instead of 64K.

--
Jon Smirl
[email protected]

2005-02-17 17:15:15

by Jesse Barnes

[permalink] [raw]
Subject: Re: pci_map_rom bug?

On Thursday, February 17, 2005 8:29 am, Jon Smirl wrote:
> On Wed, 16 Feb 2005 16:00:47 -0800, Jesse Barnes <[email protected]> wrote:
> > It looks like it's trying to verify all the ROMs on a given PCI device
> > rather than just the one we just ioremap'd above. Should this check just
> > be inline and the loop deleted? In that case, all of the breaks would
> > turn into return NULLs (though the code should probably be refactored to
> > make that a little clearer) along with an iounmap?
>
> The ROM experts on linux-pci supplied that code. It is legal to have
> multiple images in a ROM for different formats, x86, OpenFirmware,
> proprietary. The loop is adding all of the images together. Above we
> IO remapped the entire PCI window which may be 64K and it contained
> two images each at 20K. The extra loop returns the smaller size. This
> lets the copy_rom case allocate 40K of memory instead of 64K.

Ah, ok, but we still have the situation that cause me to post the cleanup
patch in the first place--pci_map_rom succeeds, but the first two bytes
aren't 0x55aa but 0x0303... Any ideas?

Jesse

2005-02-17 17:33:46

by [email protected]

[permalink] [raw]
Subject: Re: pci_map_rom bug?

On Thu, 17 Feb 2005 09:14:04 -0800, Jesse Barnes <[email protected]> wrote:
> Ah, ok, but we still have the situation that cause me to post the cleanup
> patch in the first place--pci_map_rom succeeds, but the first two bytes
> aren't 0x55aa but 0x0303... Any ideas?

Check for returned size of zero and ignore the ROM?

>
> Jesse
>


--
Jon Smirl
[email protected]