Hello, Alexander E. Patrakov, le Sun 17 Jan 2010 21:42:38 +0500, a écrit : > [If this review is stupid, disregard - I know nothing about PCI internals]
Thanks for your review, it's wasn't stupid at all, it's always good to make sure even cases that are not supposed to happen are handled. I'll send a fixed patch. > 17.01.2010 19:37, Samuel Thibault wrote: > >+ sav = inl(0xCF8); > >+ outl(0x80000000 | (bus<< 16) | (dev<< 11) | (func<< 8) | (reg& > >~3), 0xCF8); > >+ 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? The I/O is harmless, but it's better to not even do it indeed. > >+ printf("unknown header type %02x\n", header_type); > > printf to stdout in a library? Shouldn't that better go to stderr? Indeed. > >+static int > >+get_test_val_size( uint32_t testval ) > > I understand that nobody is going to pass 0x80000000 here as testval, > but, should this and the return value be unsigned nevertheless? Why not indeed. > >+static int > >+pci_device_x86_read(struct pci_device *dev, void *data, > > Can't it happen that toread == 3 here? Or is this an invalid request? It's not really supposed to happen, but it possibly could indeed. > >+ if (ret) > >+ return ret; > > and stay with io enabled and pci_sys_x86 being non-freed? Oops. > >+ 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)? Yes, actually they are the same. Samuel _______________________________________________ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg