Re: [PATCH] add PCI ROMs to sysfs

[Matthew Wilcox]

On Thu, Aug 26, 2004 at 08:40:49AM -0700, Jon Smirl wrote:
> I was computing the length because of the need to copy for the
> partially decoded case. I have one card that maps a 40KB ROM with a
> 512KB window. In that case it makes a 450KB different in the copy size.

Youch.  That card's actually breaking the PCI spec, but never mind, it exists.

> The copy case is adding a lot of complexity to the code. We could just
> give it the boot and require that partially decoded drivers just turn
> the ROM attribute off.
> 
> If not, how do I get the size from your algorithm?

In words, what the algorithm is doing is:

	for_each_rom_image
		get_the_pci_data_structure
		get_the_rom_length_from_pds

At the end of the algorithm, 'image' should be pointing to the end of the
ROM data.

I'm just tracking down another bug with the current code -- it maps the
ROMs into the wrong address range on my ia64 zx1 box.  I'll send more
mail once I've fixed that.

> --- Matthew Wilcox <willy@xxxxxxxxxx> wrote:
> 
> > On Wed, Aug 25, 2004 at 01:06:38PM -0700, Jon Smirl wrote:
> > > New version attached fixes this and the function documentation.
> > 
> > Sorry, I found two more bugs ;-(
> > 
> > > +unsigned char *
> > > +pci_map_rom(struct pci_dev *pdev, size_t *size)
> > > +{
> > [...]
> > > +	/* 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 */
> > 
> > First, you're dereferencing an ioremapped pointer directly instead of
> > using readb() et al.  This is a portability no-no but happens to work
> > on x86.
> > 
> > Second, only images of code type 0 (Intel x86, PC-AT compatible) are
> > specified to have length as the third byte.  OpenFirmware, PA-RISC
> > and
> > EFI may or may not have size as the third byte.  It's also possible
> > that there are multiple images in the ROM.  So what you have to do to
> > be correct is something like ...
> > 
> > 	int last_image;
> > 	char *image = rom;
> > 	do {
> > 		char *pds;
> > 		if (readb(image) != 0x55)
> > 			break;
> > 		if (readb(image + 1) != 0xAA)
> > 			break;
> > 		pds = image + readw(image + 16);
> > 		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;
> > 		image += readw(pds + 16) * 512;
> > 	} while (!last_image);
> > 
> > But I'm not sure it's worth it.  I'm inclined to just map the whole
> > thing.
> > 
> > -- 
> > "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
> > 
> 
> =====
> Jon Smirl
> jonsmirl@xxxxxxxxx
> 
> 
> 		
> __________________________________
> Do you Yahoo!?
> Yahoo! Mail is new and improved - Check it out!
> http://promotions.yahoo.com/new_mail
> 

-- 
"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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/