Re: [dpdk-dev] [PATCH 1/4] eal: provide functions to access PCI config

[David Marchand <david.marchand@6wind.com>]

On Wed, Jun 17, 2015 at 1:51 AM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> From: Stephen Hemminger <shemming@brocade.com>
>
> Some drivers need ability to access PCI config (for example for power
> management). This adds an abstraction to do this for both Linux
> and BSD.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_eal/bsdapp/eal/eal_pci.c             | 83
> +++++++++++++++++++++++++
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  2 +
>  lib/librte_eal/common/include/rte_pci.h         | 28 +++++++++
>  lib/librte_eal/linuxapp/eal/eal_pci.c           | 50 +++++++++++++++
>  lib/librte_eal/linuxapp/eal/eal_pci_init.h      | 11 ++++
>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c       | 14 +++++
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c      | 16 +++++
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  2 +
>  8 files changed, 206 insertions(+)
>
> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c
> b/lib/librte_eal/bsdapp/eal/eal_pci.c
> index 61e8921..bf6bc93 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
> @@ -490,6 +490,89 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver
> *dr, struct rte_pci_device *d
>         return 1;
>  }
> [snip]
> +
> +       if (len == 3 || len > sizeof(pi.pi_data)) {
> +               RTE_LOG(ERR, EAL, "%s(): invalid pci read length\n",
> __func__);
> +               goto error;
> +       }
>

A comment on length 3 would help to understand why it is refused.


diff --git a/lib/librte_eal/common/include/rte_pci.h
> b/lib/librte_eal/common/include/rte_pci.h
> index b4f38bc..62fb0ea 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -394,6 +394,34 @@ void rte_eal_pci_register(struct rte_pci_driver
> *driver);
>   */
>  void rte_eal_pci_unregister(struct rte_pci_driver *driver);
>
> +/**
> + * Read PCI config space.
> + *
> + * @param device
> + *   A pointer to a rte_pci_device structure describing the device
> + *   to use
> + * @param buf
> + *   A data buffer where the bytes should be read into
> + * @param size
> + *   The length of the data buffer.
> + */
> +int rte_eal_pci_read_config(const struct rte_pci_device *device,
> +                           void *buf, size_t len, off_t offset);
> +
>

Parameters do not match the description.


> +/**
> + * Write PCI config space.
> + *
> + * @param device
> + *   A pointer to a rte_pci_device structure describing the device
> + *   to use
> + * @param buf
> + *   A data buffer containing the bytes should be written
> + * @param size
> + *   The length of the data buffer.
> + */
> +int rte_eal_pci_write_config(const struct rte_pci_device *device,
> +                            const void *buf, size_t len, off_t offset);
> +
>

Idem.



The rest looks good to me.


-- 
David Marchand