| 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: <68c66882-3a91-4843-b08e-2f9d5d2d0290@intel.com>
Date: Mon, 10 Mar 2025 10:26:25 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Shannon Nelson <shannon.nelson@....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/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. ...
>
> 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