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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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.


>  }
>  
> -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);
>  	}
>  }
>