Re: [PATCH] - hurd pci-arbiter: remove embedded pciaccess code

[Samuel Thibault]

Hello,

Thanks for this :)

Damien Zammit, le sam. 10 nov. 2018 18:58:35 +1100, a ecrit:
> @@ -35,6 +35,8 @@ include ../Makeconf
>  
>  CFLAGS += -I$(PORTDIR)/include
>  
> +CPPFLAGS += -imacros $(srcdir)/config.h

Why is this needed?

(that's the reason why you have to add a config.h file, but its content
is completely useless).

> @@ -85,10 +86,10 @@ io_config_file (struct pci_device * dev, off_t offset, 
> size_t * len,
>    assert_backtrace (dev != 0);
>  
>    /* Don't exceed the config space size */
> -  if (offset > dev->config_size)
> +  if (offset > PCI_CONFIG_SIZE)
>      return EINVAL;

> --- a/pci-arbiter/pcifs.h
> +++ b/pci-arbiter/pcifs.h
> @@ -27,8 +27,11 @@
> +// FIXME: Hardcoded PCI config size
> +#define PCI_CONFIG_SIZE 256

So it seems that libpciaccess doesn't provide an interface for this.
Please leave as TODO in config_block_op to just return what could be
read once op() returns EIO, so that we do get the short reads
implemented by just leaving it up to libpciaccess. Limiting to 256 bytes
will be fine for now.

> @@ -85,7 +85,7 @@ S_pci_conf_read (struct protid * master, int reg, char 
> **data,
>    error_t err;
>    pthread_mutex_t *lock;
>    struct pcifs_dirent *e;
> -
> +  
>    if (!master)
>      return EOPNOTSUPP;
>  

Please avoid such unneeded hunk :)

> @@ -266,7 +276,7 @@ create_fs_tree (struct pcifs * fs, struct pci_system * 
> pci_sys)
>        e_stat = func_parent->stat;
>        e_stat.st_mode &= ~(S_IFDIR | S_IXUSR | S_IXGRP);
>        e_stat.st_mode |= S_IFREG | S_IWUSR | S_IWGRP;
> -      e_stat.st_size = device->config_size;
> +      e_stat.st_size = PCI_CONFIG_SIZE; // FIXME: Hardcoded

Mmm. This will also be a TODO. I'd say on the long run libpciaccess
should really provide a way to get the config size instead of having to
just get EIO.

Samuel