| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <eea7b6cf-5e69-4799-9faa-ef625e4545b2@amd.com>
Date: Thu, 13 Feb 2025 14:44:44 -0800
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: jgg@...dia.com, andrew.gospodarek@...adcom.com,
aron.silverton@...cle.com, dan.j.williams@...el.com, daniel.vetter@...ll.ch,
dave.jiang@...el.com, dsahern@...nel.org, gospo@...adcom.com,
hch@...radead.org, itayavr@...dia.com, jiri@...dia.com, kuba@...nel.org,
lbloch@...dia.com, leonro@...dia.com, saeedm@...dia.com,
linux-cxl@...r.kernel.org, linux-rdma@...r.kernel.org,
netdev@...r.kernel.org, brett.creeley@....com
Subject: Re: [RFC PATCH fwctl 1/5] pds_core: specify auxiliary_device to be
created
On 2/12/2025 3:57 AM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 15:48:50 -0800
> Shannon Nelson <shannon.nelson@....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@....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);
>> }
>> }
>>
>
Powered by blists - more mailing lists