On Tue, Sep 19, 2023 at 4:19 AM Xia, Chenbo <chenbo....@intel.com> wrote:
>
> Hi David,
>
> > -----Original Message-----
> > From: David Marchand <david.march...@redhat.com>
> > Sent: Thursday, September 14, 2023 8:29 PM
> > To: Xia, Chenbo <chenbo....@intel.com>; Maxime Coquelin
> > <maxime.coque...@redhat.com>
> > Cc: dev@dpdk.org; tho...@monjalon.net; ferruh.yi...@amd.com;
> > nipun.gu...@amd.com; Richardson, Bruce <bruce.richard...@intel.com>;
> > Burakov, Anatoly <anatoly.bura...@intel.com>; Jay Zhou
> > <jianjay.z...@huawei.com>; McDaniel, Timothy <timothy.mcdan...@intel.com>;
> > Julien Aube <julien_d...@jaube.fr>; Rahul Lakkireddy
> > <rahul.lakkire...@chelsio.com>; Guo, Junfeng <junfeng....@intel.com>;
> > Jeroen de Borst <jeroe...@google.com>; Rushil Gupta <rush...@google.com>;
> > Joshua Washington <joshw...@google.com>; Dongdong Liu
> > <liudongdo...@huawei.com>; Yisen Zhuang <yisen.zhu...@huawei.com>; Gaetan
> > Rivet <gr...@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....@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.march...@redhat.com>
> > > > Acked-by: Bruce Richardson <bruce.richard...@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, ®, 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
