[If this review is stupid, disregard - I know nothing about PCI internals] 17.01.2010 19:37, Samuel Thibault wrote: > + sav = inl(0xCF8); > + outl(0x80000000 | (bus<< 16) | (dev<< 11) | (func<< 8) | (reg& ~3), > 0xCF8); > + /* NOTE: x86 is already LE */ > + switch (size) { ... > + default: > + ret = EIO; > + break; > + } > + outl(sav, 0xCF8);
Here a read or write request that can be rejected right away (wrong size) still leads to I/O port access. Is it OK? > + printf("unknown header type %02x\n", header_type); printf to stdout in a library? Shouldn't that better go to stderr? (btw, there is another case in src/common_capability.c) > +/** Returns the size of a region based on the all-ones test value */ > +static int > +get_test_val_size( uint32_t testval ) > +{ > + int size = 1; I understand that nobody is going to pass 0x80000000 here as testval, but, should this and the return value be unsigned nevertheless? > +static int > +pci_device_x86_read(struct pci_device *dev, void *data, > + pciaddr_t offset, pciaddr_t size, pciaddr_t *bytes_read) > +{ > + struct pci_system_x86 *pci_sys_x86 = (struct pci_system_x86 *) pci_sys; > + int err; > + > + *bytes_read = 0; > + while (size> 0) { > + int toread = 4; > + if (toread> size) > + toread = size; > + if (toread> 4 - (offset& 0x3)) > + toread = 4 - (offset& 0x3); Can't it happen that toread == 3 here? Or is this an invalid request? > +_pci_hidden int > +pci_system_x86_create(void) > +{ > + struct pci_device_private *device; > + int ret, bus, dev, ndevs, func, nfuncs; > + struct pci_system_x86 *pci_sys_x86; > + uint32_t reg; > + > + ret = x86_enable_io(); > + if (ret) > + return ret; > + > + pci_sys_x86 = calloc(1, sizeof(struct pci_system_x86)); > + if (pci_sys_x86 == NULL) { > + x86_disable_io(); > + return ENOMEM; > + } > + pci_sys =&pci_sys_x86->system; > + > + ret = pci_probe(pci_sys_x86); > + if (ret) > + return ret; and stay with io enabled and pci_sys_x86 being non-freed? > + pci_sys->devices = calloc(ndevs, sizeof(struct pci_device_private)); > + if (pci_sys->devices == NULL) { > + x86_disable_io(); > + free(pci_sys); Is this supposed to be free(pci_sys_x86)? -- Alexander E. Patrakov _______________________________________________ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg