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

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

On 2/12/2025 3:57 AM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 15:48:50 -0800
> Shannon Nelson <shannon.nelson@amd.com> 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@amd.com>
> 
> 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);
>>        }
>>   }
>>
>