[PATCH v2 04/15] bus/pci: find PCI capability

[David Marchand]

On Tue, Sep 19, 2023 at 4:19 AM Xia, Chenbo <chenbo.xia at intel.com> wrote:
>
> Hi David,
>
> > -----Original Message-----
> > From: David Marchand <david.marchand at redhat.com>
> > Sent: Thursday, September 14, 2023 8:29 PM
> > To: Xia, Chenbo <chenbo.xia at intel.com>; Maxime Coquelin
> > <maxime.coquelin at redhat.com>
> > Cc: dev at dpdk.org; thomas at monjalon.net; ferruh.yigit at amd.com;
> > nipun.gupta at amd.com; Richardson, Bruce <bruce.richardson at intel.com>;
> > Burakov, Anatoly <anatoly.burakov at intel.com>; Jay Zhou
> > <jianjay.zhou at huawei.com>; McDaniel, Timothy <timothy.mcdaniel at intel.com>;
> > Julien Aube <julien_dpdk at jaube.fr>; Rahul Lakkireddy
> > <rahul.lakkireddy at chelsio.com>; Guo, Junfeng <junfeng.guo at intel.com>;
> > Jeroen de Borst <jeroendb at google.com>; Rushil Gupta <rushilg at google.com>;
> > Joshua Washington <joshwash at google.com>; Dongdong Liu
> > <liudongdong3 at huawei.com>; Yisen Zhuang <yisen.zhuang at huawei.com>; Gaetan
> > Rivet <grive at u256.net>
> > Subject: Re: [PATCH v2 04/15] bus/pci: find PCI capability
> >
> > Hello Chenbo,
> >
> > On Thu, Sep 7, 2023 at 2:43 PM Xia, Chenbo <chenbo.xia at intel.com> wrote:
> > > > Introduce two helpers so that drivers stop reinventing the wheel when
> > it
> > > > comes to finding capabilities in a device PCI configuration space.
> > > > Use it in existing drivers.
> > > >
> > > > Note:
> > > > - base/ drivers code is left untouched, only some wrappers in cxgbe
> > > >   are touched,
> > > > - bnx2x maintained a per device cache of capabilities, this code has
> > been
> > > >   reworked to only cache the capabilities used in this driver,
> > > >
> > > > Signed-off-by: David Marchand <david.marchand at redhat.com>
> > > > Acked-by: Bruce Richardson <bruce.richardson at intel.com>
> > > > ---
> > > > Changes since v1:
> > > > - updated commitlog,
> > > > - separated VFIO changes for using standard PCI helper in a separate
> > > >   patch,
> > > > - marked new experimental symbols with current version,
> > > > - reordered defines in rte_pci.h,
> > > >
> > > > ---
> > > >  drivers/bus/pci/linux/pci_vfio.c   |  74 ++++--------------
> > > >  drivers/bus/pci/pci_common.c       |  45 +++++++++++
> > > >  drivers/bus/pci/rte_bus_pci.h      |  31 ++++++++
> > > >  drivers/bus/pci/version.map        |   4 +
> > > >  drivers/crypto/virtio/virtio_pci.c |  57 +++++---------
> > > >  drivers/event/dlb2/pf/dlb2_main.c  |  42 +---------
> > > >  drivers/net/bnx2x/bnx2x.c          |  41 +++++-----
> > > >  drivers/net/cxgbe/base/adapter.h   |  28 +------
> > > >  drivers/net/gve/gve_ethdev.c       |  46 ++---------
> > > >  drivers/net/gve/gve_ethdev.h       |   4 -
> > > >  drivers/net/hns3/hns3_ethdev_vf.c  |  79 +++----------------
> > > >  drivers/net/virtio/virtio_pci.c    | 121 +++++-----------------------
> > -
> > > >  lib/pci/rte_pci.h                  |  11 +++
> > > >  13 files changed, 186 insertions(+), 397 deletions(-)
> > > >
> > > > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > > > b/drivers/bus/pci/linux/pci_vfio.c
> > > > index 958f8b3b52..614ed5d696 100644
> > > > --- a/drivers/bus/pci/linux/pci_vfio.c
> > > > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > > > @@ -110,74 +110,34 @@ static int
> > > >  pci_vfio_get_msix_bar(const struct rte_pci_device *dev,
> > > >       struct pci_msix_table *msix_table)
> > > >  {
> > > > -     int ret;
> > > > -     uint32_t reg;
> > > > -     uint16_t flags;
> > > > -     uint8_t cap_id, cap_offset;
> > > > +     off_t cap_offset;
> > > >
> > > > -     /* read PCI capability pointer from config space */
> > > > -     ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> > > > PCI_CAPABILITY_LIST);
> > > > -     if (ret != sizeof(reg)) {
> > > > -             RTE_LOG(ERR, EAL,
> > > > -                     "Cannot read capability pointer from PCI config
> > > > space!\n");
> > > > +     cap_offset = rte_pci_find_capability(dev, PCI_CAP_ID_MSIX);
> > >
> > > I notice in some cases we use rte_pci_has_capability_list() to check
> > first,
> > > then looking for specific cap, in other cases we don't use
> > > rte_pci_has_capability_list(). Since we define this API, should we
> > always do
> > > the check?
> >
> > Hum, I am not sure of what you suggest here.
> >
> > I tried to do a 1:1 conversion of what existed.
> > Do you mean there are places where I missed converting some
> > rte_pci_read_config(PCI_CAPABILITY_LIST) to
> > rte_pci_has_capability_list()?
> > If so, could you point at them?
> >
> > Or are you suggesting to have this check as part of
> > rte_pci_find_capability() ?
>
> Your conversion is correct. I was saying about the usage of
> rte_pci_has_capability_list/rte_pci_find_capability, logically should we
> always call rte_pci_has_capability_list first before find capability?
>
> I don't have strong opinion on this. You can choose to keep the same or
> call rte_pci_has_capability_list more.

I prefer to keep it as is (the series is already big enough).
We can still add more checks in the future.

Thanks Chenbo.


-- 
David Marchand