Re: [RFC PATCH fwctl 1/5] pds_core: specify auxiliary_device to be created

["Nelson, Shannon" <shannon.nelson@xxxxxxx>]

On 2/12/2025 3:57 AM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 15:48:50 -0800
> Shannon Nelson <shannon.nelson@xxxxxxx> wrote:
>
> > In preparation for adding a new auxiliary_device for the
> > PF, make the vif type an argument to pdsc_auxbus_dev_add().
> > We also now pass in the address to where we'll keep the new
> > padev pointer so that the caller can specify where to save it
> > but we can still change it under the mutex and keep the mutex
> > usage within the function.
> >
> > Signed-off-by: Shannon Nelson <shannon.nelson@xxxxxxx>
>
> One trivial comment inline.
>
> > ---
> >   drivers/net/ethernet/amd/pds_core/auxbus.c  | 41 ++++++++++-----------
> >   drivers/net/ethernet/amd/pds_core/core.h    |  7 +++-
> >   drivers/net/ethernet/amd/pds_core/devlink.c |  6 ++-
> >   drivers/net/ethernet/amd/pds_core/main.c    | 11 ++++--
> >   4 files changed, 36 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
> > index 2babea110991..0a3035adda52 100644
> > --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
> > +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
> > @@ -175,34 +175,37 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
> >        return padev;
> >   }
> >
> > -int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
> > +int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf,
> > +                     struct pds_auxiliary_dev **pd_ptr)
> >   {
> >        struct pds_auxiliary_dev *padev;
> > -     int err = 0;
> >
> >        if (!cf)
> >                return -ENODEV;
> >
> > +     if (!*pd_ptr)
> > +             return 0;
> > +
> >        mutex_lock(&pf->config_lock);
> >
> > -     padev = pf->vfs[cf->vf_id].padev;
> > -     if (padev) {
> > -             pds_client_unregister(pf, padev->client_id);
> > -             auxiliary_device_delete(&padev->aux_dev);
> > -             auxiliary_device_uninit(&padev->aux_dev);
> > -             padev->client_id = 0;
> > -     }
> > -     pf->vfs[cf->vf_id].padev = NULL;
> > +     padev = *pd_ptr;
> > +     pds_client_unregister(pf, padev->client_id);
> > +     auxiliary_device_delete(&padev->aux_dev);
> > +     auxiliary_device_uninit(&padev->aux_dev);
> > +     padev->client_id = 0;
> > +     *pd_ptr = NULL;
> >
> >        mutex_unlock(&pf->config_lock);
> > -     return err;
> > +
> > +     return 0;
>
> If you are always going to return 0, maybe change the signature
> to not return anything?
>
> Would require changing the ternary usage below, but perhaps
> it is worth it to remove the implication of failures being
> a possibility.

There is the one error case in pdsc_auxbus_dev_del(), but even that is 
rather useless in that any call is already going to have to have a valid 
pdsc struct to get here.  Yes, I can look at adding a clean-up patch to 
get rid of this useless return before changing the rest of the function 
signature.

sln


> >   }
> >
> > -int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf)
> > +int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf,
> > +                     enum pds_core_vif_types vt,
> > +                     struct pds_auxiliary_dev **pd_ptr)
> >   {
> >        struct pds_auxiliary_dev *padev;
> >        char devname[PDS_DEVNAME_LEN];
> > -     enum pds_core_vif_types vt;
> >        unsigned long mask;
> >        u16 vt_support;
> >        int client_id;
> > @@ -211,6 +214,9 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf)
> >        if (!cf)
> >                return -ENODEV;
> >
> > +     if (vt >= PDS_DEV_TYPE_MAX)
> > +             return -EINVAL;
> > +
> >        mutex_lock(&pf->config_lock);
> >
> >        mask = BIT_ULL(PDSC_S_FW_DEAD) |
> > @@ -222,17 +228,10 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf)
> >                goto out_unlock;
> >        }
> >
> > -     /* We only support vDPA so far, so it is the only one to
> > -      * be verified that it is available in the Core device and
> > -      * enabled in the devlink param.  In the future this might
> > -      * become a loop for several VIF types.
> > -      */
> > -
> >        /* Verify that the type is supported and enabled.  It is not
> >         * an error if there is no auxbus device support for this
> >         * VF, it just means something else needs to happen with it.
> >         */
> > -     vt = PDS_DEV_TYPE_VDPA;
> >        vt_support = !!le16_to_cpu(pf->dev_ident.vif_types[vt]);
> >        if (!(vt_support &&
> >              pf->viftype_status[vt].supported &&
> > @@ -258,7 +257,7 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf)
> >                err = PTR_ERR(padev);
> >                goto out_unlock;
> >        }
> > -     pf->vfs[cf->vf_id].padev = padev;
> > +     *pd_ptr = padev;
> >
> >   out_unlock:
> >        mutex_unlock(&pf->config_lock);
> > diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
> > index 14522d6d5f86..065031dd5af6 100644
> > --- a/drivers/net/ethernet/amd/pds_core/core.h
> > +++ b/drivers/net/ethernet/amd/pds_core/core.h
> > @@ -303,8 +303,11 @@ void pdsc_health_thread(struct work_struct *work);
> >   int pdsc_register_notify(struct notifier_block *nb);
> >   void pdsc_unregister_notify(struct notifier_block *nb);
> >   void pdsc_notify(unsigned long event, void *data);
> > -int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf);
> > -int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf);
> > +int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf,
> > +                     enum pds_core_vif_types vt,
> > +                     struct pds_auxiliary_dev **pd_ptr);
> > +int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf,
> > +                     struct pds_auxiliary_dev **pd_ptr);
> >
> >   void pdsc_process_adminq(struct pdsc_qcq *qcq);
> >   void pdsc_work_thread(struct work_struct *work);
> > diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
> > index 44971e71991f..c2f380f18f21 100644
> > --- a/drivers/net/ethernet/amd/pds_core/devlink.c
> > +++ b/drivers/net/ethernet/amd/pds_core/devlink.c
> > @@ -56,8 +56,10 @@ int pdsc_dl_enable_set(struct devlink *dl, u32 id,
> >        for (vf_id = 0; vf_id < pdsc->num_vfs; vf_id++) {
> >                struct pdsc *vf = pdsc->vfs[vf_id].vf;
> >
> > -             err = ctx->val.vbool ? pdsc_auxbus_dev_add(vf, pdsc) :
> > -                                    pdsc_auxbus_dev_del(vf, pdsc);
> > +             err = ctx->val.vbool ? pdsc_auxbus_dev_add(vf, pdsc, vt_entry->vif_id,
> > +                                                        &pdsc->vfs[vf_id].padev) :
> > +                                    pdsc_auxbus_dev_del(vf, pdsc,
> > +                                                        &pdsc->vfs[vf_id].padev);
> >        }
> >
> >        return err;
> > diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
> > index 660268ff9562..a3a68889137b 100644
> > --- a/drivers/net/ethernet/amd/pds_core/main.c
> > +++ b/drivers/net/ethernet/amd/pds_core/main.c
> > @@ -190,7 +190,8 @@ static int pdsc_init_vf(struct pdsc *vf)
> >        devl_unlock(dl);
> >
> >        pf->vfs[vf->vf_id].vf = vf;
> > -     err = pdsc_auxbus_dev_add(vf, pf);
> > +     err = pdsc_auxbus_dev_add(vf, pf, PDS_DEV_TYPE_VDPA,
> > +                               &pf->vfs[vf->vf_id].padev);
> >        if (err) {
> >                devl_lock(dl);
> >                devl_unregister(dl);
> > @@ -417,7 +418,7 @@ static void pdsc_remove(struct pci_dev *pdev)
> >
> >                pf = pdsc_get_pf_struct(pdsc->pdev);
> >                if (!IS_ERR(pf)) {
> > -                     pdsc_auxbus_dev_del(pdsc, pf);
> > +                     pdsc_auxbus_dev_del(pdsc, pf, &pf->vfs[pdsc->vf_id].padev);
> >                        pf->vfs[pdsc->vf_id].vf = NULL;
> >                }
> >        } else {
> > @@ -482,7 +483,8 @@ static void pdsc_reset_prepare(struct pci_dev *pdev)
> >
> >                pf = pdsc_get_pf_struct(pdsc->pdev);
> >                if (!IS_ERR(pf))
> > -                     pdsc_auxbus_dev_del(pdsc, pf);
> > +                     pdsc_auxbus_dev_del(pdsc, pf,
> > +                                         &pf->vfs[pdsc->vf_id].padev);
> >        }
> >
> >        pdsc_unmap_bars(pdsc);
> > @@ -527,7 +529,8 @@ static void pdsc_reset_done(struct pci_dev *pdev)
> >
> >                pf = pdsc_get_pf_struct(pdsc->pdev);
> >                if (!IS_ERR(pf))
> > -                     pdsc_auxbus_dev_add(pdsc, pf);
> > +                     pdsc_auxbus_dev_add(pdsc, pf, PDS_DEV_TYPE_VDPA,
> > +                                         &pf->vfs[pdsc->vf_id].padev);
> >        }
> >   }