Re: [PATCH v6 1/2] bus/pci: add function to enable/disable PASID

[Jerin Jacob <jerinjacobk@gmail.com>]

On Sat, Nov 4, 2023 at 7:31 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Nov 03, 2023 at 01:29:32PM -0500, Abdullah Sevincer wrote:
> > This commit implements an internal api to enable and disable PASID for
> > a device e.g. device driver event/dlb2.
> >
> > For kernels when PASID enabled by default it breaks DLB functionality,
> > hence disabling PASID is required for DLB to function properly.
> >
> > PASID capability is not exposed to users hence offset can not be
> > retrieved by rte_pci_find_ext_capability() api. Therefore, api
> > implemented in this commit accepts an offset for PASID with an enable
> > flag which is used to enable/disable PASID.
> >
> > Signed-off-by: Abdullah Sevincer <abdullah.sevincer@intel.com>
> > ---
> >  drivers/bus/pci/pci_common.c  |  7 +++++++
> >  drivers/bus/pci/rte_bus_pci.h | 13 +++++++++++++
> >  drivers/bus/pci/version.map   |  1 +
> >  lib/pci/rte_pci.h             |  4 ++++
> >  4 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> > index 921d957bf6..5aac2406f1 100644
> > --- a/drivers/bus/pci/pci_common.c
> > +++ b/drivers/bus/pci/pci_common.c
> > @@ -938,6 +938,13 @@ rte_pci_set_bus_master(const struct rte_pci_device *dev, bool enable)
> >       return 0;
> >  }
> >
> > +int
> > +rte_pci_pasid_ena_dis(const struct rte_pci_device *dev, off_t offset, bool enable)
>
> While I realise we are now at v6 of this patchset, and the name was
> suggested on v4, seeing it implemented I'm afraid I think
> rte_pci_pasid_ena_dis is not a great name! I also agree that the pasid_set
> name was a bit misleading too, leaving us with a naming problem.
> I have two suggestions:
>
> * if we want to keep one function - "rte_pci_pasid_set_state", which makes
>   it clear we are not setting the pasid, but the pasid state.
> * separate this explicitly into rte_pci_pasid_enable() and
>   rte_pci_pasid_disable() functions. This is the cleanest solution but it
>   doesn't align with some of the other functions in pci lib which set the
>   state.
>
> Jerin, any further thoughts? and sorry for late feedback.

Yes. Above two functions are better than ena_dis().

Other option could be, rte_pci_pasid_ctrl() which is aligned with PCI
register name.

No strong opinion for the name. Feel free to pick one from above three options.


>
> /Bruce