libpciaccess x86 backend

[Samuel Thibault]

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