Re: [PATCH v8 net-next 10/14] pds_core: add auxiliary_bus devices

[Leon Romanovsky <leon@kernel.org>]

On Sat, Apr 01, 2023 at 01:15:03PM -0700, Shannon Nelson wrote:
> On 4/1/23 11:27 AM, Leon Romanovsky wrote:
> > 
> > On Thu, Mar 30, 2023 at 04:46:24PM -0700, Shannon Nelson wrote:
> > > An auxiliary_bus device is created for each vDPA type VF at VF probe
> > > and destroyed at VF remove.  The VFs are always removed on PF remove, so
> > > there should be no issues with VFs trying to access missing PF structures.
> > > 
> > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> > > ---
> > >   drivers/net/ethernet/amd/pds_core/Makefile |   1 +
> > >   drivers/net/ethernet/amd/pds_core/auxbus.c | 142 +++++++++++++++++++++
> > >   drivers/net/ethernet/amd/pds_core/core.h   |   6 +
> > >   drivers/net/ethernet/amd/pds_core/main.c   |  36 +++++-
> > >   include/linux/pds/pds_auxbus.h             |  16 +++
> > >   include/linux/pds/pds_common.h             |   1 +
> > >   6 files changed, 200 insertions(+), 2 deletions(-)
> > >   create mode 100644 drivers/net/ethernet/amd/pds_core/auxbus.c
> > >   create mode 100644 include/linux/pds/pds_auxbus.h
> > 
> > I feel that this auxbus usage is still not correct.
> > 
> > The idea of auxiliary devices is to partition physical device (for
> > example PCI device) to different sub-devices, where every sub-device
> > belongs to different sub-system. It is not intended to create per-VF
> > devices.
> > 
> > In your case, you should create XXX vDPA auxiliary devices which are
> > connected in one-to-one scheme to their PCI VF counterpart.
> 
> I don't understand - first I read
>     "It is not intended to create per-VF devices"
> and then
>     "you should create XXX vDPA auxiliary devices which are
>     connected in one-to-one scheme to their PCI VF counterpart."
> These seem at first to be directly contradictory statements, so maybe I'm
> missing some nuance.

It is not, as I'm looking in the code and don't expect to see the code
like this. It gives me a sense that auxdevice is not created properly
as nothing shouldn't be happen from these checks.

+	if (pf->state) {
+		dev_warn(vf->dev, "%s: PF in a transition state (%lu)\n",
+			 __func__, pf->state);
+		err = -EBUSY;
+	} else if (!pf->vfs) {
+		dev_warn(vf->dev, "%s: PF vfs array not ready\n",
+			 __func__);
+		err = -ENOTTY;
+	} else if (vf->vf_id >= pf->num_vfs) {
+		dev_warn(vf->dev, "%s: vfid %d out of range\n",
+			 __func__, vf->vf_id);
+		err = -ERANGE;
+	} else if (pf->vfs[vf->vf_id].padev) {
+		dev_warn(vf->dev, "%s: vfid %d already running\n",
+			 __func__, vf->vf_id);
+		err = -ENODEV;
+	}

> 
> We have a PF device that has an adminq, VF devices that don't have an
> adminq, and the adminq is needed for some basic setup before the rest of the
> vDPA driver can use the VF.  To access the PF's adminq we set up an
> auxiliary device per feature in each VF - but currently only offer one
> feature (vDPA) and no sub-devices yet.  We're trying to plan for the future.

It looks like premature effort to me.

> 
> Is it that we only have one feature per VF so far is what is causing the
> discomfort?

This whole patch is not easy for me.

Thanks

> 
> sln
>