Re: [PATCH] add PCI ROMs to sysfs

["Petr Vandrovec" <VANDROVE@vc.cvut.cz>]

On  3 Aug 04 at 23:08, Jon Smirl wrote:

> +   unsigned long start;
> +   loff_t i, size;
> +   char direct_access = dev->rom_info.rom ? 0 : 1;
> +   unsigned char *rom = NULL;

What about using 'rom' variable for non-direct access too?

      unsigned char *rom = dev->rom_info.rom;
      char direct_access = rom == NULL;
      
> +   struct resource *r = &dev->resource[PCI_ROM_RESOURCE];
> +
> +   if (off > size)

You can do  'if (off >= size) ...' And well, where is size assigned at
all?

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

Test for failure? Test for no ROM devices?

> +       
> +       printk("read_rom start %lx size %x\n", start, size);
> +       printk("rom bytes %02x %02x\n", rom, rom + 1);

readb(rom), readb(rom+1)?

> +   }
> +   if (off + count > size) {

Is off + count guaranteed to not overflow?

> +       size -= off;
> +       count = size;
> +   } else
> +       size = count;
> +
> +   i = 0;
> +   while (size > 0) {
> +       unsigned char val;
> +       if (direct_access)
> +           val = readb(rom + off);

Please read it with readl. At least on my Matrox G550 reading 64KB ROM with
byte accesses takes 1334ms, with 16bit accesses 840ms and with
32bit (or 64bit MMX) accesses 551ms. Straight (non-IO aware) memcpy 
takes 535ms. And put some conditional_schedule()s here, 550ms (or even
34ms for 4KB chunk) is IMHO too long.

> +       else
> +           val = *(dev->rom_info.rom + off);

If you made changes suggested at the beginning, you can do this instead:

              val = rom[off];

                                                Best regards,
                                                    Petr Vandrovec