Re: [PATCH] pci-arbiter: Remove embedded pciaccess code

[Samuel Thibault]

Hello,

Thanks for your work?

Could you try to split your changes in separate patches? Otherwise the
review is harder since the reviewer has to work out which pieces are
intended to what part of the changes, and applying the parts that look
ok while other parts are getting repolished is harder since the reviewer
might miss pieces.

I don't remember: can we apply this patch in Debian Hurd already? (i.e.
does the libpciaccess there (possibly in unreleased) work fine enough
for our needs?)

Joan Lledó via Bug reports for the GNU Hurd, le dim. 27 oct. 2019 08:48:19 
+0100, a ecrit:
> -      err = op (dev->bus, dev->dev, dev->func, offset, data, 4);
> +      if (op.read)
> +     err = op.io_op.read (dev, data, offset, 4, &actual);
> +      else
> +     err = op.io_op.write (dev, data, offset, 4, &actual);

This is a bit unfortunate.

> these two libpciaccess functions have different prototypes, we cannot
> use one single pointer to function like we did until now, so I added
> some changes to use always the proper prototype.

This looks a bit complex, I would say we should rather just cast:

typedef int (*pci_op_t) (struct pci_device * dev, void *data,
                         pciaddr_t reg, pciaddr_t width,
                         pciaddr_t * bytes);

        io_config_file (node->nn->ln->device, offset, len, data,
-                       pci_sys->write);
+                       (pci_op_t) pci_sys->write);

Because it is completely compatible to pass a void* to a function that
takes a const void*. That will simplify the whole thing a lot.

> except the change in S_pci_get_dev_rom() in pci-ops.c.
> I don't know what's this change for, but if we're always setting base_addr
> to 0, then probably we don't need that field anymore and can remove it from
> struct pci_xrom_bar.

> @@ -260,7 +260,7 @@ S_pci_get_dev_rom (struct protid * master, char **data, 
> size_t * datalen)
>      }
>  
>    /* Copy the regions info */
> -  rom.base_addr = e->device->rom_base;
> +  rom.base_addr = 0; // pci_device_private only
>    rom.size = e->device->rom_size;
>    memcpy (*data, &rom, size);

The change is because rom_base is a private field in libpciaccess, we'd
need to get libpciaccess to expose it so we can properly take it into
account. For the time being, putting FIXME here would be enough.


The rest looks good, but since I'm not sure which parts can be applied
independently of the rest (some buffering issues for instance, I guess),
I prefer not to commit anything yet.

Samuel