| 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: <6dc6a934-e896-45c3-b74d-6adf0ba1e258@amd.com>
Date: Mon, 17 Mar 2025 15:30:43 -0700
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Dave Jiang <dave.jiang@...el.com>, jgg@...dia.com,
andrew.gospodarek@...adcom.com, aron.silverton@...cle.com,
dan.j.williams@...el.com, daniel.vetter@...ll.ch, dsahern@...nel.org,
gregkh@...uxfoundation.org, hch@...radead.org, itayavr@...dia.com,
jiri@...dia.com, Jonathan.Cameron@...wei.com, kuba@...nel.org,
lbloch@...dia.com, leonro@...dia.com, linux-cxl@...r.kernel.org,
linux-rdma@...r.kernel.org, netdev@...r.kernel.org, saeedm@...dia.com
Cc: brett.creeley@....com
Subject: Re: [PATCH v3 2/6] pds_core: specify auxiliary_device to be created
On 3/10/2025 10:26 AM, Dave Jiang wrote:
>
> On 3/7/25 11:53 AM, Shannon Nelson 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.
>
> Please consider changing the commit log to use imperative language and avoid using pronouns such as "we".
>
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes
> https://kernelnewbies.org/PatchPhilosophy#:~:text=Please%20read%20that%20whole%20section,it%20into%20the%20git%20history.
>
> Maybe something like:
> Pass in the address of the padev pointer so the caller can specify where to save it. ...
Done - thanks,
sln
>
>>
>> Reviewed-by: Leon Romanovsky <leonro@...dia.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
>
> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
>
>> ---
>> drivers/net/ethernet/amd/pds_core/auxbus.c | 37 ++++++++++-----------
>> drivers/net/ethernet/amd/pds_core/core.h | 7 ++--
>> drivers/net/ethernet/amd/pds_core/devlink.c | 5 +--
>> drivers/net/ethernet/amd/pds_core/main.c | 11 +++---
>> 4 files changed, 33 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
>> index 78fba368e797..563de9e7ce0a 100644
>> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
>> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
>> @@ -175,29 +175,32 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
>> return padev;
>> }
>>
>> -void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
>> +void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf,
>> + struct pds_auxiliary_dev **pd_ptr)
>> {
>> struct pds_auxiliary_dev *padev;
>>
>> + if (!*pd_ptr)
>> + return;
>> +
>> 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);
>> }
>>
>> -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;
>> @@ -206,6 +209,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) |
>> @@ -217,17 +223,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 &&
>> @@ -253,7 +252,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 631a59cfdd7e..f075e68c64db 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);
>> -void 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);
>> +void 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 4e2b92ddef6f..c5c787df61a4 100644
>> --- a/drivers/net/ethernet/amd/pds_core/devlink.c
>> +++ b/drivers/net/ethernet/amd/pds_core/devlink.c
>> @@ -57,9 +57,10 @@ int pdsc_dl_enable_set(struct devlink *dl, u32 id,
>> struct pdsc *vf = pdsc->vfs[vf_id].vf;
>>
>> if (ctx->val.vbool)
>> - err = pdsc_auxbus_dev_add(vf, pdsc);
>> + err = pdsc_auxbus_dev_add(vf, pdsc, vt_entry->vif_id,
>> + &pdsc->vfs[vf_id].padev);
>> else
>> - pdsc_auxbus_dev_del(vf, pdsc);
>> + 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