libpciaccess x86 backend

[Alexander E. Patrakov]

[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