Re: [PATCH v3 2/6] pds_core: specify auxiliary_device to be created

[Dave Jiang <dave.jiang@intel.com>]

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@nvidia.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.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);
>  	}
>  }
>