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

[Leon Romanovsky <leon@kernel.org>]

On Tue, Apr 04, 2023 at 02:44:34PM -0700, Shannon Nelson wrote:
> On 4/2/23 11:18 PM, Leon Romanovsky wrote:
> > 
> > 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.
> 
> Yes, those are extraneous checks left from testing the new driver
> organization.  They are no longer needed, and can come out in the next
> round.
> 
> In addition to spreading out the pds_core.rst creation across the patchset
> and adding more to the commit descriptions, I'll see if there are some other
> nips and tucks I can do to possibly make the patchset more palatable.

You also need to rework your auxdevice id creation scheme to be more
like other drivers.

This line assumes that you have only one PF device in the system.
+       aux_dev->id = vf->id;

Thanks

> 
> sln