[RFC PATCH fwctl 0/5] pds_fwctl: fwctl for AMD/Pensando core devices

[RFC PATCH fwctl 0/5] pds_fwctl: fwctl for AMD/Pensando core devices

[Shannon Nelson]

Following along from Jason's work [1] we have our initial RFC patchset
for pds_fwctl - an auxiliary_bus driver for supporting fwctl through the
pds_core driver and its PDS core device.

The PDS core is PCI device that is separate and distinct from the
ionic Eth device and from the other PCI devices that can be supported
by the AMD/Pensando DSC.  It is used by pds_vdpa and pds_vfio_pci to
coordinate/communicate with the FW for setting up their services.

Until now the DSC's basic configurations for defining what devices to
support and for getting low-level device debug information have been
through internal commands not available from the host side, requiring
access into the DSC's own OS.  Adding the fwctl service allows us to start
bringing these capabilities up into the host, but they don't replace the
existing function-specific tools.  For example, these are things that make
the Eth PCI device appear on the PCI bus, while the tuning of the specific
Eth features still go through the standard ethtool/devlink/ip/etc tools.

The first two patches add a new pds_core.fwctl auxiliary_device to the
pds_core driver.  This is only supported by the pds_core PF and not on
any VFs.

The remaining patches add the pds_fwctl driver framework and then fill
in the details for the fwctl services.

[1] https://lore.kernel.org/netdev/0-v4-0cf4ec3b8143+4995-fwctl_jgg@nvidia.com/
    [PATCH v4 00/10] Introduce fwctl subystem

Brett Creeley (1):
  pds_fwctl: add rpc and query support

Shannon Nelson (4):
  pds_core: specify auxiliary_device to be created
  pds_core: add new fwctl auxilary_device
  pds_fwctl: initial driver framework
  pds_fwctl: add Documentation entries

 Documentation/userspace-api/fwctl/fwctl.rst   |   1 +
 Documentation/userspace-api/fwctl/index.rst   |   1 +
 .../userspace-api/fwctl/pds_fwctl.rst         |  41 ++
 MAINTAINERS                                   |   7 +
 drivers/fwctl/Kconfig                         |  10 +
 drivers/fwctl/Makefile                        |   1 +
 drivers/fwctl/pds/Makefile                    |   4 +
 drivers/fwctl/pds/main.c                      | 558 ++++++++++++++++++
 drivers/net/ethernet/amd/pds_core/auxbus.c    |  44 +-
 drivers/net/ethernet/amd/pds_core/core.c      |   7 +
 drivers/net/ethernet/amd/pds_core/core.h      |   8 +-
 drivers/net/ethernet/amd/pds_core/devlink.c   |   6 +-
 drivers/net/ethernet/amd/pds_core/main.c      |  21 +-
 include/linux/pds/pds_adminq.h                | 264 +++++++++
 include/linux/pds/pds_common.h                |   2 +
 include/uapi/fwctl/fwctl.h                    |   1 +
 include/uapi/fwctl/pds.h                      |  43 ++
 17 files changed, 988 insertions(+), 31 deletions(-)
 create mode 100644 Documentation/userspace-api/fwctl/pds_fwctl.rst
 create mode 100644 drivers/fwctl/pds/Makefile
 create mode 100644 drivers/fwctl/pds/main.c
 create mode 100644 include/uapi/fwctl/pds.h

-- 
2.17.1

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

[Shannon Nelson]

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

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

[Jonathan Cameron]

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

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

[Nelson, Shannon]

On 2/12/2025 3:57 AM, Jonathan Cameron wrote:
> 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.

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

[RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Shannon Nelson]

Add support for a new fwctl-based auxiliary_device for creating a
channel for fwctl support into the AMD/Pensando DSC.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/auxbus.c |  3 +--
 drivers/net/ethernet/amd/pds_core/core.c   |  7 +++++++
 drivers/net/ethernet/amd/pds_core/core.h   |  1 +
 drivers/net/ethernet/amd/pds_core/main.c   | 10 ++++++++++
 include/linux/pds/pds_common.h             |  2 ++
 5 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
index 0a3035adda52..857697ae1e4b 100644
--- a/drivers/net/ethernet/amd/pds_core/auxbus.c
+++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
@@ -229,8 +229,7 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf,
 	}
 
 	/* 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.
+	 * an error if there is no auxbus device support.
 	 */
 	vt_support = !!le16_to_cpu(pf->dev_ident.vif_types[vt]);
 	if (!(vt_support &&
diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
index 536635e57727..1eb0d92786f7 100644
--- a/drivers/net/ethernet/amd/pds_core/core.c
+++ b/drivers/net/ethernet/amd/pds_core/core.c
@@ -402,6 +402,9 @@ static int pdsc_core_init(struct pdsc *pdsc)
 }
 
 static struct pdsc_viftype pdsc_viftype_defaults[] = {
+	[PDS_DEV_TYPE_FWCTL] = { .name = PDS_DEV_TYPE_FWCTL_STR,
+				 .vif_id = PDS_DEV_TYPE_FWCTL,
+				 .dl_id = -1 },
 	[PDS_DEV_TYPE_VDPA] = { .name = PDS_DEV_TYPE_VDPA_STR,
 				.vif_id = PDS_DEV_TYPE_VDPA,
 				.dl_id = DEVLINK_PARAM_GENERIC_ID_ENABLE_VNET },
@@ -428,6 +431,10 @@ static int pdsc_viftypes_init(struct pdsc *pdsc)
 
 		/* See what the Core device has for support */
 		vt_support = !!le16_to_cpu(pdsc->dev_ident.vif_types[vt]);
+
+		if (vt == PDS_DEV_TYPE_FWCTL)
+			pdsc->viftype_status[vt].enabled = true;
+
 		dev_dbg(pdsc->dev, "VIF %s is %ssupported\n",
 			pdsc->viftype_status[vt].name,
 			vt_support ? "" : "not ");
diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
index 065031dd5af6..218bb9c4c780 100644
--- a/drivers/net/ethernet/amd/pds_core/core.h
+++ b/drivers/net/ethernet/amd/pds_core/core.h
@@ -156,6 +156,7 @@ struct pdsc {
 	struct dentry *dentry;
 	struct device *dev;
 	struct pdsc_dev_bar bars[PDS_CORE_BARS_MAX];
+	struct pds_auxiliary_dev *padev;
 	struct pdsc_vf *vfs;
 	int num_vfs;
 	int vf_id;
diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
index a3a68889137b..7f20c3f5f349 100644
--- a/drivers/net/ethernet/amd/pds_core/main.c
+++ b/drivers/net/ethernet/amd/pds_core/main.c
@@ -265,6 +265,10 @@ static int pdsc_init_pf(struct pdsc *pdsc)
 
 	mutex_unlock(&pdsc->config_lock);
 
+	err = pdsc_auxbus_dev_add(pdsc, pdsc, PDS_DEV_TYPE_FWCTL, &pdsc->padev);
+	if (err)
+		goto err_out_teardown;
+
 	dl = priv_to_devlink(pdsc);
 	devl_lock(dl);
 	err = devl_params_register(dl, pdsc_dl_params,
@@ -427,6 +431,7 @@ static void pdsc_remove(struct pci_dev *pdev)
 		 * shut themselves down.
 		 */
 		pdsc_sriov_configure(pdev, 0);
+		pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
 
 		timer_shutdown_sync(&pdsc->wdtimer);
 		if (pdsc->wq)
@@ -485,6 +490,8 @@ static void pdsc_reset_prepare(struct pci_dev *pdev)
 		if (!IS_ERR(pf))
 			pdsc_auxbus_dev_del(pdsc, pf,
 					    &pf->vfs[pdsc->vf_id].padev);
+	} else {
+		pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
 	}
 
 	pdsc_unmap_bars(pdsc);
@@ -531,6 +538,9 @@ static void pdsc_reset_done(struct pci_dev *pdev)
 		if (!IS_ERR(pf))
 			pdsc_auxbus_dev_add(pdsc, pf, PDS_DEV_TYPE_VDPA,
 					    &pf->vfs[pdsc->vf_id].padev);
+	} else {
+		pdsc_auxbus_dev_add(pdsc, pdsc, PDS_DEV_TYPE_FWCTL,
+				    &pdsc->padev);
 	}
 }
 
diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h
index 5802e1deef24..b193adbe7cc3 100644
--- a/include/linux/pds/pds_common.h
+++ b/include/linux/pds/pds_common.h
@@ -29,6 +29,7 @@ enum pds_core_vif_types {
 	PDS_DEV_TYPE_ETH	= 3,
 	PDS_DEV_TYPE_RDMA	= 4,
 	PDS_DEV_TYPE_LM		= 5,
+	PDS_DEV_TYPE_FWCTL	= 6,
 
 	/* new ones added before this line */
 	PDS_DEV_TYPE_MAX	= 16   /* don't change - used in struct size */
@@ -40,6 +41,7 @@ enum pds_core_vif_types {
 #define PDS_DEV_TYPE_ETH_STR	"Eth"
 #define PDS_DEV_TYPE_RDMA_STR	"RDMA"
 #define PDS_DEV_TYPE_LM_STR	"LM"
+#define PDS_DEV_TYPE_FWCTL_STR	"fwctl"
 
 #define PDS_VDPA_DEV_NAME	PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_VDPA_STR
 #define PDS_VFIO_LM_DEV_NAME	PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_LM_STR "." PDS_DEV_TYPE_VFIO_STR
-- 
2.17.1

Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Jonathan Cameron]

On Tue, 11 Feb 2025 15:48:51 -0800
Shannon Nelson <shannon.nelson@amd.com> wrote:

> Add support for a new fwctl-based auxiliary_device for creating a
> channel for fwctl support into the AMD/Pensando DSC.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>

Hi Shannon,

> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
> index a3a68889137b..7f20c3f5f349 100644
> --- a/drivers/net/ethernet/amd/pds_core/main.c
> +++ b/drivers/net/ethernet/amd/pds_core/main.c
> @@ -265,6 +265,10 @@ static int pdsc_init_pf(struct pdsc *pdsc)
>  
>  	mutex_unlock(&pdsc->config_lock);
>  
> +	err = pdsc_auxbus_dev_add(pdsc, pdsc, PDS_DEV_TYPE_FWCTL, &pdsc->padev);
> +	if (err)
> +		goto err_out_teardown;
> +

If you fail after this point, do you not want to remove this device?
Also superficially shouldn't this be goto err_out_stop? I think
the call is just after pdsc_start(pdsc) though maybe I'm looking at
an old tree.


>  	dl = priv_to_devlink(pdsc);
>  	devl_lock(dl);
>  	err = devl_params_register(dl, pdsc_dl_params,

Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Nelson, Shannon]

On 2/12/2025 4:02 AM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 15:48:51 -0800
> Shannon Nelson <shannon.nelson@amd.com> wrote:
> 
>> Add support for a new fwctl-based auxiliary_device for creating a
>> channel for fwctl support into the AMD/Pensando DSC.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> 
> Hi Shannon,
> 
>> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
>> index a3a68889137b..7f20c3f5f349 100644
>> --- a/drivers/net/ethernet/amd/pds_core/main.c
>> +++ b/drivers/net/ethernet/amd/pds_core/main.c
>> @@ -265,6 +265,10 @@ static int pdsc_init_pf(struct pdsc *pdsc)
>>
>>        mutex_unlock(&pdsc->config_lock);
>>
>> +     err = pdsc_auxbus_dev_add(pdsc, pdsc, PDS_DEV_TYPE_FWCTL, &pdsc->padev);
>> +     if (err)
>> +             goto err_out_teardown;
>> +
> 
> If you fail after this point, do you not want to remove this device?
> Also superficially shouldn't this be goto err_out_stop? I think
> the call is just after pdsc_start(pdsc) though maybe I'm looking at
> an old tree.

Thanks, I'll fix this.
sln

> 
> 
>>        dl = priv_to_devlink(pdsc);
>>        devl_lock(dl);
>>        err = devl_params_register(dl, pdsc_dl_params,
> 

Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Jonathan Cameron]

On Tue, 11 Feb 2025 15:48:51 -0800
Shannon Nelson <shannon.nelson@amd.com> wrote:

> Add support for a new fwctl-based auxiliary_device for creating a
> channel for fwctl support into the AMD/Pensando DSC.

auxiliary_device in title.

Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Nelson, Shannon]

On 2/12/2025 4:03 AM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 15:48:51 -0800
> Shannon Nelson <shannon.nelson@amd.com> wrote:
> 
>> Add support for a new fwctl-based auxiliary_device for creating a
>> channel for fwctl support into the AMD/Pensando DSC.
> 
> auxiliary_device in title.
> 

Good catch - I almost had them all fixed...
sln

Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Leon Romanovsky]

On Tue, Feb 11, 2025 at 03:48:51PM -0800, Shannon Nelson wrote:
> Add support for a new fwctl-based auxiliary_device for creating a
> channel for fwctl support into the AMD/Pensando DSC.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  drivers/net/ethernet/amd/pds_core/auxbus.c |  3 +--
>  drivers/net/ethernet/amd/pds_core/core.c   |  7 +++++++
>  drivers/net/ethernet/amd/pds_core/core.h   |  1 +
>  drivers/net/ethernet/amd/pds_core/main.c   | 10 ++++++++++
>  include/linux/pds/pds_common.h             |  2 ++
>  5 files changed, 21 insertions(+), 2 deletions(-)

<...>

My comment is only slightly related to the patch itself, but worth to
write it anyway.

> diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h
> index 5802e1deef24..b193adbe7cc3 100644
> --- a/include/linux/pds/pds_common.h
> +++ b/include/linux/pds/pds_common.h
> @@ -29,6 +29,7 @@ enum pds_core_vif_types {
>  	PDS_DEV_TYPE_ETH	= 3,
>  	PDS_DEV_TYPE_RDMA	= 4,
>  	PDS_DEV_TYPE_LM		= 5,
> +	PDS_DEV_TYPE_FWCTL	= 6,

This enum and defines below should be cleaned from unsupported types.
I don't see any code for RDMA, LM and ETH.

Thanks

>  
>  	/* new ones added before this line */
>  	PDS_DEV_TYPE_MAX	= 16   /* don't change - used in struct size */
> @@ -40,6 +41,7 @@ enum pds_core_vif_types {
>  #define PDS_DEV_TYPE_ETH_STR	"Eth"
>  #define PDS_DEV_TYPE_RDMA_STR	"RDMA"
>  #define PDS_DEV_TYPE_LM_STR	"LM"
> +#define PDS_DEV_TYPE_FWCTL_STR	"fwctl"
>  
>  #define PDS_VDPA_DEV_NAME	PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_VDPA_STR
>  #define PDS_VFIO_LM_DEV_NAME	PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_LM_STR "." PDS_DEV_TYPE_VFIO_STR
> -- 
> 2.17.1
> 
> 

Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Nelson, Shannon]

On 2/18/2025 11:28 AM, Leon Romanovsky wrote:
> 
> On Tue, Feb 11, 2025 at 03:48:51PM -0800, Shannon Nelson wrote:
>> Add support for a new fwctl-based auxiliary_device for creating a
>> channel for fwctl support into the AMD/Pensando DSC.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>>   drivers/net/ethernet/amd/pds_core/auxbus.c |  3 +--
>>   drivers/net/ethernet/amd/pds_core/core.c   |  7 +++++++
>>   drivers/net/ethernet/amd/pds_core/core.h   |  1 +
>>   drivers/net/ethernet/amd/pds_core/main.c   | 10 ++++++++++
>>   include/linux/pds/pds_common.h             |  2 ++
>>   5 files changed, 21 insertions(+), 2 deletions(-)
> 
> <...>
> 
> My comment is only slightly related to the patch itself, but worth to
> write it anyway.
> 
>> diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h
>> index 5802e1deef24..b193adbe7cc3 100644
>> --- a/include/linux/pds/pds_common.h
>> +++ b/include/linux/pds/pds_common.h
>> @@ -29,6 +29,7 @@ enum pds_core_vif_types {
>>        PDS_DEV_TYPE_ETH        = 3,
>>        PDS_DEV_TYPE_RDMA       = 4,
>>        PDS_DEV_TYPE_LM         = 5,
>> +     PDS_DEV_TYPE_FWCTL      = 6,
> 
> This enum and defines below should be cleaned from unsupported types.
> I don't see any code for RDMA, LM and ETH.
> 
> Thanks

I've looked at those a few times over the life of this code, but I 
continue to leave them there because they are part of the firmware 
interface definition, whether we use them or not.

You're right, there is no ETH or RDMA type code, they exist as 
historical artifacts of the early interface design.

The LM type underlies the device used by the pds-vfio-pci driver and is 
a value that pds_core will see in the device identity information 
gathered from the firmware if there are VFIO VFs configured in the FW.

I'd rather not mess with this enum for this patchset, but I'll keep this 
in mind for future cleanup work.

Thanks,
sln

> 
>>
>>        /* new ones added before this line */
>>        PDS_DEV_TYPE_MAX        = 16   /* don't change - used in struct size */
>> @@ -40,6 +41,7 @@ enum pds_core_vif_types {
>>   #define PDS_DEV_TYPE_ETH_STR "Eth"
>>   #define PDS_DEV_TYPE_RDMA_STR        "RDMA"
>>   #define PDS_DEV_TYPE_LM_STR  "LM"
>> +#define PDS_DEV_TYPE_FWCTL_STR       "fwctl"
>>
>>   #define PDS_VDPA_DEV_NAME    PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_VDPA_STR
>>   #define PDS_VFIO_LM_DEV_NAME PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_LM_STR "." PDS_DEV_TYPE_VFIO_STR
>> --
>> 2.17.1
>>
>>

Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Leon Romanovsky]

On Tue, Feb 18, 2025 at 12:00:52PM -0800, Nelson, Shannon wrote:
> On 2/18/2025 11:28 AM, Leon Romanovsky wrote:
> > 
> > On Tue, Feb 11, 2025 at 03:48:51PM -0800, Shannon Nelson wrote:
> > > Add support for a new fwctl-based auxiliary_device for creating a
> > > channel for fwctl support into the AMD/Pensando DSC.
> > > 
> > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> > > ---
> > >   drivers/net/ethernet/amd/pds_core/auxbus.c |  3 +--
> > >   drivers/net/ethernet/amd/pds_core/core.c   |  7 +++++++
> > >   drivers/net/ethernet/amd/pds_core/core.h   |  1 +
> > >   drivers/net/ethernet/amd/pds_core/main.c   | 10 ++++++++++
> > >   include/linux/pds/pds_common.h             |  2 ++
> > >   5 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > <...>
> > 
> > My comment is only slightly related to the patch itself, but worth to
> > write it anyway.
> > 
> > > diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h
> > > index 5802e1deef24..b193adbe7cc3 100644
> > > --- a/include/linux/pds/pds_common.h
> > > +++ b/include/linux/pds/pds_common.h
> > > @@ -29,6 +29,7 @@ enum pds_core_vif_types {
> > >        PDS_DEV_TYPE_ETH        = 3,
> > >        PDS_DEV_TYPE_RDMA       = 4,
> > >        PDS_DEV_TYPE_LM         = 5,
> > > +     PDS_DEV_TYPE_FWCTL      = 6,
> > 
> > This enum and defines below should be cleaned from unsupported types.
> > I don't see any code for RDMA, LM and ETH.
> > 
> > Thanks
> 
> I've looked at those a few times over the life of this code, but I continue
> to leave them there because they are part of the firmware interface
> definition, whether we use them or not.

How? You are passing some number which FW is not aware of it. You can
pass any number you want there. Even it is not true, you can
PDS_DEV_TYPE_FWCTL = 6 here, but remove rest of enums and *_STR defines.

> 
> You're right, there is no ETH or RDMA type code, they exist as historical
> artifacts of the early interface design.

This make me wonder why netdev merged this code which has nothing to do
with netdev subsystem at all.

Thanks

Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Nelson, Shannon]

On 2/19/2025 12:24 AM, Leon Romanovsky wrote:
> 
> On Tue, Feb 18, 2025 at 12:00:52PM -0800, Nelson, Shannon wrote:
>> On 2/18/2025 11:28 AM, Leon Romanovsky wrote:
>>>
>>> On Tue, Feb 11, 2025 at 03:48:51PM -0800, Shannon Nelson wrote:
>>>> Add support for a new fwctl-based auxiliary_device for creating a
>>>> channel for fwctl support into the AMD/Pensando DSC.
>>>>
>>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>>>> ---
>>>>    drivers/net/ethernet/amd/pds_core/auxbus.c |  3 +--
>>>>    drivers/net/ethernet/amd/pds_core/core.c   |  7 +++++++
>>>>    drivers/net/ethernet/amd/pds_core/core.h   |  1 +
>>>>    drivers/net/ethernet/amd/pds_core/main.c   | 10 ++++++++++
>>>>    include/linux/pds/pds_common.h             |  2 ++
>>>>    5 files changed, 21 insertions(+), 2 deletions(-)
>>>
>>> <...>
>>>
>>> My comment is only slightly related to the patch itself, but worth to
>>> write it anyway.
>>>
>>>> diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h
>>>> index 5802e1deef24..b193adbe7cc3 100644
>>>> --- a/include/linux/pds/pds_common.h
>>>> +++ b/include/linux/pds/pds_common.h
>>>> @@ -29,6 +29,7 @@ enum pds_core_vif_types {
>>>>         PDS_DEV_TYPE_ETH        = 3,
>>>>         PDS_DEV_TYPE_RDMA       = 4,
>>>>         PDS_DEV_TYPE_LM         = 5,
>>>> +     PDS_DEV_TYPE_FWCTL      = 6,
>>>
>>> This enum and defines below should be cleaned from unsupported types.
>>> I don't see any code for RDMA, LM and ETH.
>>>
>>> Thanks
>>
>> I've looked at those a few times over the life of this code, but I continue
>> to leave them there because they are part of the firmware interface
>> definition, whether we use them or not.
> 
> How? You are passing some number which FW is not aware of it. You can
> pass any number you want there. Even it is not true, you can
> PDS_DEV_TYPE_FWCTL = 6 here, but remove rest of enums and *_STR defines.

When pds_core starts up it gets ident/config information from the 
firmware in a struct pds_core_dev_identity, which includes the 
vif_types[] array which tells us how many of each PDS_DEV_TYPE_xx are 
supported in the FW.  This is indexed by enum pds_core_vif_types.

> 
>>
>> You're right, there is no ETH or RDMA type code, they exist as historical
>> artifacts of the early interface design.
> 
> This make me wonder why netdev merged this code which has nothing to do
> with netdev subsystem at all.

The pds_core was originally brought in for supporting pds_vdpa and 
pds_vfio_pci.  At the time, it was essentially following the example of 
the mlx core module; at the time there wasn't any push back or 
suggestions of a different place to land.  Maybe further fwctl and 
"core" related discussions will suggest another approach.

sln

> 
> Thanks

Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Leon Romanovsky]

On Thu, Feb 20, 2025 at 03:20:14PM -0800, Nelson, Shannon wrote:
> On 2/19/2025 12:24 AM, Leon Romanovsky wrote:
> > 
> > On Tue, Feb 18, 2025 at 12:00:52PM -0800, Nelson, Shannon wrote:
> > > On 2/18/2025 11:28 AM, Leon Romanovsky wrote:
> > > > 
> > > > On Tue, Feb 11, 2025 at 03:48:51PM -0800, Shannon Nelson wrote:
> > > > > Add support for a new fwctl-based auxiliary_device for creating a
> > > > > channel for fwctl support into the AMD/Pensando DSC.
> > > > > 
> > > > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> > > > > ---
> > > > >    drivers/net/ethernet/amd/pds_core/auxbus.c |  3 +--
> > > > >    drivers/net/ethernet/amd/pds_core/core.c   |  7 +++++++
> > > > >    drivers/net/ethernet/amd/pds_core/core.h   |  1 +
> > > > >    drivers/net/ethernet/amd/pds_core/main.c   | 10 ++++++++++
> > > > >    include/linux/pds/pds_common.h             |  2 ++
> > > > >    5 files changed, 21 insertions(+), 2 deletions(-)
> > > > 
> > > > <...>
> > > > 
> > > > My comment is only slightly related to the patch itself, but worth to
> > > > write it anyway.
> > > > 
> > > > > diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h
> > > > > index 5802e1deef24..b193adbe7cc3 100644
> > > > > --- a/include/linux/pds/pds_common.h
> > > > > +++ b/include/linux/pds/pds_common.h
> > > > > @@ -29,6 +29,7 @@ enum pds_core_vif_types {
> > > > >         PDS_DEV_TYPE_ETH        = 3,
> > > > >         PDS_DEV_TYPE_RDMA       = 4,
> > > > >         PDS_DEV_TYPE_LM         = 5,
> > > > > +     PDS_DEV_TYPE_FWCTL      = 6,
> > > > 
> > > > This enum and defines below should be cleaned from unsupported types.
> > > > I don't see any code for RDMA, LM and ETH.
> > > > 
> > > > Thanks
> > > 
> > > I've looked at those a few times over the life of this code, but I continue
> > > to leave them there because they are part of the firmware interface
> > > definition, whether we use them or not.
> > 
> > How? You are passing some number which FW is not aware of it. You can
> > pass any number you want there. Even it is not true, you can
> > PDS_DEV_TYPE_FWCTL = 6 here, but remove rest of enums and *_STR defines.
> 
> When pds_core starts up it gets ident/config information from the firmware
> in a struct pds_core_dev_identity, which includes the vif_types[] array
> which tells us how many of each PDS_DEV_TYPE_xx are supported in the FW.
> This is indexed by enum pds_core_vif_types.

So just leave in the kernel, the PDS_DEV_TYPE_XXX which you support.

> 
> > 
> > > 
> > > You're right, there is no ETH or RDMA type code, they exist as historical
> > > artifacts of the early interface design.
> > 
> > This make me wonder why netdev merged this code which has nothing to do
> > with netdev subsystem at all.
> 
> The pds_core was originally brought in for supporting pds_vdpa and
> pds_vfio_pci.  At the time, it was essentially following the example of the
> mlx core module; at the time there wasn't any push back or suggestions of a
> different place to land.  Maybe further fwctl and "core" related discussions
> will suggest another approach.

The understanding of that "core" drivers don't belong to netdev was
always there.

Thanks

> 
> sln
> 
> > 
> > Thanks
> 
> 

[RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Shannon Nelson]

Initial files for adding a new fwctl driver for the AMD/Pensando PDS
devices.  This sets up a simple auxiliary_bus driver that registers
with fwctl subsystem.  It expects that a pds_core device has set up
the auxiliary_device pds_core.fwctl

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 MAINTAINERS                    |   7 ++
 drivers/fwctl/Kconfig          |  10 ++
 drivers/fwctl/Makefile         |   1 +
 drivers/fwctl/pds/Makefile     |   4 +
 drivers/fwctl/pds/main.c       | 195 +++++++++++++++++++++++++++++++++
 include/linux/pds/pds_adminq.h |  77 +++++++++++++
 include/uapi/fwctl/fwctl.h     |   1 +
 include/uapi/fwctl/pds.h       |  27 +++++
 8 files changed, 322 insertions(+)
 create mode 100644 drivers/fwctl/pds/Makefile
 create mode 100644 drivers/fwctl/pds/main.c
 create mode 100644 include/uapi/fwctl/pds.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 413ab79bf2f4..123f8a9c0b26 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9602,6 +9602,13 @@ T:	git git://linuxtv.org/media.git
 F:	Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
 F:	drivers/media/i2c/gc2145.c
 
+FWCTL PDS DRIVER
+M:	Brett Creeley <brett.creeley@amd.com>
+R:	Shannon Nelson <shannon.nelson@amd.com>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	drivers/fwctl/pds/
+
 GATEWORKS SYSTEM CONTROLLER (GSC) DRIVER
 M:	Tim Harvey <tharvey@gateworks.com>
 S:	Maintained
diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
index 0a542a247303..df87ce5bd8aa 100644
--- a/drivers/fwctl/Kconfig
+++ b/drivers/fwctl/Kconfig
@@ -28,5 +28,15 @@ config FWCTL_MLX5
 	  This will allow configuration and debug tools to work out of the box on
 	  mainstream kernel.
 
+	  If you don't know what to do here, say N.
+
+config FWCTL_PDS
+	tristate "AMD/Pensando pds fwctl driver"
+	depends on PDS_CORE
+	help
+	  The pds_fwctl driver provides an fwctl interface for a user process
+	  to access the debug and configuration information of the AMD/Pensando
+	  DSC hardware family.
+
 	  If you don't know what to do here, say N.
 endif
diff --git a/drivers/fwctl/Makefile b/drivers/fwctl/Makefile
index 5fb289243286..692e4b8d7beb 100644
--- a/drivers/fwctl/Makefile
+++ b/drivers/fwctl/Makefile
@@ -2,5 +2,6 @@
 obj-$(CONFIG_FWCTL) += fwctl.o
 obj-$(CONFIG_FWCTL_BNXT) += bnxt/
 obj-$(CONFIG_FWCTL_MLX5) += mlx5/
+obj-$(CONFIG_FWCTL_PDS) += pds/
 
 fwctl-y += main.o
diff --git a/drivers/fwctl/pds/Makefile b/drivers/fwctl/pds/Makefile
new file mode 100644
index 000000000000..c14cba128e3b
--- /dev/null
+++ b/drivers/fwctl/pds/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
+obj-$(CONFIG_FWCTL_PDS) += pds_fwctl.o
+
+pds_fwctl-y += main.o
diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
new file mode 100644
index 000000000000..24979fe0deea
--- /dev/null
+++ b/drivers/fwctl/pds/main.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
+/* Copyright(c) Advanced Micro Devices, Inc */
+
+#include <linux/module.h>
+#include <linux/auxiliary_bus.h>
+#include <linux/pci.h>
+#include <linux/vmalloc.h>
+
+#include <uapi/fwctl/fwctl.h>
+#include <uapi/fwctl/pds.h>
+#include <linux/fwctl.h>
+
+#include <linux/pds/pds_common.h>
+#include <linux/pds/pds_core_if.h>
+#include <linux/pds/pds_adminq.h>
+#include <linux/pds/pds_auxbus.h>
+
+struct pdsfc_uctx {
+	struct fwctl_uctx uctx;
+	u32 uctx_caps;
+	u32 uctx_uid;
+};
+
+struct pdsfc_dev {
+	struct fwctl_device fwctl;
+	struct pds_auxiliary_dev *padev;
+	struct pdsc *pdsc;
+	u32 caps;
+	dma_addr_t ident_pa;
+	struct pds_fwctl_ident *ident;
+};
+DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));
+
+static int pdsfc_open_uctx(struct fwctl_uctx *uctx)
+{
+	struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
+	struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
+	struct device *dev = &uctx->fwctl->dev;
+
+	dev_dbg(dev, "%s: caps = 0x%04x\n", __func__, pdsfc->caps);
+	pdsfc_uctx->uctx_caps = pdsfc->caps;
+
+	return 0;
+}
+
+static void pdsfc_close_uctx(struct fwctl_uctx *uctx)
+{
+}
+
+static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length)
+{
+	struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
+	struct fwctl_info_pds *info;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	info->uctx_caps = pdsfc_uctx->uctx_caps;
+
+	return info;
+}
+
+static void pdsfc_free_ident(struct pdsfc_dev *pdsfc)
+{
+	struct device *dev = &pdsfc->fwctl.dev;
+
+	if (pdsfc->ident) {
+		dma_free_coherent(dev, sizeof(*pdsfc->ident),
+				  pdsfc->ident, pdsfc->ident_pa);
+		pdsfc->ident = NULL;
+		pdsfc->ident_pa = DMA_MAPPING_ERROR;
+	}
+}
+
+static int pdsfc_identify(struct pdsfc_dev *pdsfc)
+{
+	struct device *dev = &pdsfc->fwctl.dev;
+	union pds_core_adminq_comp comp = {0};
+	union pds_core_adminq_cmd cmd = {0};
+	struct pds_fwctl_ident *ident;
+	dma_addr_t ident_pa;
+	int err = 0;
+
+	ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL);
+	err = dma_mapping_error(dev->parent, ident_pa);
+	if (err) {
+		dev_err(dev, "Failed to map ident\n");
+		return err;
+	}
+
+	cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
+	cmd.fwctl_ident.version = 0;
+	cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident));
+	cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa);
+
+	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
+	if (err) {
+		dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa);
+		dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
+			cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
+		return err;
+	}
+
+	pdsfc->ident = ident;
+	pdsfc->ident_pa = ident_pa;
+
+	dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n",
+		ident->version, ident->max_req_sz, ident->max_resp_sz,
+		ident->max_req_sg_elems, ident->max_resp_sg_elems);
+
+	return 0;
+}
+
+static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
+			  void *in, size_t in_len, size_t *out_len)
+{
+	return NULL;
+}
+
+static const struct fwctl_ops pdsfc_ops = {
+	.device_type = FWCTL_DEVICE_TYPE_PDS,
+	.uctx_size = sizeof(struct pdsfc_uctx),
+	.open_uctx = pdsfc_open_uctx,
+	.close_uctx = pdsfc_close_uctx,
+	.info = pdsfc_info,
+	.fw_rpc = pdsfc_fw_rpc,
+};
+
+static int pdsfc_probe(struct auxiliary_device *adev,
+		       const struct auxiliary_device_id *id)
+{
+	struct pdsfc_dev *pdsfc __free(pdsfc_dev);
+	struct pds_auxiliary_dev *padev;
+	struct device *dev = &adev->dev;
+	int err = 0;
+
+	padev = container_of(adev, struct pds_auxiliary_dev, aux_dev);
+	pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
+				   struct pdsfc_dev, fwctl);
+	if (!pdsfc) {
+		dev_err(dev, "Failed to allocate fwctl device struct\n");
+		return -ENOMEM;
+	}
+	pdsfc->padev = padev;
+
+	err = pdsfc_identify(pdsfc);
+	if (err) {
+		dev_err(dev, "Failed to identify device, err %d\n", err);
+		return err;
+	}
+
+	err = fwctl_register(&pdsfc->fwctl);
+	if (err) {
+		dev_err(dev, "Failed to register device, err %d\n", err);
+		return err;
+	}
+
+	auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
+
+	return 0;
+
+free_ident:
+	pdsfc_free_ident(pdsfc);
+	return err;
+}
+
+static void pdsfc_remove(struct auxiliary_device *adev)
+{
+	struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
+
+	fwctl_unregister(&pdsfc->fwctl);
+	pdsfc_free_ident(pdsfc);
+}
+
+static const struct auxiliary_device_id pdsfc_id_table[] = {
+	{.name = PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_FWCTL_STR },
+	{}
+};
+MODULE_DEVICE_TABLE(auxiliary, pdsfc_id_table);
+
+static struct auxiliary_driver pdsfc_driver = {
+	.name = "pds_fwctl",
+	.probe = pdsfc_probe,
+	.remove = pdsfc_remove,
+	.id_table = pdsfc_id_table,
+};
+
+module_auxiliary_driver(pdsfc_driver);
+
+MODULE_IMPORT_NS(FWCTL);
+MODULE_DESCRIPTION("pds fwctl driver");
+MODULE_AUTHOR("Shannon Nelson <shannon.nelson@amd.com>");
+MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>");
+MODULE_LICENSE("Dual BSD/GPL");
diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
index 4b4e9a98b37b..7fc353b63353 100644
--- a/include/linux/pds/pds_adminq.h
+++ b/include/linux/pds/pds_adminq.h
@@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd {
 	u8     status;
 };
 
+enum pds_fwctl_cmd_opcode {
+	PDS_FWCTL_CMD_IDENT		= 70,
+};
+
+/**
+ * struct pds_fwctl_cmd - Firmware control command structure
+ * @opcode: Opcode
+ * @rsvd:   Word boundary padding
+ * @ep:     Endpoint identifier.
+ * @op:     Operation identifier.
+ */
+struct pds_fwctl_cmd {
+	u8     opcode;
+	u8     rsvd[3];
+	__le32 ep;
+	__le32 op;
+} __packed;
+
+/**
+ * struct pds_fwctl_comp - Firmware control completion structure
+ * @status:     Status of the firmware control operation
+ * @rsvd:       Word boundary padding
+ * @comp_index: Completion index in little-endian format
+ * @rsvd2:      Word boundary padding
+ * @color:      Color bit indicating the state of the completion
+ */
+struct pds_fwctl_comp {
+	u8     status;
+	u8     rsvd;
+	__le16 comp_index;
+	u8     rsvd2[11];
+	u8     color;
+} __packed;
+
+/**
+ * struct pds_fwctl_ident_cmd - Firmware control identification command structure
+ * @opcode:   Operation code for the command
+ * @rsvd:     Word boundary padding
+ * @version:  Interface version
+ * @rsvd2:    Word boundary padding
+ * @len:      Length of the identification data
+ * @ident_pa: Physical address of the identification data
+ */
+struct pds_fwctl_ident_cmd {
+	u8     opcode;
+	u8     rsvd;
+	u8     version;
+	u8     rsvd2;
+	__le32 len;
+	__le64 ident_pa;
+} __packed;
+
+/**
+ * struct pds_fwctl_ident - Firmware control identification structure
+ * @features:    Supported features
+ * @version:     Interface version
+ * @rsvd:        Word boundary padding
+ * @max_req_sz:  Maximum request size
+ * @max_resp_sz: Maximum response size
+ * @max_req_sg_elems:  Maximum number of request SGs
+ * @max_resp_sg_elems: Maximum number of response SGs
+ */
+struct pds_fwctl_ident {
+	__le64 features;
+	u8     version;
+	u8     rsvd[3];
+	__le32 max_req_sz;
+	__le32 max_resp_sz;
+	u8     max_req_sg_elems;
+	u8     max_resp_sg_elems;
+} __packed;
+
 union pds_core_adminq_cmd {
 	u8     opcode;
 	u8     bytes[64];
@@ -1216,6 +1288,9 @@ union pds_core_adminq_cmd {
 	struct pds_lm_dirty_enable_cmd	  lm_dirty_enable;
 	struct pds_lm_dirty_disable_cmd	  lm_dirty_disable;
 	struct pds_lm_dirty_seq_ack_cmd	  lm_dirty_seq_ack;
+
+	struct pds_fwctl_cmd		  fwctl;
+	struct pds_fwctl_ident_cmd	  fwctl_ident;
 };
 
 union pds_core_adminq_comp {
@@ -1243,6 +1318,8 @@ union pds_core_adminq_comp {
 
 	struct pds_lm_state_size_comp	  lm_state_size;
 	struct pds_lm_dirty_status_comp	  lm_dirty_status;
+
+	struct pds_fwctl_comp		  fwctl;
 };
 
 #ifndef __CHECKER__
diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
index 518f054f02d2..a884e9f6dc2c 100644
--- a/include/uapi/fwctl/fwctl.h
+++ b/include/uapi/fwctl/fwctl.h
@@ -44,6 +44,7 @@ enum fwctl_device_type {
 	FWCTL_DEVICE_TYPE_ERROR = 0,
 	FWCTL_DEVICE_TYPE_MLX5 = 1,
 	FWCTL_DEVICE_TYPE_BNXT = 3,
+	FWCTL_DEVICE_TYPE_PDS = 4,
 };
 
 /**
diff --git a/include/uapi/fwctl/pds.h b/include/uapi/fwctl/pds.h
new file mode 100644
index 000000000000..a01b032cbdb1
--- /dev/null
+++ b/include/uapi/fwctl/pds.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/* Copyright(c) Advanced Micro Devices, Inc */
+
+/*
+ * fwctl interface info for pds_fwctl
+ */
+
+#ifndef _UAPI_FWCTL_PDS_H_
+#define _UAPI_FWCTL_PDS_H_
+
+#include <linux/types.h>
+
+/*
+ * struct fwctl_info_pds
+ *
+ * Return basic information about the FW interface available.
+ */
+struct fwctl_info_pds {
+	__u32 uid;
+	__u32 uctx_caps;
+};
+
+enum pds_fwctl_capabilities {
+	PDS_FWCTL_QUERY_CAP = 0,
+	PDS_FWCTL_SEND_CAP,
+};
+#endif /* _UAPI_FWCTL_PDS_H_ */
-- 
2.17.1

Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Jonathan Cameron]

On Tue, 11 Feb 2025 15:48:52 -0800
Shannon Nelson <shannon.nelson@amd.com> wrote:

> Initial files for adding a new fwctl driver for the AMD/Pensando PDS
> devices.  This sets up a simple auxiliary_bus driver that registers
> with fwctl subsystem.  It expects that a pds_core device has set up
> the auxiliary_device pds_core.fwctl
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
Hi Shannon,
A few comments inline

Jonathan



> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
> new file mode 100644
> index 000000000000..24979fe0deea
> --- /dev/null
> +++ b/drivers/fwctl/pds/main.c
> @@ -0,0 +1,195 @@

> +
> +static int pdsfc_identify(struct pdsfc_dev *pdsfc)
> +{
> +	struct device *dev = &pdsfc->fwctl.dev;
> +	union pds_core_adminq_comp comp = {0};
> +	union pds_core_adminq_cmd cmd = {0};
> +	struct pds_fwctl_ident *ident;
> +	dma_addr_t ident_pa;
> +	int err = 0;
> +
> +	ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL);
> +	err = dma_mapping_error(dev->parent, ident_pa);
> +	if (err) {
> +		dev_err(dev, "Failed to map ident\n");
> +		return err;
> +	}
> +
> +	cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
> +	cmd.fwctl_ident.version = 0;
> +	cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident));
> +	cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa);

Could save intializing cmd above and do it here where all
the data is available.

	cmd = (union pds_core_adminq_cmd) {
		.fwctl_ident = {
			.opcode = ...
etc. Up to you.


	}
> +
> +	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
> +	if (err) {
> +		dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa);
> +		dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
> +			cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
> +		return err;
> +	}
> +
> +	pdsfc->ident = ident;
> +	pdsfc->ident_pa = ident_pa;

I guess it will become clear in later patches, but I'm not immediately sure why
it makes sense to keep a copy of ident and the dma mappings live etc.
Does it change at runtime?


> +
> +	dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n",
> +		ident->version, ident->max_req_sz, ident->max_resp_sz,
> +		ident->max_req_sg_elems, ident->max_resp_sg_elems);
> +
> +	return 0;
> +}

> +static int pdsfc_probe(struct auxiliary_device *adev,
> +		       const struct auxiliary_device_id *id)
> +{
> +	struct pdsfc_dev *pdsfc __free(pdsfc_dev);
Convention for these is to put the constructor and destructor definitions
on one line.  I'm too lazy to find the email from Linus where he
specified this but Dan did add docs to cleanup.h.
https://elixir.bootlin.com/linux/v6.14-rc2/source/include/linux/cleanup.h#L129
is referring to setting this to NULL, which is minimum that should be done
as future code changes might mean there is a failure path between
declaration and use.  Anyhow, it argues in favor of inline as shown
below.


> +	struct pds_auxiliary_dev *padev;
> +	struct device *dev = &adev->dev;
> +	int err = 0;
Set in all paths where it is used so no need to set it here.

> +
> +	padev = container_of(adev, struct pds_auxiliary_dev, aux_dev);
	struct pdsfc_dev *pdsfc __free(pdsfc_dev) = 
 		fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
> +				   struct pdsfc_dev, fwctl);
> +	pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
> +				   struct pdsfc_dev, fwctl);
> +	if (!pdsfc) {
> +		dev_err(dev, "Failed to allocate fwctl device struct\n");
> +		return -ENOMEM;
> +	}
> +	pdsfc->padev = padev;
> +
> +	err = pdsfc_identify(pdsfc);
> +	if (err) {
> +		dev_err(dev, "Failed to identify device, err %d\n", err);
> +		return err;

Perhaps use return dev_err_probe() just to get the pretty printing.
Note though that it won't print for enomem cases.

> +	}
> +
> +	err = fwctl_register(&pdsfc->fwctl);
> +	if (err) {
> +		dev_err(dev, "Failed to register device, err %d\n", err);
> +		return err;
> +	}
> +
> +	auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
> +
> +	return 0;
> +
> +free_ident:

Nothing goes here. Which is good as missing __free magic with gotos
is a recipe for pain.

> +	pdsfc_free_ident(pdsfc);
> +	return err;
> +}
> +
> +static void pdsfc_remove(struct auxiliary_device *adev)
> +{
> +	struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
> +
> +	fwctl_unregister(&pdsfc->fwctl);
> +	pdsfc_free_ident(pdsfc);
> +}

> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
> index 4b4e9a98b37b..7fc353b63353 100644
> --- a/include/linux/pds/pds_adminq.h
> +++ b/include/linux/pds/pds_adminq.h
> @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd {
>  	u8     status;
>  };
>  
> +enum pds_fwctl_cmd_opcode {
> +	PDS_FWCTL_CMD_IDENT		= 70,
> +};
> +
> +/**
> + * struct pds_fwctl_cmd - Firmware control command structure
> + * @opcode: Opcode
> + * @rsvd:   Word boundary padding
> + * @ep:     Endpoint identifier.
> + * @op:     Operation identifier.
> + */
> +struct pds_fwctl_cmd {
> +	u8     opcode;
> +	u8     rsvd[3];
> +	__le32 ep;
> +	__le32 op;
> +} __packed;
None of these actually need to be packed given explicit padding to
natural alignment of all fields.  Arguably it does no harm though
so up to you.

> +
> +/**
> + * struct pds_fwctl_comp - Firmware control completion structure
> + * @status:     Status of the firmware control operation
> + * @rsvd:       Word boundary padding
> + * @comp_index: Completion index in little-endian format
> + * @rsvd2:      Word boundary padding
> + * @color:      Color bit indicating the state of the completion
> + */
> +struct pds_fwctl_comp {
> +	u8     status;
> +	u8     rsvd;
> +	__le16 comp_index;
> +	u8     rsvd2[11];
> +	u8     color;
> +} __packed;

Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Nelson, Shannon]

On 2/12/2025 4:22 AM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 15:48:52 -0800
> Shannon Nelson <shannon.nelson@amd.com> wrote:
> 
>> Initial files for adding a new fwctl driver for the AMD/Pensando PDS
>> devices.  This sets up a simple auxiliary_bus driver that registers
>> with fwctl subsystem.  It expects that a pds_core device has set up
>> the auxiliary_device pds_core.fwctl
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
> Hi Shannon,
> A few comments inline
> 
> Jonathan
> 
> 
> 
>> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
>> new file mode 100644
>> index 000000000000..24979fe0deea
>> --- /dev/null
>> +++ b/drivers/fwctl/pds/main.c
>> @@ -0,0 +1,195 @@
> 
>> +
>> +static int pdsfc_identify(struct pdsfc_dev *pdsfc)
>> +{
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     union pds_core_adminq_comp comp = {0};
>> +     union pds_core_adminq_cmd cmd = {0};
>> +     struct pds_fwctl_ident *ident;
>> +     dma_addr_t ident_pa;
>> +     int err = 0;
>> +
>> +     ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL);
>> +     err = dma_mapping_error(dev->parent, ident_pa);
>> +     if (err) {
>> +             dev_err(dev, "Failed to map ident\n");
>> +             return err;
>> +     }
>> +
>> +     cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
>> +     cmd.fwctl_ident.version = 0;
>> +     cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident));
>> +     cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa);
> 
> Could save intializing cmd above and do it here where all
> the data is available.
> 
>          cmd = (union pds_core_adminq_cmd) {
>                  .fwctl_ident = {
>                          .opcode = ...
> etc. Up to you.
> 
> 
>          }

Yeah, there are a lot of ways to do this.  In a lot of the ionic code we 
do a bunch of the init in the declaration and add to the more dynamic 
field values later in the function.  Let me think on this a little...


>> +
>> +     err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> +     if (err) {
>> +             dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa);
>> +             dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
>> +                     cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
>> +             return err;
>> +     }
>> +
>> +     pdsfc->ident = ident;
>> +     pdsfc->ident_pa = ident_pa;
> 
> I guess it will become clear in later patches, but I'm not immediately sure why
> it makes sense to keep a copy of ident and the dma mappings live etc.
> Does it change at runtime?

No, actually this doesn't change at runtime, we could copy it into a 
local storage and drop the DMA mapping, which will relieve us of the 
need to remember to clean it up later.


> 
> 
>> +
>> +     dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n",
>> +             ident->version, ident->max_req_sz, ident->max_resp_sz,
>> +             ident->max_req_sg_elems, ident->max_resp_sg_elems);
>> +
>> +     return 0;
>> +}
> 
>> +static int pdsfc_probe(struct auxiliary_device *adev,
>> +                    const struct auxiliary_device_id *id)
>> +{
>> +     struct pdsfc_dev *pdsfc __free(pdsfc_dev);
> Convention for these is to put the constructor and destructor definitions
> on one line.  I'm too lazy to find the email from Linus where he
> specified this but Dan did add docs to cleanup.h.
> https://elixir.bootlin.com/linux/v6.14-rc2/source/include/linux/cleanup.h#L129
> is referring to setting this to NULL, which is minimum that should be done
> as future code changes might mean there is a failure path between
> declaration and use.  Anyhow, it argues in favor of inline as shown
> below.

Obviously we're still new to this pattern - I'll look at fixing these 
things up.

> 
> 
>> +     struct pds_auxiliary_dev *padev;
>> +     struct device *dev = &adev->dev;
>> +     int err = 0;
> Set in all paths where it is used so no need to set it here.

Got it.

> 
>> +
>> +     padev = container_of(adev, struct pds_auxiliary_dev, aux_dev);
>          struct pdsfc_dev *pdsfc __free(pdsfc_dev) =
>                  fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
>> +                                struct pdsfc_dev, fwctl);
>> +     pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
>> +                                struct pdsfc_dev, fwctl);
>> +     if (!pdsfc) {
>> +             dev_err(dev, "Failed to allocate fwctl device struct\n");
>> +             return -ENOMEM;
>> +     }
>> +     pdsfc->padev = padev;
>> +
>> +     err = pdsfc_identify(pdsfc);
>> +     if (err) {
>> +             dev_err(dev, "Failed to identify device, err %d\n", err);
>> +             return err;
> 
> Perhaps use return dev_err_probe() just to get the pretty printing.
> Note though that it won't print for enomem cases.

Sure

> 
>> +     }
>> +
>> +     err = fwctl_register(&pdsfc->fwctl);
>> +     if (err) {
>> +             dev_err(dev, "Failed to register device, err %d\n", err);
>> +             return err;
>> +     }
>> +
>> +     auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
>> +
>> +     return 0;
>> +
>> +free_ident:
> 
> Nothing goes here. Which is good as missing __free magic with gotos
> is a recipe for pain.

The above ident change will remove the need for this.

> 
>> +     pdsfc_free_ident(pdsfc);
>> +     return err;
>> +}
>> +
>> +static void pdsfc_remove(struct auxiliary_device *adev)
>> +{
>> +     struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>> +
>> +     fwctl_unregister(&pdsfc->fwctl);
>> +     pdsfc_free_ident(pdsfc);
>> +}
> 
>> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
>> index 4b4e9a98b37b..7fc353b63353 100644
>> --- a/include/linux/pds/pds_adminq.h
>> +++ b/include/linux/pds/pds_adminq.h
>> @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd {
>>        u8     status;
>>   };
>>
>> +enum pds_fwctl_cmd_opcode {
>> +     PDS_FWCTL_CMD_IDENT             = 70,
>> +};
>> +
>> +/**
>> + * struct pds_fwctl_cmd - Firmware control command structure
>> + * @opcode: Opcode
>> + * @rsvd:   Word boundary padding
>> + * @ep:     Endpoint identifier.
>> + * @op:     Operation identifier.
>> + */
>> +struct pds_fwctl_cmd {
>> +     u8     opcode;
>> +     u8     rsvd[3];
>> +     __le32 ep;
>> +     __le32 op;
>> +} __packed;
> None of these actually need to be packed given explicit padding to
> natural alignment of all fields.  Arguably it does no harm though
> so up to you.

Old belt-and-suspenders habits...

Since this is the common style on the rest of the interface definitions, 
I'd prefer to keep it for now.

sln


> 
>> +
>> +/**
>> + * struct pds_fwctl_comp - Firmware control completion structure
>> + * @status:     Status of the firmware control operation
>> + * @rsvd:       Word boundary padding
>> + * @comp_index: Completion index in little-endian format
>> + * @rsvd2:      Word boundary padding
>> + * @color:      Color bit indicating the state of the completion
>> + */
>> +struct pds_fwctl_comp {
>> +     u8     status;
>> +     u8     rsvd;
>> +     __le16 comp_index;
>> +     u8     rsvd2[11];
>> +     u8     color;
>> +} __packed;
> 

Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Jason Gunthorpe]

On Thu, Feb 13, 2025 at 03:06:14PM -0800, Nelson, Shannon wrote:

> > > +/**
> > > + * struct pds_fwctl_cmd - Firmware control command structure
> > > + * @opcode: Opcode
> > > + * @rsvd:   Word boundary padding
> > > + * @ep:     Endpoint identifier.
> > > + * @op:     Operation identifier.
> > > + */
> > > +struct pds_fwctl_cmd {
> > > +     u8     opcode;
> > > +     u8     rsvd[3];
> > > +     __le32 ep;
> > > +     __le32 op;
> > > +} __packed;
> > None of these actually need to be packed given explicit padding to
> > natural alignment of all fields.  Arguably it does no harm though
> > so up to you.
> 
> Old belt-and-suspenders habits...

In that case it is worth knowing that __packed also changes the
assumed alignment of the struct. You can access a __packed struct at
any byte.

On x86 this is mostly meaningless, but on other arches it can effect
code generation as the compiler will have to assume that, say, a 64
bit load is not naturally aligned and emit a more expensive sequence
to load it.

Which is why you occasionally see things like:
  __attribute__ ((packed,aligned(8)));

Which says that there is no padding inside the struct, but also that
the compiler can assume a guaranteed starting alignment for the
memory.

Jason

Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Dave Jiang]

On 2/11/25 4:48 PM, Shannon Nelson wrote:
> Initial files for adding a new fwctl driver for the AMD/Pensando PDS
> devices.  This sets up a simple auxiliary_bus driver that registers
> with fwctl subsystem.  It expects that a pds_core device has set up
> the auxiliary_device pds_core.fwctl
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  MAINTAINERS                    |   7 ++
>  drivers/fwctl/Kconfig          |  10 ++
>  drivers/fwctl/Makefile         |   1 +
>  drivers/fwctl/pds/Makefile     |   4 +
>  drivers/fwctl/pds/main.c       | 195 +++++++++++++++++++++++++++++++++
>  include/linux/pds/pds_adminq.h |  77 +++++++++++++
>  include/uapi/fwctl/fwctl.h     |   1 +
>  include/uapi/fwctl/pds.h       |  27 +++++
>  8 files changed, 322 insertions(+)
>  create mode 100644 drivers/fwctl/pds/Makefile
>  create mode 100644 drivers/fwctl/pds/main.c
>  create mode 100644 include/uapi/fwctl/pds.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 413ab79bf2f4..123f8a9c0b26 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9602,6 +9602,13 @@ T:	git git://linuxtv.org/media.git
>  F:	Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
>  F:	drivers/media/i2c/gc2145.c
>  
> +FWCTL PDS DRIVER
> +M:	Brett Creeley <brett.creeley@amd.com>
> +R:	Shannon Nelson <shannon.nelson@amd.com>
> +L:	linux-kernel@vger.kernel.org
> +S:	Maintained
> +F:	drivers/fwctl/pds/
> +
>  GATEWORKS SYSTEM CONTROLLER (GSC) DRIVER
>  M:	Tim Harvey <tharvey@gateworks.com>
>  S:	Maintained
> diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
> index 0a542a247303..df87ce5bd8aa 100644
> --- a/drivers/fwctl/Kconfig
> +++ b/drivers/fwctl/Kconfig
> @@ -28,5 +28,15 @@ config FWCTL_MLX5
>  	  This will allow configuration and debug tools to work out of the box on
>  	  mainstream kernel.
>  
> +	  If you don't know what to do here, say N.
> +
> +config FWCTL_PDS
> +	tristate "AMD/Pensando pds fwctl driver"
> +	depends on PDS_CORE
> +	help
> +	  The pds_fwctl driver provides an fwctl interface for a user process
> +	  to access the debug and configuration information of the AMD/Pensando
> +	  DSC hardware family.
> +
>  	  If you don't know what to do here, say N.
>  endif
> diff --git a/drivers/fwctl/Makefile b/drivers/fwctl/Makefile
> index 5fb289243286..692e4b8d7beb 100644
> --- a/drivers/fwctl/Makefile
> +++ b/drivers/fwctl/Makefile
> @@ -2,5 +2,6 @@
>  obj-$(CONFIG_FWCTL) += fwctl.o
>  obj-$(CONFIG_FWCTL_BNXT) += bnxt/
>  obj-$(CONFIG_FWCTL_MLX5) += mlx5/
> +obj-$(CONFIG_FWCTL_PDS) += pds/
>  
>  fwctl-y += main.o
> diff --git a/drivers/fwctl/pds/Makefile b/drivers/fwctl/pds/Makefile
> new file mode 100644
> index 000000000000..c14cba128e3b
> --- /dev/null
> +++ b/drivers/fwctl/pds/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +obj-$(CONFIG_FWCTL_PDS) += pds_fwctl.o
> +
> +pds_fwctl-y += main.o
> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
> new file mode 100644
> index 000000000000..24979fe0deea
> --- /dev/null
> +++ b/drivers/fwctl/pds/main.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/* Copyright(c) Advanced Micro Devices, Inc */
> +
> +#include <linux/module.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/pci.h>
> +#include <linux/vmalloc.h>
> +
> +#include <uapi/fwctl/fwctl.h>
> +#include <uapi/fwctl/pds.h>
> +#include <linux/fwctl.h>
> +
> +#include <linux/pds/pds_common.h>
> +#include <linux/pds/pds_core_if.h>
> +#include <linux/pds/pds_adminq.h>
> +#include <linux/pds/pds_auxbus.h>
> +
> +struct pdsfc_uctx {
> +	struct fwctl_uctx uctx;
> +	u32 uctx_caps;
> +	u32 uctx_uid;
> +};
> +
> +struct pdsfc_dev {
> +	struct fwctl_device fwctl;
> +	struct pds_auxiliary_dev *padev;
> +	struct pdsc *pdsc;
> +	u32 caps;
> +	dma_addr_t ident_pa;
> +	struct pds_fwctl_ident *ident;
> +};
> +DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));
> +
> +static int pdsfc_open_uctx(struct fwctl_uctx *uctx)
> +{
> +	struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
> +	struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
> +	struct device *dev = &uctx->fwctl->dev;
> +
> +	dev_dbg(dev, "%s: caps = 0x%04x\n", __func__, pdsfc->caps);
> +	pdsfc_uctx->uctx_caps = pdsfc->caps;
> +
> +	return 0;
> +}
> +
> +static void pdsfc_close_uctx(struct fwctl_uctx *uctx)
> +{
> +}
> +
> +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length)
> +{
> +	struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
> +	struct fwctl_info_pds *info;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return ERR_PTR(-ENOMEM);
> +
> +	info->uctx_caps = pdsfc_uctx->uctx_caps;
> +
> +	return info;
> +}
> +
> +static void pdsfc_free_ident(struct pdsfc_dev *pdsfc)
> +{
> +	struct device *dev = &pdsfc->fwctl.dev;
> +
> +	if (pdsfc->ident) {
> +		dma_free_coherent(dev, sizeof(*pdsfc->ident),
> +				  pdsfc->ident, pdsfc->ident_pa);
> +		pdsfc->ident = NULL;
> +		pdsfc->ident_pa = DMA_MAPPING_ERROR;
> +	}
> +}
> +
> +static int pdsfc_identify(struct pdsfc_dev *pdsfc)
> +{
> +	struct device *dev = &pdsfc->fwctl.dev;
> +	union pds_core_adminq_comp comp = {0};
> +	union pds_core_adminq_cmd cmd = {0};
> +	struct pds_fwctl_ident *ident;
> +	dma_addr_t ident_pa;
> +	int err = 0;
> +
> +	ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL);
> +	err = dma_mapping_error(dev->parent, ident_pa);
> +	if (err) {
> +		dev_err(dev, "Failed to map ident\n");
> +		return err;
> +	}
> +
> +	cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
> +	cmd.fwctl_ident.version = 0;
> +	cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident));
> +	cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa);
> +
> +	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
> +	if (err) {
> +		dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa);
> +		dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
> +			cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
> +		return err;
> +	}
> +
> +	pdsfc->ident = ident;
> +	pdsfc->ident_pa = ident_pa;
> +
> +	dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n",
> +		ident->version, ident->max_req_sz, ident->max_resp_sz,
> +		ident->max_req_sg_elems, ident->max_resp_sg_elems);
> +
> +	return 0;
> +}
> +
> +static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> +			  void *in, size_t in_len, size_t *out_len)
> +{
> +	return NULL;
> +}
> +
> +static const struct fwctl_ops pdsfc_ops = {
> +	.device_type = FWCTL_DEVICE_TYPE_PDS,
> +	.uctx_size = sizeof(struct pdsfc_uctx),
> +	.open_uctx = pdsfc_open_uctx,
> +	.close_uctx = pdsfc_close_uctx,
> +	.info = pdsfc_info,
> +	.fw_rpc = pdsfc_fw_rpc,
> +};
> +
> +static int pdsfc_probe(struct auxiliary_device *adev,
> +		       const struct auxiliary_device_id *id)
> +{
> +	struct pdsfc_dev *pdsfc __free(pdsfc_dev);
> +	struct pds_auxiliary_dev *padev;
> +	struct device *dev = &adev->dev;
> +	int err = 0;
> +
> +	padev = container_of(adev, struct pds_auxiliary_dev, aux_dev);
> +	pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
> +				   struct pdsfc_dev, fwctl);

The suggested formatting of using scope-based cleanup is to just declare the variable inline as you need it rather than split the cleanup vs the allocation.

	struct pdsfc_dev *pdsfc __free(pdsfc_dev) =
			fwctl_alloc_device(...); 

> +	if (!pdsfc) {
> +		dev_err(dev, "Failed to allocate fwctl device struct\n");
> +		return -ENOMEM;
> +	}
> +	pdsfc->padev = padev;
> +
> +	err = pdsfc_identify(pdsfc);
> +	if (err) {
> +		dev_err(dev, "Failed to identify device, err %d\n", err);> +		return err;
> +	}
> +
> +	err = fwctl_register(&pdsfc->fwctl);
> +	if (err) {
> +		dev_err(dev, "Failed to register device, err %d\n", err);

Missing freeing of the 'ident' from dma_alloc_coherent. Although mixing gotos and __free() would get really messy in a hurry. I suggest you setup pdsfc_identify() like an allocation function with returning 'struct pds_fwctl_ident *' so you can utilize a custom __free() to free up the dma memory. and then you can do:
pdsfc->ident = no_free_ptr(ident);
pdsfc->ident_pa = ident_pa; /* ident_pa passed in as a ptr to pdsfc_identify() for write back */

> +		return err;
> +	}
> +
> +	auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
> +
> +	return 0;
> +
> +free_ident:

nothing goes here.

DJ

> +	pdsfc_free_ident(pdsfc);
> +	return err;
> +}
> +
> +static void pdsfc_remove(struct auxiliary_device *adev)
> +{
> +	struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
> +
> +	fwctl_unregister(&pdsfc->fwctl);
> +	pdsfc_free_ident(pdsfc);
> +}
> +
> +static const struct auxiliary_device_id pdsfc_id_table[] = {
> +	{.name = PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_FWCTL_STR },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, pdsfc_id_table);
> +
> +static struct auxiliary_driver pdsfc_driver = {
> +	.name = "pds_fwctl",
> +	.probe = pdsfc_probe,
> +	.remove = pdsfc_remove,
> +	.id_table = pdsfc_id_table,
> +};
> +
> +module_auxiliary_driver(pdsfc_driver);
> +
> +MODULE_IMPORT_NS(FWCTL);
> +MODULE_DESCRIPTION("pds fwctl driver");
> +MODULE_AUTHOR("Shannon Nelson <shannon.nelson@amd.com>");
> +MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>");
> +MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
> index 4b4e9a98b37b..7fc353b63353 100644
> --- a/include/linux/pds/pds_adminq.h
> +++ b/include/linux/pds/pds_adminq.h
> @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd {
>  	u8     status;
>  };
>  
> +enum pds_fwctl_cmd_opcode {
> +	PDS_FWCTL_CMD_IDENT		= 70,
> +};
> +
> +/**
> + * struct pds_fwctl_cmd - Firmware control command structure
> + * @opcode: Opcode
> + * @rsvd:   Word boundary padding
> + * @ep:     Endpoint identifier.
> + * @op:     Operation identifier.
> + */
> +struct pds_fwctl_cmd {
> +	u8     opcode;
> +	u8     rsvd[3];
> +	__le32 ep;
> +	__le32 op;
> +} __packed;
> +
> +/**
> + * struct pds_fwctl_comp - Firmware control completion structure
> + * @status:     Status of the firmware control operation
> + * @rsvd:       Word boundary padding
> + * @comp_index: Completion index in little-endian format
> + * @rsvd2:      Word boundary padding
> + * @color:      Color bit indicating the state of the completion
> + */
> +struct pds_fwctl_comp {
> +	u8     status;
> +	u8     rsvd;
> +	__le16 comp_index;
> +	u8     rsvd2[11];
> +	u8     color;
> +} __packed;
> +
> +/**
> + * struct pds_fwctl_ident_cmd - Firmware control identification command structure
> + * @opcode:   Operation code for the command
> + * @rsvd:     Word boundary padding
> + * @version:  Interface version
> + * @rsvd2:    Word boundary padding
> + * @len:      Length of the identification data
> + * @ident_pa: Physical address of the identification data
> + */
> +struct pds_fwctl_ident_cmd {
> +	u8     opcode;
> +	u8     rsvd;
> +	u8     version;
> +	u8     rsvd2;
> +	__le32 len;
> +	__le64 ident_pa;
> +} __packed;
> +
> +/**
> + * struct pds_fwctl_ident - Firmware control identification structure
> + * @features:    Supported features
> + * @version:     Interface version
> + * @rsvd:        Word boundary padding
> + * @max_req_sz:  Maximum request size
> + * @max_resp_sz: Maximum response size
> + * @max_req_sg_elems:  Maximum number of request SGs
> + * @max_resp_sg_elems: Maximum number of response SGs
> + */
> +struct pds_fwctl_ident {
> +	__le64 features;
> +	u8     version;
> +	u8     rsvd[3];
> +	__le32 max_req_sz;
> +	__le32 max_resp_sz;
> +	u8     max_req_sg_elems;
> +	u8     max_resp_sg_elems;
> +} __packed;
> +
>  union pds_core_adminq_cmd {
>  	u8     opcode;
>  	u8     bytes[64];
> @@ -1216,6 +1288,9 @@ union pds_core_adminq_cmd {
>  	struct pds_lm_dirty_enable_cmd	  lm_dirty_enable;
>  	struct pds_lm_dirty_disable_cmd	  lm_dirty_disable;
>  	struct pds_lm_dirty_seq_ack_cmd	  lm_dirty_seq_ack;
> +
> +	struct pds_fwctl_cmd		  fwctl;
> +	struct pds_fwctl_ident_cmd	  fwctl_ident;
>  };
>  
>  union pds_core_adminq_comp {
> @@ -1243,6 +1318,8 @@ union pds_core_adminq_comp {
>  
>  	struct pds_lm_state_size_comp	  lm_state_size;
>  	struct pds_lm_dirty_status_comp	  lm_dirty_status;
> +
> +	struct pds_fwctl_comp		  fwctl;
>  };
>  
>  #ifndef __CHECKER__
> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
> index 518f054f02d2..a884e9f6dc2c 100644
> --- a/include/uapi/fwctl/fwctl.h
> +++ b/include/uapi/fwctl/fwctl.h
> @@ -44,6 +44,7 @@ enum fwctl_device_type {
>  	FWCTL_DEVICE_TYPE_ERROR = 0,
>  	FWCTL_DEVICE_TYPE_MLX5 = 1,
>  	FWCTL_DEVICE_TYPE_BNXT = 3,
> +	FWCTL_DEVICE_TYPE_PDS = 4,
>  };
>  
>  /**
> diff --git a/include/uapi/fwctl/pds.h b/include/uapi/fwctl/pds.h
> new file mode 100644
> index 000000000000..a01b032cbdb1
> --- /dev/null
> +++ b/include/uapi/fwctl/pds.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/* Copyright(c) Advanced Micro Devices, Inc */
> +
> +/*
> + * fwctl interface info for pds_fwctl
> + */
> +
> +#ifndef _UAPI_FWCTL_PDS_H_
> +#define _UAPI_FWCTL_PDS_H_
> +
> +#include <linux/types.h>
> +
> +/*
> + * struct fwctl_info_pds
> + *
> + * Return basic information about the FW interface available.
> + */
> +struct fwctl_info_pds {
> +	__u32 uid;
> +	__u32 uctx_caps;
> +};
> +
> +enum pds_fwctl_capabilities {
> +	PDS_FWCTL_QUERY_CAP = 0,
> +	PDS_FWCTL_SEND_CAP,
> +};
> +#endif /* _UAPI_FWCTL_PDS_H_ */

Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Nelson, Shannon]

On 2/12/2025 3:26 PM, Dave Jiang wrote:
> 
> On 2/11/25 4:48 PM, Shannon Nelson wrote:
>> Initial files for adding a new fwctl driver for the AMD/Pensando PDS
>> devices.  This sets up a simple auxiliary_bus driver that registers
>> with fwctl subsystem.  It expects that a pds_core device has set up
>> the auxiliary_device pds_core.fwctl
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>>   MAINTAINERS                    |   7 ++
>>   drivers/fwctl/Kconfig          |  10 ++
>>   drivers/fwctl/Makefile         |   1 +
>>   drivers/fwctl/pds/Makefile     |   4 +
>>   drivers/fwctl/pds/main.c       | 195 +++++++++++++++++++++++++++++++++
>>   include/linux/pds/pds_adminq.h |  77 +++++++++++++
>>   include/uapi/fwctl/fwctl.h     |   1 +
>>   include/uapi/fwctl/pds.h       |  27 +++++
>>   8 files changed, 322 insertions(+)
>>   create mode 100644 drivers/fwctl/pds/Makefile
>>   create mode 100644 drivers/fwctl/pds/main.c
>>   create mode 100644 include/uapi/fwctl/pds.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 413ab79bf2f4..123f8a9c0b26 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9602,6 +9602,13 @@ T:     git git://linuxtv.org/media.git
>>   F:   Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
>>   F:   drivers/media/i2c/gc2145.c
>>
>> +FWCTL PDS DRIVER
>> +M:   Brett Creeley <brett.creeley@amd.com>
>> +R:   Shannon Nelson <shannon.nelson@amd.com>
>> +L:   linux-kernel@vger.kernel.org
>> +S:   Maintained
>> +F:   drivers/fwctl/pds/
>> +
>>   GATEWORKS SYSTEM CONTROLLER (GSC) DRIVER
>>   M:   Tim Harvey <tharvey@gateworks.com>
>>   S:   Maintained
>> diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
>> index 0a542a247303..df87ce5bd8aa 100644
>> --- a/drivers/fwctl/Kconfig
>> +++ b/drivers/fwctl/Kconfig
>> @@ -28,5 +28,15 @@ config FWCTL_MLX5
>>          This will allow configuration and debug tools to work out of the box on
>>          mainstream kernel.
>>
>> +       If you don't know what to do here, say N.
>> +
>> +config FWCTL_PDS
>> +     tristate "AMD/Pensando pds fwctl driver"
>> +     depends on PDS_CORE
>> +     help
>> +       The pds_fwctl driver provides an fwctl interface for a user process
>> +       to access the debug and configuration information of the AMD/Pensando
>> +       DSC hardware family.
>> +
>>          If you don't know what to do here, say N.
>>   endif
>> diff --git a/drivers/fwctl/Makefile b/drivers/fwctl/Makefile
>> index 5fb289243286..692e4b8d7beb 100644
>> --- a/drivers/fwctl/Makefile
>> +++ b/drivers/fwctl/Makefile
>> @@ -2,5 +2,6 @@
>>   obj-$(CONFIG_FWCTL) += fwctl.o
>>   obj-$(CONFIG_FWCTL_BNXT) += bnxt/
>>   obj-$(CONFIG_FWCTL_MLX5) += mlx5/
>> +obj-$(CONFIG_FWCTL_PDS) += pds/
>>
>>   fwctl-y += main.o
>> diff --git a/drivers/fwctl/pds/Makefile b/drivers/fwctl/pds/Makefile
>> new file mode 100644
>> index 000000000000..c14cba128e3b
>> --- /dev/null
>> +++ b/drivers/fwctl/pds/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
>> +obj-$(CONFIG_FWCTL_PDS) += pds_fwctl.o
>> +
>> +pds_fwctl-y += main.o
>> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
>> new file mode 100644
>> index 000000000000..24979fe0deea
>> --- /dev/null
>> +++ b/drivers/fwctl/pds/main.c
>> @@ -0,0 +1,195 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
>> +/* Copyright(c) Advanced Micro Devices, Inc */
>> +
>> +#include <linux/module.h>
>> +#include <linux/auxiliary_bus.h>
>> +#include <linux/pci.h>
>> +#include <linux/vmalloc.h>
>> +
>> +#include <uapi/fwctl/fwctl.h>
>> +#include <uapi/fwctl/pds.h>
>> +#include <linux/fwctl.h>
>> +
>> +#include <linux/pds/pds_common.h>
>> +#include <linux/pds/pds_core_if.h>
>> +#include <linux/pds/pds_adminq.h>
>> +#include <linux/pds/pds_auxbus.h>
>> +
>> +struct pdsfc_uctx {
>> +     struct fwctl_uctx uctx;
>> +     u32 uctx_caps;
>> +     u32 uctx_uid;
>> +};
>> +
>> +struct pdsfc_dev {
>> +     struct fwctl_device fwctl;
>> +     struct pds_auxiliary_dev *padev;
>> +     struct pdsc *pdsc;
>> +     u32 caps;
>> +     dma_addr_t ident_pa;
>> +     struct pds_fwctl_ident *ident;
>> +};
>> +DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));
>> +
>> +static int pdsfc_open_uctx(struct fwctl_uctx *uctx)
>> +{
>> +     struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
>> +     struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
>> +     struct device *dev = &uctx->fwctl->dev;
>> +
>> +     dev_dbg(dev, "%s: caps = 0x%04x\n", __func__, pdsfc->caps);
>> +     pdsfc_uctx->uctx_caps = pdsfc->caps;
>> +
>> +     return 0;
>> +}
>> +
>> +static void pdsfc_close_uctx(struct fwctl_uctx *uctx)
>> +{
>> +}
>> +
>> +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length)
>> +{
>> +     struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
>> +     struct fwctl_info_pds *info;
>> +
>> +     info = kzalloc(sizeof(*info), GFP_KERNEL);
>> +     if (!info)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     info->uctx_caps = pdsfc_uctx->uctx_caps;
>> +
>> +     return info;
>> +}
>> +
>> +static void pdsfc_free_ident(struct pdsfc_dev *pdsfc)
>> +{
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +
>> +     if (pdsfc->ident) {
>> +             dma_free_coherent(dev, sizeof(*pdsfc->ident),
>> +                               pdsfc->ident, pdsfc->ident_pa);
>> +             pdsfc->ident = NULL;
>> +             pdsfc->ident_pa = DMA_MAPPING_ERROR;
>> +     }
>> +}
>> +
>> +static int pdsfc_identify(struct pdsfc_dev *pdsfc)
>> +{
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     union pds_core_adminq_comp comp = {0};
>> +     union pds_core_adminq_cmd cmd = {0};
>> +     struct pds_fwctl_ident *ident;
>> +     dma_addr_t ident_pa;
>> +     int err = 0;
>> +
>> +     ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL);
>> +     err = dma_mapping_error(dev->parent, ident_pa);
>> +     if (err) {
>> +             dev_err(dev, "Failed to map ident\n");
>> +             return err;
>> +     }
>> +
>> +     cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
>> +     cmd.fwctl_ident.version = 0;
>> +     cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident));
>> +     cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa);
>> +
>> +     err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> +     if (err) {
>> +             dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa);
>> +             dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
>> +                     cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
>> +             return err;
>> +     }
>> +
>> +     pdsfc->ident = ident;
>> +     pdsfc->ident_pa = ident_pa;
>> +
>> +     dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n",
>> +             ident->version, ident->max_req_sz, ident->max_resp_sz,
>> +             ident->max_req_sg_elems, ident->max_resp_sg_elems);
>> +
>> +     return 0;
>> +}
>> +
>> +static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
>> +                       void *in, size_t in_len, size_t *out_len)
>> +{
>> +     return NULL;
>> +}
>> +
>> +static const struct fwctl_ops pdsfc_ops = {
>> +     .device_type = FWCTL_DEVICE_TYPE_PDS,
>> +     .uctx_size = sizeof(struct pdsfc_uctx),
>> +     .open_uctx = pdsfc_open_uctx,
>> +     .close_uctx = pdsfc_close_uctx,
>> +     .info = pdsfc_info,
>> +     .fw_rpc = pdsfc_fw_rpc,
>> +};
>> +
>> +static int pdsfc_probe(struct auxiliary_device *adev,
>> +                    const struct auxiliary_device_id *id)
>> +{
>> +     struct pdsfc_dev *pdsfc __free(pdsfc_dev);
>> +     struct pds_auxiliary_dev *padev;
>> +     struct device *dev = &adev->dev;
>> +     int err = 0;
>> +
>> +     padev = container_of(adev, struct pds_auxiliary_dev, aux_dev);
>> +     pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
>> +                                struct pdsfc_dev, fwctl);
> 
> The suggested formatting of using scope-based cleanup is to just declare the variable inline as you need it rather than split the cleanup vs the allocation.

I'm old - I have old patterns of distain for mid-function 
declarations... I'll try to do better :-)

> 
>          struct pdsfc_dev *pdsfc __free(pdsfc_dev) =
>                          fwctl_alloc_device(...);
> 
>> +     if (!pdsfc) {
>> +             dev_err(dev, "Failed to allocate fwctl device struct\n");
>> +             return -ENOMEM;
>> +     }
>> +     pdsfc->padev = padev;
>> +
>> +     err = pdsfc_identify(pdsfc);
>> +     if (err) {
>> +             dev_err(dev, "Failed to identify device, err %d\n", err);> +            return err;
>> +     }
>> +
>> +     err = fwctl_register(&pdsfc->fwctl);
>> +     if (err) {
>> +             dev_err(dev, "Failed to register device, err %d\n", err);
> 
> Missing freeing of the 'ident' from dma_alloc_coherent. Although mixing gotos and __free() would get really messy in a hurry. I suggest you setup pdsfc_identify() like an allocation function with returning 'struct pds_fwctl_ident *' so you can utilize a custom __free() to free up the dma memory. and then you can do:
> pdsfc->ident = no_free_ptr(ident);
> pdsfc->ident_pa = ident_pa; /* ident_pa passed in as a ptr to pdsfc_identify() for write back */

As discussed with Jonathan, this will get reworked to not need to keep 
the DMA mapping around.

> 
>> +             return err;
>> +     }
>> +
>> +     auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
>> +
>> +     return 0;
>> +
>> +free_ident:
> 
> nothing goes here.

Thanks!
sln

> 
> DJ
> 
>> +     pdsfc_free_ident(pdsfc);
>> +     return err;
>> +}
>> +
>> +static void pdsfc_remove(struct auxiliary_device *adev)
>> +{
>> +     struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>> +
>> +     fwctl_unregister(&pdsfc->fwctl);
>> +     pdsfc_free_ident(pdsfc);
>> +}
>> +
>> +static const struct auxiliary_device_id pdsfc_id_table[] = {
>> +     {.name = PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_FWCTL_STR },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(auxiliary, pdsfc_id_table);
>> +
>> +static struct auxiliary_driver pdsfc_driver = {
>> +     .name = "pds_fwctl",
>> +     .probe = pdsfc_probe,
>> +     .remove = pdsfc_remove,
>> +     .id_table = pdsfc_id_table,
>> +};
>> +
>> +module_auxiliary_driver(pdsfc_driver);
>> +
>> +MODULE_IMPORT_NS(FWCTL);
>> +MODULE_DESCRIPTION("pds fwctl driver");
>> +MODULE_AUTHOR("Shannon Nelson <shannon.nelson@amd.com>");
>> +MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>");
>> +MODULE_LICENSE("Dual BSD/GPL");
>> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
>> index 4b4e9a98b37b..7fc353b63353 100644
>> --- a/include/linux/pds/pds_adminq.h
>> +++ b/include/linux/pds/pds_adminq.h
>> @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd {
>>        u8     status;
>>   };
>>
>> +enum pds_fwctl_cmd_opcode {
>> +     PDS_FWCTL_CMD_IDENT             = 70,
>> +};
>> +
>> +/**
>> + * struct pds_fwctl_cmd - Firmware control command structure
>> + * @opcode: Opcode
>> + * @rsvd:   Word boundary padding
>> + * @ep:     Endpoint identifier.
>> + * @op:     Operation identifier.
>> + */
>> +struct pds_fwctl_cmd {
>> +     u8     opcode;
>> +     u8     rsvd[3];
>> +     __le32 ep;
>> +     __le32 op;
>> +} __packed;
>> +
>> +/**
>> + * struct pds_fwctl_comp - Firmware control completion structure
>> + * @status:     Status of the firmware control operation
>> + * @rsvd:       Word boundary padding
>> + * @comp_index: Completion index in little-endian format
>> + * @rsvd2:      Word boundary padding
>> + * @color:      Color bit indicating the state of the completion
>> + */
>> +struct pds_fwctl_comp {
>> +     u8     status;
>> +     u8     rsvd;
>> +     __le16 comp_index;
>> +     u8     rsvd2[11];
>> +     u8     color;
>> +} __packed;
>> +
>> +/**
>> + * struct pds_fwctl_ident_cmd - Firmware control identification command structure
>> + * @opcode:   Operation code for the command
>> + * @rsvd:     Word boundary padding
>> + * @version:  Interface version
>> + * @rsvd2:    Word boundary padding
>> + * @len:      Length of the identification data
>> + * @ident_pa: Physical address of the identification data
>> + */
>> +struct pds_fwctl_ident_cmd {
>> +     u8     opcode;
>> +     u8     rsvd;
>> +     u8     version;
>> +     u8     rsvd2;
>> +     __le32 len;
>> +     __le64 ident_pa;
>> +} __packed;
>> +
>> +/**
>> + * struct pds_fwctl_ident - Firmware control identification structure
>> + * @features:    Supported features
>> + * @version:     Interface version
>> + * @rsvd:        Word boundary padding
>> + * @max_req_sz:  Maximum request size
>> + * @max_resp_sz: Maximum response size
>> + * @max_req_sg_elems:  Maximum number of request SGs
>> + * @max_resp_sg_elems: Maximum number of response SGs
>> + */
>> +struct pds_fwctl_ident {
>> +     __le64 features;
>> +     u8     version;
>> +     u8     rsvd[3];
>> +     __le32 max_req_sz;
>> +     __le32 max_resp_sz;
>> +     u8     max_req_sg_elems;
>> +     u8     max_resp_sg_elems;
>> +} __packed;
>> +
>>   union pds_core_adminq_cmd {
>>        u8     opcode;
>>        u8     bytes[64];
>> @@ -1216,6 +1288,9 @@ union pds_core_adminq_cmd {
>>        struct pds_lm_dirty_enable_cmd    lm_dirty_enable;
>>        struct pds_lm_dirty_disable_cmd   lm_dirty_disable;
>>        struct pds_lm_dirty_seq_ack_cmd   lm_dirty_seq_ack;
>> +
>> +     struct pds_fwctl_cmd              fwctl;
>> +     struct pds_fwctl_ident_cmd        fwctl_ident;
>>   };
>>
>>   union pds_core_adminq_comp {
>> @@ -1243,6 +1318,8 @@ union pds_core_adminq_comp {
>>
>>        struct pds_lm_state_size_comp     lm_state_size;
>>        struct pds_lm_dirty_status_comp   lm_dirty_status;
>> +
>> +     struct pds_fwctl_comp             fwctl;
>>   };
>>
>>   #ifndef __CHECKER__
>> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
>> index 518f054f02d2..a884e9f6dc2c 100644
>> --- a/include/uapi/fwctl/fwctl.h
>> +++ b/include/uapi/fwctl/fwctl.h
>> @@ -44,6 +44,7 @@ enum fwctl_device_type {
>>        FWCTL_DEVICE_TYPE_ERROR = 0,
>>        FWCTL_DEVICE_TYPE_MLX5 = 1,
>>        FWCTL_DEVICE_TYPE_BNXT = 3,
>> +     FWCTL_DEVICE_TYPE_PDS = 4,
>>   };
>>
>>   /**
>> diff --git a/include/uapi/fwctl/pds.h b/include/uapi/fwctl/pds.h
>> new file mode 100644
>> index 000000000000..a01b032cbdb1
>> --- /dev/null
>> +++ b/include/uapi/fwctl/pds.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/* Copyright(c) Advanced Micro Devices, Inc */
>> +
>> +/*
>> + * fwctl interface info for pds_fwctl
>> + */
>> +
>> +#ifndef _UAPI_FWCTL_PDS_H_
>> +#define _UAPI_FWCTL_PDS_H_
>> +
>> +#include <linux/types.h>
>> +
>> +/*
>> + * struct fwctl_info_pds
>> + *
>> + * Return basic information about the FW interface available.
>> + */
>> +struct fwctl_info_pds {
>> +     __u32 uid;
>> +     __u32 uctx_caps;
>> +};
>> +
>> +enum pds_fwctl_capabilities {
>> +     PDS_FWCTL_QUERY_CAP = 0,
>> +     PDS_FWCTL_SEND_CAP,
>> +};
>> +#endif /* _UAPI_FWCTL_PDS_H_ */
> 

Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Leon Romanovsky]

On Tue, Feb 11, 2025 at 03:48:52PM -0800, Shannon Nelson wrote:
> Initial files for adding a new fwctl driver for the AMD/Pensando PDS
> devices.  This sets up a simple auxiliary_bus driver that registers
> with fwctl subsystem.  It expects that a pds_core device has set up
> the auxiliary_device pds_core.fwctl
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  MAINTAINERS                    |   7 ++
>  drivers/fwctl/Kconfig          |  10 ++
>  drivers/fwctl/Makefile         |   1 +
>  drivers/fwctl/pds/Makefile     |   4 +
>  drivers/fwctl/pds/main.c       | 195 +++++++++++++++++++++++++++++++++
>  include/linux/pds/pds_adminq.h |  77 +++++++++++++
>  include/uapi/fwctl/fwctl.h     |   1 +
>  include/uapi/fwctl/pds.h       |  27 +++++
>  8 files changed, 322 insertions(+)
>  create mode 100644 drivers/fwctl/pds/Makefile
>  create mode 100644 drivers/fwctl/pds/main.c
>  create mode 100644 include/uapi/fwctl/pds.h

<...>

> +static int pdsfc_open_uctx(struct fwctl_uctx *uctx)
> +{
> +	struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
> +	struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
> +	struct device *dev = &uctx->fwctl->dev;
> +
> +	dev_dbg(dev, "%s: caps = 0x%04x\n", __func__, pdsfc->caps);

This driver is too noisy and has too many debug/err prints.

> +	pdsfc_uctx->uctx_caps = pdsfc->caps;
> +
> +	return 0;
> +}
> +
> +static void pdsfc_close_uctx(struct fwctl_uctx *uctx)
> +{
> +}
> +
> +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length)
> +{
> +	struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
> +	struct fwctl_info_pds *info;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return ERR_PTR(-ENOMEM);
> +
> +	info->uctx_caps = pdsfc_uctx->uctx_caps;
> +
> +	return info;
> +}
> +
> +static void pdsfc_free_ident(struct pdsfc_dev *pdsfc)
> +{
> +	struct device *dev = &pdsfc->fwctl.dev;
> +
> +	if (pdsfc->ident) {

It is not kernel style, which is success oriented.
If (!pdsfc->ident)
   return;

However I don't know how can this happen. You shouldn't call to pdsfc_free_ident
if ident wasn't set.

> +		dma_free_coherent(dev, sizeof(*pdsfc->ident),
> +				  pdsfc->ident, pdsfc->ident_pa);
> +		pdsfc->ident = NULL;

Please don't assign NULL to pointers if they are not reused.

> +		pdsfc->ident_pa = DMA_MAPPING_ERROR;

Same.

> +	}
> +}
> +
> +static int pdsfc_identify(struct pdsfc_dev *pdsfc)
> +{
> +	struct device *dev = &pdsfc->fwctl.dev;
> +	union pds_core_adminq_comp comp = {0};
> +	union pds_core_adminq_cmd cmd = {0};
> +	struct pds_fwctl_ident *ident;
> +	dma_addr_t ident_pa;
> +	int err = 0;

There is no need to assign 0 to err.

> +
> +	ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL);
> +	err = dma_mapping_error(dev->parent, ident_pa);
> +	if (err) {
> +		dev_err(dev, "Failed to map ident\n");

This is one of the examples of such extra prints.

> +		return err;
> +	}
> +
> +	cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
> +	cmd.fwctl_ident.version = 0;

How will you manage this version field?

> +	cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident));
> +	cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa);
> +
> +	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
> +	if (err) {
> +		dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa);
> +		dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
> +			cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
> +		return err;
> +	}
> +
> +	pdsfc->ident = ident;
> +	pdsfc->ident_pa = ident_pa;
> +
> +	dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n",
> +		ident->version, ident->max_req_sz, ident->max_resp_sz,
> +		ident->max_req_sg_elems, ident->max_resp_sg_elems);
> +
> +	return 0;
> +}
> +
> +static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> +			  void *in, size_t in_len, size_t *out_len)
> +{
> +	return NULL;
> +}
> +
> +static const struct fwctl_ops pdsfc_ops = {
> +	.device_type = FWCTL_DEVICE_TYPE_PDS,
> +	.uctx_size = sizeof(struct pdsfc_uctx),
> +	.open_uctx = pdsfc_open_uctx,
> +	.close_uctx = pdsfc_close_uctx,
> +	.info = pdsfc_info,
> +	.fw_rpc = pdsfc_fw_rpc,
> +};
> +
> +static int pdsfc_probe(struct auxiliary_device *adev,
> +		       const struct auxiliary_device_id *id)
> +{
> +	struct pdsfc_dev *pdsfc __free(pdsfc_dev);
> +	struct pds_auxiliary_dev *padev;
> +	struct device *dev = &adev->dev;
> +	int err = 0;
> +
> +	padev = container_of(adev, struct pds_auxiliary_dev, aux_dev);
> +	pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
> +				   struct pdsfc_dev, fwctl);
> +	if (!pdsfc) {
> +		dev_err(dev, "Failed to allocate fwctl device struct\n");
> +		return -ENOMEM;
> +	}
> +	pdsfc->padev = padev;
> +
> +	err = pdsfc_identify(pdsfc);
> +	if (err) {
> +		dev_err(dev, "Failed to identify device, err %d\n", err);
> +		return err;
> +	}
> +
> +	err = fwctl_register(&pdsfc->fwctl);
> +	if (err) {
> +		dev_err(dev, "Failed to register device, err %d\n", err);
> +		return err;
> +	}
> +
> +	auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
> +
> +	return 0;
> +
> +free_ident:
> +	pdsfc_free_ident(pdsfc);
> +	return err;
> +}
> +
> +static void pdsfc_remove(struct auxiliary_device *adev)
> +{
> +	struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
> +
> +	fwctl_unregister(&pdsfc->fwctl);
> +	pdsfc_free_ident(pdsfc);
> +}
> +
> +static const struct auxiliary_device_id pdsfc_id_table[] = {
> +	{.name = PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_FWCTL_STR },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, pdsfc_id_table);
> +
> +static struct auxiliary_driver pdsfc_driver = {
> +	.name = "pds_fwctl",
> +	.probe = pdsfc_probe,
> +	.remove = pdsfc_remove,
> +	.id_table = pdsfc_id_table,
> +};
> +
> +module_auxiliary_driver(pdsfc_driver);
> +
> +MODULE_IMPORT_NS(FWCTL);
> +MODULE_DESCRIPTION("pds fwctl driver");
> +MODULE_AUTHOR("Shannon Nelson <shannon.nelson@amd.com>");
> +MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>");
> +MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
> index 4b4e9a98b37b..7fc353b63353 100644
> --- a/include/linux/pds/pds_adminq.h
> +++ b/include/linux/pds/pds_adminq.h
> @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd {
>  	u8     status;
>  };
>  
> +enum pds_fwctl_cmd_opcode {
> +	PDS_FWCTL_CMD_IDENT		= 70,

Please try to avoid from vertical space alignment. It doesn't survive
time and at some point you will need to reformat it, which will cause
to churn and harm backporting/stable without any reason.

Thanks

Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Nelson, Shannon]

On 2/18/2025 11:51 AM, Leon Romanovsky wrote:
> 
> On Tue, Feb 11, 2025 at 03:48:52PM -0800, Shannon Nelson wrote:
>> Initial files for adding a new fwctl driver for the AMD/Pensando PDS
>> devices.  This sets up a simple auxiliary_bus driver that registers
>> with fwctl subsystem.  It expects that a pds_core device has set up
>> the auxiliary_device pds_core.fwctl
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>>   MAINTAINERS                    |   7 ++
>>   drivers/fwctl/Kconfig          |  10 ++
>>   drivers/fwctl/Makefile         |   1 +
>>   drivers/fwctl/pds/Makefile     |   4 +
>>   drivers/fwctl/pds/main.c       | 195 +++++++++++++++++++++++++++++++++
>>   include/linux/pds/pds_adminq.h |  77 +++++++++++++
>>   include/uapi/fwctl/fwctl.h     |   1 +
>>   include/uapi/fwctl/pds.h       |  27 +++++
>>   8 files changed, 322 insertions(+)
>>   create mode 100644 drivers/fwctl/pds/Makefile
>>   create mode 100644 drivers/fwctl/pds/main.c
>>   create mode 100644 include/uapi/fwctl/pds.h
> 
> <...>
> 
>> +static int pdsfc_open_uctx(struct fwctl_uctx *uctx)
>> +{
>> +     struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
>> +     struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
>> +     struct device *dev = &uctx->fwctl->dev;
>> +
>> +     dev_dbg(dev, "%s: caps = 0x%04x\n", __func__, pdsfc->caps);
> 
> This driver is too noisy and has too many debug/err prints.

Yes, still being worked on, but also we used dbp prints to keep the 
noise to a minimum.  Most of these will disappear as we move out of RFC 
mode.

> 
>> +     pdsfc_uctx->uctx_caps = pdsfc->caps;
>> +
>> +     return 0;
>> +}
>> +
>> +static void pdsfc_close_uctx(struct fwctl_uctx *uctx)
>> +{
>> +}
>> +
>> +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length)
>> +{
>> +     struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx);
>> +     struct fwctl_info_pds *info;
>> +
>> +     info = kzalloc(sizeof(*info), GFP_KERNEL);
>> +     if (!info)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     info->uctx_caps = pdsfc_uctx->uctx_caps;
>> +
>> +     return info;
>> +}
>> +
>> +static void pdsfc_free_ident(struct pdsfc_dev *pdsfc)
>> +{
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +
>> +     if (pdsfc->ident) {
> 
> It is not kernel style, which is success oriented.
> If (!pdsfc->ident)
>     return;
> 
> However I don't know how can this happen. You shouldn't call to pdsfc_free_ident
> if ident wasn't set.

This will change as we rework the ident handling to simply cache it and 
immediately drop the DMA linkage as suggested by an earlier review.

> 
>> +             dma_free_coherent(dev, sizeof(*pdsfc->ident),
>> +                               pdsfc->ident, pdsfc->ident_pa);
>> +             pdsfc->ident = NULL;
> 
> Please don't assign NULL to pointers if they are not reused.
> 
>> +             pdsfc->ident_pa = DMA_MAPPING_ERROR;
> 
> Same.
> 
>> +     }
>> +}
>> +
>> +static int pdsfc_identify(struct pdsfc_dev *pdsfc)
>> +{
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     union pds_core_adminq_comp comp = {0};
>> +     union pds_core_adminq_cmd cmd = {0};
>> +     struct pds_fwctl_ident *ident;
>> +     dma_addr_t ident_pa;
>> +     int err = 0;
> 
> There is no need to assign 0 to err.
> 
>> +
>> +     ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL);
>> +     err = dma_mapping_error(dev->parent, ident_pa);
>> +     if (err) {
>> +             dev_err(dev, "Failed to map ident\n");
> 
> This is one of the examples of such extra prints.
> 
>> +             return err;
>> +     }
>> +
>> +     cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
>> +     cmd.fwctl_ident.version = 0;
> 
> How will you manage this version field?

Since there is only version 0 at this point, there is not much to 
manage.  I wanted to explicitly show the setting to version 0, but maybe 
that can be assumed by the basic struct init.

> 
>> +     cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident));
>> +     cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa);
>> +
>> +     err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> +     if (err) {
>> +             dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa);
>> +             dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
>> +                     cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
>> +             return err;
>> +     }
>> +
>> +     pdsfc->ident = ident;
>> +     pdsfc->ident_pa = ident_pa;
>> +
>> +     dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n",
>> +             ident->version, ident->max_req_sz, ident->max_resp_sz,
>> +             ident->max_req_sg_elems, ident->max_resp_sg_elems);
>> +
>> +     return 0;
>> +}
>> +
>> +static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
>> +                       void *in, size_t in_len, size_t *out_len)
>> +{
>> +     return NULL;
>> +}
>> +
>> +static const struct fwctl_ops pdsfc_ops = {
>> +     .device_type = FWCTL_DEVICE_TYPE_PDS,
>> +     .uctx_size = sizeof(struct pdsfc_uctx),
>> +     .open_uctx = pdsfc_open_uctx,
>> +     .close_uctx = pdsfc_close_uctx,
>> +     .info = pdsfc_info,
>> +     .fw_rpc = pdsfc_fw_rpc,
>> +};
>> +
>> +static int pdsfc_probe(struct auxiliary_device *adev,
>> +                    const struct auxiliary_device_id *id)
>> +{
>> +     struct pdsfc_dev *pdsfc __free(pdsfc_dev);
>> +     struct pds_auxiliary_dev *padev;
>> +     struct device *dev = &adev->dev;
>> +     int err = 0;
>> +
>> +     padev = container_of(adev, struct pds_auxiliary_dev, aux_dev);
>> +     pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
>> +                                struct pdsfc_dev, fwctl);
>> +     if (!pdsfc) {
>> +             dev_err(dev, "Failed to allocate fwctl device struct\n");
>> +             return -ENOMEM;
>> +     }
>> +     pdsfc->padev = padev;
>> +
>> +     err = pdsfc_identify(pdsfc);
>> +     if (err) {
>> +             dev_err(dev, "Failed to identify device, err %d\n", err);
>> +             return err;
>> +     }
>> +
>> +     err = fwctl_register(&pdsfc->fwctl);
>> +     if (err) {
>> +             dev_err(dev, "Failed to register device, err %d\n", err);
>> +             return err;
>> +     }
>> +
>> +     auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
>> +
>> +     return 0;
>> +
>> +free_ident:
>> +     pdsfc_free_ident(pdsfc);
>> +     return err;
>> +}
>> +
>> +static void pdsfc_remove(struct auxiliary_device *adev)
>> +{
>> +     struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>> +
>> +     fwctl_unregister(&pdsfc->fwctl);
>> +     pdsfc_free_ident(pdsfc);
>> +}
>> +
>> +static const struct auxiliary_device_id pdsfc_id_table[] = {
>> +     {.name = PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_FWCTL_STR },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(auxiliary, pdsfc_id_table);
>> +
>> +static struct auxiliary_driver pdsfc_driver = {
>> +     .name = "pds_fwctl",
>> +     .probe = pdsfc_probe,
>> +     .remove = pdsfc_remove,
>> +     .id_table = pdsfc_id_table,
>> +};
>> +
>> +module_auxiliary_driver(pdsfc_driver);
>> +
>> +MODULE_IMPORT_NS(FWCTL);
>> +MODULE_DESCRIPTION("pds fwctl driver");
>> +MODULE_AUTHOR("Shannon Nelson <shannon.nelson@amd.com>");
>> +MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>");
>> +MODULE_LICENSE("Dual BSD/GPL");
>> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
>> index 4b4e9a98b37b..7fc353b63353 100644
>> --- a/include/linux/pds/pds_adminq.h
>> +++ b/include/linux/pds/pds_adminq.h
>> @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd {
>>        u8     status;
>>   };
>>
>> +enum pds_fwctl_cmd_opcode {
>> +     PDS_FWCTL_CMD_IDENT             = 70,
> 
> Please try to avoid from vertical space alignment. It doesn't survive
> time and at some point you will need to reformat it, which will cause
> to churn and harm backporting/stable without any reason.

Yeah, that was inherited from earlier work not necessary here.

> 
> Thanks

Thanks for your time amd comments,
sln

Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Leon Romanovsky]

On Tue, Feb 18, 2025 at 02:19:03PM -0800, Nelson, Shannon wrote:
> On 2/18/2025 11:51 AM, Leon Romanovsky wrote:
> > 
> > On Tue, Feb 11, 2025 at 03:48:52PM -0800, Shannon Nelson wrote:
> > > Initial files for adding a new fwctl driver for the AMD/Pensando PDS
> > > devices.  This sets up a simple auxiliary_bus driver that registers
> > > with fwctl subsystem.  It expects that a pds_core device has set up
> > > the auxiliary_device pds_core.fwctl
> > > 
> > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> > > ---
> > >   MAINTAINERS                    |   7 ++
> > >   drivers/fwctl/Kconfig          |  10 ++
> > >   drivers/fwctl/Makefile         |   1 +
> > >   drivers/fwctl/pds/Makefile     |   4 +
> > >   drivers/fwctl/pds/main.c       | 195 +++++++++++++++++++++++++++++++++
> > >   include/linux/pds/pds_adminq.h |  77 +++++++++++++
> > >   include/uapi/fwctl/fwctl.h     |   1 +
> > >   include/uapi/fwctl/pds.h       |  27 +++++
> > >   8 files changed, 322 insertions(+)
> > >   create mode 100644 drivers/fwctl/pds/Makefile
> > >   create mode 100644 drivers/fwctl/pds/main.c
> > >   create mode 100644 include/uapi/fwctl/pds.h
> > 
> > <...>

<...>

> > > +             return err;
> > > +     }
> > > +
> > > +     cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
> > > +     cmd.fwctl_ident.version = 0;
> > 
> > How will you manage this version field?
> 
> Since there is only version 0 at this point, there is not much to manage.  I
> wanted to explicitly show the setting to version 0, but maybe that can be
> assumed by the basic struct init.

But the question is slightly different "How will you manage this version field?"

Thanks

Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Nelson, Shannon]

On 2/19/2025 12:25 AM, Leon Romanovsky wrote:
> 
> On Tue, Feb 18, 2025 at 02:19:03PM -0800, Nelson, Shannon wrote:
>> On 2/18/2025 11:51 AM, Leon Romanovsky wrote:
>>>
>>> On Tue, Feb 11, 2025 at 03:48:52PM -0800, Shannon Nelson wrote:
>>>> Initial files for adding a new fwctl driver for the AMD/Pensando PDS
>>>> devices.  This sets up a simple auxiliary_bus driver that registers
>>>> with fwctl subsystem.  It expects that a pds_core device has set up
>>>> the auxiliary_device pds_core.fwctl
>>>>
>>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>>>> ---
>>>>    MAINTAINERS                    |   7 ++
>>>>    drivers/fwctl/Kconfig          |  10 ++
>>>>    drivers/fwctl/Makefile         |   1 +
>>>>    drivers/fwctl/pds/Makefile     |   4 +
>>>>    drivers/fwctl/pds/main.c       | 195 +++++++++++++++++++++++++++++++++
>>>>    include/linux/pds/pds_adminq.h |  77 +++++++++++++
>>>>    include/uapi/fwctl/fwctl.h     |   1 +
>>>>    include/uapi/fwctl/pds.h       |  27 +++++
>>>>    8 files changed, 322 insertions(+)
>>>>    create mode 100644 drivers/fwctl/pds/Makefile
>>>>    create mode 100644 drivers/fwctl/pds/main.c
>>>>    create mode 100644 include/uapi/fwctl/pds.h
>>>
>>> <...>
> 
> <...>
> 
>>>> +             return err;
>>>> +     }
>>>> +
>>>> +     cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
>>>> +     cmd.fwctl_ident.version = 0;
>>>
>>> How will you manage this version field?
>>
>> Since there is only version 0 at this point, there is not much to manage.  I
>> wanted to explicitly show the setting to version 0, but maybe that can be
>> assumed by the basic struct init.
> 
> But the question is slightly different "How will you manage this version field?"

If we find we have to change the interface in a non-backward-compatable 
way, we'll increment the version number that we support, and watch for 
the version number supported by the firmware as reported in the ident 
struct data and interpret the data appropriately.  Similarly, if the 
firmware sees that the host driver is at a lower version number, it will 
handle data in the older format.

sln

Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

[Leon Romanovsky]

On Thu, Feb 20, 2025 at 03:27:37PM -0800, Nelson, Shannon wrote:
> On 2/19/2025 12:25 AM, Leon Romanovsky wrote:
> > 
> > On Tue, Feb 18, 2025 at 02:19:03PM -0800, Nelson, Shannon wrote:
> > > On 2/18/2025 11:51 AM, Leon Romanovsky wrote:
> > > > 
> > > > On Tue, Feb 11, 2025 at 03:48:52PM -0800, Shannon Nelson wrote:
> > > > > Initial files for adding a new fwctl driver for the AMD/Pensando PDS
> > > > > devices.  This sets up a simple auxiliary_bus driver that registers
> > > > > with fwctl subsystem.  It expects that a pds_core device has set up
> > > > > the auxiliary_device pds_core.fwctl
> > > > > 
> > > > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> > > > > ---
> > > > >    MAINTAINERS                    |   7 ++
> > > > >    drivers/fwctl/Kconfig          |  10 ++
> > > > >    drivers/fwctl/Makefile         |   1 +
> > > > >    drivers/fwctl/pds/Makefile     |   4 +
> > > > >    drivers/fwctl/pds/main.c       | 195 +++++++++++++++++++++++++++++++++
> > > > >    include/linux/pds/pds_adminq.h |  77 +++++++++++++
> > > > >    include/uapi/fwctl/fwctl.h     |   1 +
> > > > >    include/uapi/fwctl/pds.h       |  27 +++++
> > > > >    8 files changed, 322 insertions(+)
> > > > >    create mode 100644 drivers/fwctl/pds/Makefile
> > > > >    create mode 100644 drivers/fwctl/pds/main.c
> > > > >    create mode 100644 include/uapi/fwctl/pds.h
> > > > 
> > > > <...>
> > 
> > <...>
> > 
> > > > > +             return err;
> > > > > +     }
> > > > > +
> > > > > +     cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT;
> > > > > +     cmd.fwctl_ident.version = 0;
> > > > 
> > > > How will you manage this version field?
> > > 
> > > Since there is only version 0 at this point, there is not much to manage.  I
> > > wanted to explicitly show the setting to version 0, but maybe that can be
> > > assumed by the basic struct init.
> > 
> > But the question is slightly different "How will you manage this version field?"
> 
> If we find we have to change the interface in a non-backward-compatable way,
> we'll increment the version number that we support, and watch for the
> version number supported by the firmware as reported in the ident struct
> data and interpret the data appropriately.  Similarly, if the firmware sees
> that the host driver is at a lower version number, it will handle data in
> the older format.

Thanks.

> 
> sln
> 
> 
> 

[RFC PATCH fwctl 4/5] pds_fwctl: add rpc and query support

[Shannon Nelson]

From: Brett Creeley <brett.creeley@amd.com>

The pds_fwctl driver doesn't know what RPC operations are available
in the firmware, so also doesn't know what scope they might have.  The
userland utility supplies the firmware "endpoint" and "operation" id values
and this driver queries the firmware for endpoints and their available
operations.  The operation descriptions include the scope information
which the driver uses for scope testing.

Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/fwctl/pds/main.c       | 369 ++++++++++++++++++++++++++++++++-
 include/linux/pds/pds_adminq.h | 187 +++++++++++++++++
 include/uapi/fwctl/pds.h       |  16 ++
 3 files changed, 569 insertions(+), 3 deletions(-)

diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
index 24979fe0deea..b60a66ef1fac 100644
--- a/drivers/fwctl/pds/main.c
+++ b/drivers/fwctl/pds/main.c
@@ -15,12 +15,22 @@
 #include <linux/pds/pds_adminq.h>
 #include <linux/pds/pds_auxbus.h>
 
+DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T));
+DEFINE_FREE(kvfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T));
+
 struct pdsfc_uctx {
 	struct fwctl_uctx uctx;
 	u32 uctx_caps;
 	u32 uctx_uid;
 };
 
+struct pdsfc_rpc_endpoint_info {
+	u32 endpoint;
+	dma_addr_t operations_pa;
+	struct pds_fwctl_query_data *operations;
+	struct mutex lock;	/* lock for endpoint info management */
+};
+
 struct pdsfc_dev {
 	struct fwctl_device fwctl;
 	struct pds_auxiliary_dev *padev;
@@ -28,6 +38,9 @@ struct pdsfc_dev {
 	u32 caps;
 	dma_addr_t ident_pa;
 	struct pds_fwctl_ident *ident;
+	dma_addr_t endpoints_pa;
+	struct pds_fwctl_query_data *endpoints;
+	struct pdsfc_rpc_endpoint_info *endpoint_info;
 };
 DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));
 
@@ -112,10 +125,351 @@ static int pdsfc_identify(struct pdsfc_dev *pdsfc)
 	return 0;
 }
 
+static void pdsfc_free_endpoints(struct pdsfc_dev *pdsfc)
+{
+	struct device *dev = &pdsfc->fwctl.dev;
+
+	if (pdsfc->endpoints) {
+		int i;
+
+		for (i = 0; pdsfc->endpoint_info && i < pdsfc->endpoints->num_entries; i++)
+			mutex_destroy(&pdsfc->endpoint_info[i].lock);
+		vfree(pdsfc->endpoint_info);
+		pdsfc->endpoint_info = NULL;
+		dma_free_coherent(dev->parent, PAGE_SIZE,
+				  pdsfc->endpoints, pdsfc->endpoints_pa);
+		pdsfc->endpoints = NULL;
+		pdsfc->endpoints_pa = DMA_MAPPING_ERROR;
+	}
+}
+
+static void pdsfc_free_operations(struct pdsfc_dev *pdsfc)
+{
+	struct device *dev = &pdsfc->fwctl.dev;
+	int i;
+
+	for (i = 0; i < pdsfc->endpoints->num_entries; i++) {
+		struct pdsfc_rpc_endpoint_info *ei = &pdsfc->endpoint_info[i];
+
+		if (ei->operations) {
+			dma_free_coherent(dev->parent, PAGE_SIZE,
+					  ei->operations, ei->operations_pa);
+			ei->operations = NULL;
+			ei->operations_pa = DMA_MAPPING_ERROR;
+		}
+	}
+}
+
+static struct pds_fwctl_query_data *pdsfc_get_endpoints(struct pdsfc_dev *pdsfc,
+							dma_addr_t *pa)
+{
+	struct pds_fwctl_query_data_endpoint *entries = NULL;
+	struct device *dev = &pdsfc->fwctl.dev;
+	union pds_core_adminq_comp comp = {0};
+	union pds_core_adminq_cmd cmd = {0};
+	struct pds_fwctl_query_data *data;
+	dma_addr_t data_pa;
+	int err;
+	int i;
+
+	data = dma_alloc_coherent(dev->parent, PAGE_SIZE, &data_pa, GFP_KERNEL);
+	err = dma_mapping_error(dev, data_pa);
+	if (err) {
+		dev_err(dev, "Failed to map endpoint list\n");
+		return ERR_PTR(err);
+	}
+
+	cmd.fwctl_query.opcode = PDS_FWCTL_CMD_QUERY;
+	cmd.fwctl_query.entity = PDS_FWCTL_RPC_ROOT;
+	cmd.fwctl_query.version = 0;
+	cmd.fwctl_query.query_data_buf_len = cpu_to_le32(PAGE_SIZE);
+	cmd.fwctl_query.query_data_buf_pa = cpu_to_le64(data_pa);
+
+	dev_dbg(dev, "cmd: opcode %d entity %d version %d query_data_buf_len %d query_data_buf_pa %llx\n",
+		cmd.fwctl_query.opcode, cmd.fwctl_query.entity, cmd.fwctl_query.version,
+		le32_to_cpu(cmd.fwctl_query.query_data_buf_len),
+		le64_to_cpu(cmd.fwctl_query.query_data_buf_pa));
+
+	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
+	if (err) {
+		dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
+			cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
+		dma_free_coherent(dev->parent, PAGE_SIZE, data, data_pa);
+		return ERR_PTR(err);
+	}
+
+	*pa = data_pa;
+
+	entries = (struct pds_fwctl_query_data_endpoint *)data->entries;
+	dev_dbg(dev, "num_entries %d\n", data->num_entries);
+	for (i = 0; i < data->num_entries; i++)
+		dev_dbg(dev, "endpoint: id %d\n", entries[i].id);
+
+	return data;
+}
+
+static int pdsfc_init_endpoints(struct pdsfc_dev *pdsfc)
+{
+	struct pds_fwctl_query_data_endpoint *ep_entry;
+	struct device *dev = &pdsfc->fwctl.dev;
+	int i;
+
+	pdsfc->endpoints = pdsfc_get_endpoints(pdsfc, &pdsfc->endpoints_pa);
+	if (IS_ERR(pdsfc->endpoints)) {
+		dev_err(dev, "Failed to query endpoints\n");
+		return PTR_ERR(pdsfc->endpoints);
+	}
+
+	pdsfc->endpoint_info = vcalloc(pdsfc->endpoints->num_entries,
+				       sizeof(*pdsfc->endpoint_info));
+	if (!pdsfc->endpoint_info) {
+		dev_err(dev, "Failed to allocate endpoint_info array\n");
+		pdsfc_free_endpoints(pdsfc);
+		return -ENOMEM;
+	}
+
+	ep_entry = (struct pds_fwctl_query_data_endpoint *)pdsfc->endpoints->entries;
+	for (i = 0; i < pdsfc->endpoints->num_entries; i++) {
+		mutex_init(&pdsfc->endpoint_info[i].lock);
+		pdsfc->endpoint_info[i].endpoint = ep_entry[i].id;
+	}
+
+	return 0;
+}
+
+static struct pds_fwctl_query_data *pdsfc_get_operations(struct pdsfc_dev *pdsfc,
+							 dma_addr_t *pa, u32 ep)
+{
+	struct pds_fwctl_query_data_operation *entries = NULL;
+	struct device *dev = &pdsfc->fwctl.dev;
+	union pds_core_adminq_comp comp = {0};
+	union pds_core_adminq_cmd cmd = {0};
+	struct pds_fwctl_query_data *data;
+	dma_addr_t data_pa;
+	int err;
+	int i;
+
+	/* Query the operations list for the given endpoint */
+	data = dma_alloc_coherent(dev->parent, PAGE_SIZE, &data_pa, GFP_KERNEL);
+	err = dma_mapping_error(dev->parent, data_pa);
+	if (err) {
+		dev_err(dev, "Failed to map operations list\n");
+		return ERR_PTR(err);
+	}
+
+	cmd.fwctl_query.opcode = PDS_FWCTL_CMD_QUERY;
+	cmd.fwctl_query.entity = PDS_FWCTL_RPC_ENDPOINT;
+	cmd.fwctl_query.version = 0;
+	cmd.fwctl_query.query_data_buf_len = cpu_to_le32(PAGE_SIZE);
+	cmd.fwctl_query.query_data_buf_pa = cpu_to_le64(data_pa);
+	cmd.fwctl_query.ep = cpu_to_le32(ep);
+
+	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
+	if (err) {
+		dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
+			cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
+		dma_free_coherent(dev->parent, PAGE_SIZE, data, data_pa);
+		return ERR_PTR(err);
+	}
+
+	*pa = data_pa;
+
+	entries = (struct pds_fwctl_query_data_operation *)data->entries;
+	dev_dbg(dev, "num_entries %d\n", data->num_entries);
+	for (i = 0; i < data->num_entries; i++)
+		dev_dbg(dev, "endpoint %d operation: id %x scope %d\n",
+			ep, entries[i].id, entries[i].scope);
+
+	return data;
+}
+
+static int pdsfc_validate_rpc(struct pdsfc_dev *pdsfc,
+			      struct fwctl_rpc_pds *rpc,
+			      enum fwctl_rpc_scope scope)
+{
+	struct pds_fwctl_query_data_operation *op_entry = NULL;
+	struct pdsfc_rpc_endpoint_info *ep_info = NULL;
+	struct device *dev = &pdsfc->fwctl.dev;
+	int i;
+
+	if (!pdsfc->ident) {
+		dev_err(dev, "Ident not available\n");
+		return -EINVAL;
+	}
+
+	/* validate rpc in_len & out_len based
+	 * on ident->max_req_sz & max_resp_sz
+	 */
+	if (rpc->in.len > pdsfc->ident->max_req_sz) {
+		dev_err(dev, "Invalid request size %u, max %u\n",
+			rpc->in.len, pdsfc->ident->max_req_sz);
+		return -EINVAL;
+	}
+
+	if (rpc->out.len > pdsfc->ident->max_resp_sz) {
+		dev_err(dev, "Invalid response size %u, max %u\n",
+			rpc->out.len, pdsfc->ident->max_resp_sz);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < pdsfc->endpoints->num_entries; i++) {
+		if (pdsfc->endpoint_info[i].endpoint == rpc->in.ep) {
+			ep_info = &pdsfc->endpoint_info[i];
+			break;
+		}
+	}
+	if (!ep_info) {
+		dev_err(dev, "Invalid endpoint %d\n", rpc->in.ep);
+		return -EINVAL;
+	}
+
+	/* query and cache this endpoint's operations */
+	mutex_lock(&ep_info->lock);
+	if (!ep_info->operations) {
+		ep_info->operations = pdsfc_get_operations(pdsfc,
+							   &ep_info->operations_pa,
+							   rpc->in.ep);
+		if (!ep_info->operations) {
+			mutex_unlock(&ep_info->lock);
+			dev_err(dev, "Failed to allocate operations list\n");
+			return -ENOMEM;
+		}
+	}
+	mutex_unlock(&ep_info->lock);
+
+	/* reject unsupported and/or out of scope commands */
+	op_entry = (struct pds_fwctl_query_data_operation *)ep_info->operations->entries;
+	for (i = 0; i < ep_info->operations->num_entries; i++) {
+		if (PDS_FWCTL_RPC_OPCODE_CMP(rpc->in.op, op_entry[i].id)) {
+			if (scope < op_entry[i].scope)
+				return -EPERM;
+			return 0;
+		}
+	}
+
+	dev_err(dev, "Invalid operation %d for endpoint %d\n", rpc->in.op, rpc->in.ep);
+
+	return -EINVAL;
+}
+
 static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
 			  void *in, size_t in_len, size_t *out_len)
 {
-	return NULL;
+	struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
+	struct fwctl_rpc_pds *rpc = (struct fwctl_rpc_pds *)in;
+	void *out_payload __free(kfree_errptr) = NULL;
+	void *in_payload __free(kfree_errptr) = NULL;
+	struct device *dev = &uctx->fwctl->dev;
+	union pds_core_adminq_comp comp = {0};
+	dma_addr_t out_payload_dma_addr = 0;
+	union pds_core_adminq_cmd cmd = {0};
+	dma_addr_t in_payload_dma_addr = 0;
+	void *out = NULL;
+	int err;
+
+	err = pdsfc_validate_rpc(pdsfc, rpc, scope);
+	if (err) {
+		dev_err(dev, "Invalid RPC request\n");
+		return ERR_PTR(err);
+	}
+
+	if (rpc->in.len > 0) {
+		in_payload = kzalloc(rpc->in.len, GFP_KERNEL);
+		if (!in_payload) {
+			dev_err(dev, "Failed to allocate in_payload\n");
+			out = ERR_PTR(-ENOMEM);
+			goto done;
+		}
+
+		if (copy_from_user(in_payload, u64_to_user_ptr(rpc->in.payload),
+				   rpc->in.len)) {
+			dev_err(dev, "Failed to copy in_payload from user\n");
+			out = ERR_PTR(-EFAULT);
+			goto done;
+		}
+
+		in_payload_dma_addr = dma_map_single(dev->parent, in_payload,
+						     rpc->in.len, DMA_TO_DEVICE);
+		err = dma_mapping_error(dev->parent, in_payload_dma_addr);
+		if (err) {
+			dev_err(dev, "Failed to map in_payload\n");
+			out = ERR_PTR(err);
+			goto done;
+		}
+	}
+
+	if (rpc->out.len > 0) {
+		out_payload = kzalloc(rpc->out.len, GFP_KERNEL);
+		if (!out_payload) {
+			dev_err(dev, "Failed to allocate out_payload\n");
+			out = ERR_PTR(-ENOMEM);
+			goto done;
+		}
+
+		out_payload_dma_addr = dma_map_single(dev->parent, out_payload,
+						      rpc->out.len, DMA_FROM_DEVICE);
+		err = dma_mapping_error(dev->parent, out_payload_dma_addr);
+		if (err) {
+			dev_err(dev, "Failed to map out_payload\n");
+			out = ERR_PTR(err);
+			goto done;
+		}
+	}
+
+	cmd.fwctl_rpc.opcode = PDS_FWCTL_CMD_RPC;
+	cmd.fwctl_rpc.flags = PDS_FWCTL_RPC_IND_REQ | PDS_FWCTL_RPC_IND_RESP;
+	cmd.fwctl_rpc.ep = cpu_to_le32(rpc->in.ep);
+	cmd.fwctl_rpc.op = cpu_to_le32(rpc->in.op);
+	cmd.fwctl_rpc.req_pa = cpu_to_le64(in_payload_dma_addr);
+	cmd.fwctl_rpc.req_sz = cpu_to_le32(rpc->in.len);
+	cmd.fwctl_rpc.resp_pa = cpu_to_le64(out_payload_dma_addr);
+	cmd.fwctl_rpc.resp_sz = cpu_to_le32(rpc->out.len);
+
+	dev_dbg(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d\n",
+		__func__, rpc->in.ep, rpc->in.op,
+		cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
+		cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems);
+
+	dynamic_hex_dump("in ", DUMP_PREFIX_OFFSET, 16, 1, in_payload, rpc->in.len, true);
+
+	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
+	if (err) {
+		dev_err(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d err %d\n",
+			__func__, rpc->in.ep, rpc->in.op,
+			cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
+			cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems,
+			err);
+		out = ERR_PTR(err);
+		goto done;
+	}
+
+	dynamic_hex_dump("out ", DUMP_PREFIX_OFFSET, 16, 1, out_payload, rpc->out.len, true);
+
+	dev_dbg(dev, "%s: status %d comp_index %d err %d resp_sz %d color %d\n",
+		__func__, comp.fwctl_rpc.status, comp.fwctl_rpc.comp_index,
+		comp.fwctl_rpc.err, comp.fwctl_rpc.resp_sz,
+		comp.fwctl_rpc.color);
+
+	if (copy_to_user(u64_to_user_ptr(rpc->out.payload), out_payload, rpc->out.len)) {
+		dev_err(dev, "Failed to copy out_payload to user\n");
+		out = ERR_PTR(-EFAULT);
+		goto done;
+	}
+
+	rpc->out.retval = le32_to_cpu(comp.fwctl_rpc.err);
+	*out_len = in_len;
+	out = in;
+
+done:
+	if (in_payload_dma_addr)
+		dma_unmap_single(dev->parent, in_payload_dma_addr,
+				 rpc->in.len, DMA_TO_DEVICE);
+
+	if (out_payload_dma_addr)
+		dma_unmap_single(dev->parent, out_payload_dma_addr,
+				 rpc->out.len, DMA_FROM_DEVICE);
+
+	return out;
 }
 
 static const struct fwctl_ops pdsfc_ops = {
@@ -150,16 +504,23 @@ static int pdsfc_probe(struct auxiliary_device *adev,
 		return err;
 	}
 
+	err = pdsfc_init_endpoints(pdsfc);
+	if (err) {
+		dev_err(dev, "Failed to init endpoints, err %d\n", err);
+		goto free_ident;
+	}
+
 	err = fwctl_register(&pdsfc->fwctl);
 	if (err) {
 		dev_err(dev, "Failed to register device, err %d\n", err);
-		return err;
+		goto free_endpoints;
 	}
-
 	auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
 
 	return 0;
 
+free_endpoints:
+	pdsfc_free_endpoints(pdsfc);
 free_ident:
 	pdsfc_free_ident(pdsfc);
 	return err;
@@ -170,6 +531,8 @@ static void pdsfc_remove(struct auxiliary_device *adev)
 	struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
 
 	fwctl_unregister(&pdsfc->fwctl);
+	pdsfc_free_operations(pdsfc);
+	pdsfc_free_endpoints(pdsfc);
 	pdsfc_free_ident(pdsfc);
 }
 
diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
index 7fc353b63353..33cd03388b15 100644
--- a/include/linux/pds/pds_adminq.h
+++ b/include/linux/pds/pds_adminq.h
@@ -1181,6 +1181,8 @@ struct pds_lm_host_vf_status_cmd {
 
 enum pds_fwctl_cmd_opcode {
 	PDS_FWCTL_CMD_IDENT		= 70,
+	PDS_FWCTL_CMD_RPC		= 71,
+	PDS_FWCTL_CMD_QUERY		= 72,
 };
 
 /**
@@ -1251,6 +1253,187 @@ struct pds_fwctl_ident {
 	u8     max_resp_sg_elems;
 } __packed;
 
+enum pds_fwctl_query_entity {
+	PDS_FWCTL_RPC_ROOT	= 0,
+	PDS_FWCTL_RPC_ENDPOINT	= 1,
+	PDS_FWCTL_RPC_OPERATION	= 2,
+};
+
+#define PDS_FWCTL_RPC_OPCODE_CMD_SHIFT	0
+#define PDS_FWCTL_RPC_OPCODE_CMD_MASK	GENMASK(15, PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)
+#define PDS_FWCTL_RPC_OPCODE_VER_SHIFT	16
+#define PDS_FWCTL_RPC_OPCODE_VER_MASK	GENMASK(23, PDS_FWCTL_RPC_OPCODE_VER_SHIFT)
+
+#define PDS_FWCTL_RPC_OPCODE_GET_CMD(op)	\
+	(((op) & PDS_FWCTL_RPC_OPCODE_CMD_MASK) >> PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)
+#define PDS_FWCTL_RPC_OPCODE_GET_VER(op)	\
+	(((op) & PDS_FWCTL_RPC_OPCODE_VER_MASK) >> PDS_FWCTL_RPC_OPCODE_VER_SHIFT)
+
+#define PDS_FWCTL_RPC_OPCODE_CMP(op1, op2) \
+	(PDS_FWCTL_RPC_OPCODE_GET_CMD(op1) == PDS_FWCTL_RPC_OPCODE_GET_CMD(op2) && \
+	 PDS_FWCTL_RPC_OPCODE_GET_VER(op1) <= PDS_FWCTL_RPC_OPCODE_GET_VER(op2))
+
+/**
+ * struct pds_fwctl_query_cmd - Firmware control query command structure
+ * @opcode: Operation code for the command
+ * @entity:  Entity type to query (enum pds_fwctl_query_entity)
+ * @version: Version of the query data structure supported by the driver
+ * @rsvd:    Word boundary padding
+ * @query_data_buf_len: Length of the query data buffer
+ * @query_data_buf_pa:  Physical address of the query data buffer
+ * @ep:      Endpoint identifier to query  (when entity is PDS_FWCTL_RPC_ENDPOINT)
+ * @op:      Operation identifier to query (when entity is PDS_FWCTL_RPC_OPERATION)
+ *
+ * This structure is used to send a query command to the firmware control
+ * interface. The structure is packed to ensure there is no padding between
+ * the fields.
+ */
+struct pds_fwctl_query_cmd {
+	u8     opcode;
+	u8     entity;
+	u8     version;
+	u8     rsvd;
+	__le32 query_data_buf_len;
+	__le64 query_data_buf_pa;
+	union {
+		__le32 ep;
+		__le32 op;
+	};
+} __packed;
+
+/**
+ * struct pds_fwctl_query_comp - Firmware control query completion structure
+ * @status:     Status of the query command
+ * @rsvd:       Word boundary padding
+ * @comp_index: Completion index in little-endian format
+ * @version:    Version of the query data structure returned by firmware. This
+ *		 should be less than or equal to the version supported by the driver.
+ * @rsvd2:      Word boundary padding
+ * @color:      Color bit indicating the state of the completion
+ */
+struct pds_fwctl_query_comp {
+	u8     status;
+	u8     rsvd;
+	__le16 comp_index;
+	u8     version;
+	u8     rsvd2[2];
+	u8     color;
+} __packed;
+
+/**
+ * struct pds_fwctl_query_data_endpoint - query data for entity PDS_FWCTL_RPC_ROOT
+ * @id: The identifier for the data endpoint.
+ */
+struct pds_fwctl_query_data_endpoint {
+	__le32 id;
+} __packed;
+
+/**
+ * struct pds_fwctl_query_data_operation - query data for entity PDS_FWCTL_RPC_ENDPOINT
+ * @id:    Operation identifier.
+ * @scope: Scope of the operation (enum fwctl_rpc_scope).
+ * @rsvd:  Word boundary padding
+ */
+struct pds_fwctl_query_data_operation {
+	__le32 id;
+	u8     scope;
+	u8     rsvd[3];
+} __packed;
+
+/**
+ * struct pds_fwctl_query_data - query data structure
+ * @version:     Version of the query data structure
+ * @rsvd:        Word boundary padding
+ * @num_entries: Number of entries in the union
+ * @entries:     Array of query data entries, depending on the entity type.
+ */
+struct pds_fwctl_query_data {
+	u8      version;
+	u8      rsvd[3];
+	__le32  num_entries;
+	uint8_t entries[];
+} __packed;
+
+/**
+ * struct pds_fwctl_rpc_cmd - Firmware control RPC command.
+ * @opcode:        opcode PDS_FWCTL_CMD_RPC
+ * @rsvd:          Word boundary padding
+ * @flags:         Indicates indirect request and/or response handling
+ * @ep:            Endpoint identifier.
+ * @op:            Operation identifier.
+ * @inline_req0:   Buffer for inline request
+ * @inline_req1:   Buffer for inline request
+ * @req_pa:        Physical address of request data.
+ * @req_sz:        Size of the request.
+ * @req_sg_elems:  Number of request SGs
+ * @req_rsvd:      Word boundary padding
+ * @inline_req2:   Buffer for inline request
+ * @resp_pa:       Physical address of response data.
+ * @resp_sz:       Size of the response.
+ * @resp_sg_elems: Number of response SGs
+ * @resp_rsvd:     Word boundary padding
+ */
+struct pds_fwctl_rpc_cmd {
+	u8     opcode;
+	u8     rsvd;
+	__le16 flags;
+#define PDS_FWCTL_RPC_IND_REQ		0x1
+#define PDS_FWCTL_RPC_IND_RESP		0x2
+	__le32 ep;
+	__le32 op;
+	u8 inline_req0[16];
+	union {
+		u8 inline_req1[16];
+		struct {
+			__le64 req_pa;
+			__le32 req_sz;
+			u8     req_sg_elems;
+			u8     req_rsvd[3];
+		};
+	};
+	union {
+		u8 inline_req2[16];
+		struct {
+			__le64 resp_pa;
+			__le32 resp_sz;
+			u8     resp_sg_elems;
+			u8     resp_rsvd[3];
+		};
+	};
+} __packed;
+
+/**
+ * struct pds_sg_elem - Transmit scatter-gather (SG) descriptor element
+ * @addr:	DMA address of SG element data buffer
+ * @len:	Length of SG element data buffer, in bytes
+ * @rsvd:	Word boundary padding
+ */
+struct pds_sg_elem {
+	__le64 addr;
+	__le32 len;
+	__le16 rsvd[2];
+} __packed;
+
+/**
+ * struct pds_fwctl_rpc_comp - Completion of a firmware control RPC.
+ * @status:     Status of the command
+ * @rsvd:       Word boundary padding
+ * @comp_index: Completion index of the command
+ * @err:        Error code, if any, from the RPC.
+ * @resp_sz:    Size of the response.
+ * @rsvd2:      Word boundary padding
+ * @color:      Color bit indicating the state of the completion.
+ */
+struct pds_fwctl_rpc_comp {
+	u8     status;
+	u8     rsvd;
+	__le16 comp_index;
+	__le32 err;
+	__le32 resp_sz;
+	u8     rsvd2[3];
+	u8     color;
+} __packed;
+
 union pds_core_adminq_cmd {
 	u8     opcode;
 	u8     bytes[64];
@@ -1291,6 +1474,8 @@ union pds_core_adminq_cmd {
 
 	struct pds_fwctl_cmd		  fwctl;
 	struct pds_fwctl_ident_cmd	  fwctl_ident;
+	struct pds_fwctl_rpc_cmd	  fwctl_rpc;
+	struct pds_fwctl_query_cmd	  fwctl_query;
 };
 
 union pds_core_adminq_comp {
@@ -1320,6 +1505,8 @@ union pds_core_adminq_comp {
 	struct pds_lm_dirty_status_comp	  lm_dirty_status;
 
 	struct pds_fwctl_comp		  fwctl;
+	struct pds_fwctl_rpc_comp	  fwctl_rpc;
+	struct pds_fwctl_query_comp	  fwctl_query;
 };
 
 #ifndef __CHECKER__
diff --git a/include/uapi/fwctl/pds.h b/include/uapi/fwctl/pds.h
index a01b032cbdb1..da6cd2d1c6fa 100644
--- a/include/uapi/fwctl/pds.h
+++ b/include/uapi/fwctl/pds.h
@@ -24,4 +24,20 @@ enum pds_fwctl_capabilities {
 	PDS_FWCTL_QUERY_CAP = 0,
 	PDS_FWCTL_SEND_CAP,
 };
+
+struct fwctl_rpc_pds {
+	struct {
+		__u32 op;
+		__u32 ep;
+		__u32 rsvd;
+		__u32 len;
+		__u64 payload;
+	} in;
+	struct {
+		__u32 retval;
+		__u32 rsvd[2];
+		__u32 len;
+		__u64 payload;
+	} out;
+};
 #endif /* _UAPI_FWCTL_PDS_H_ */
-- 
2.17.1

Re: [RFC PATCH fwctl 4/5] pds_fwctl: add rpc and query support

[Jonathan Cameron]

On Tue, 11 Feb 2025 15:48:53 -0800
Shannon Nelson <shannon.nelson@amd.com> wrote:

> From: Brett Creeley <brett.creeley@amd.com>
> 
> The pds_fwctl driver doesn't know what RPC operations are available
> in the firmware, so also doesn't know what scope they might have.  The
> userland utility supplies the firmware "endpoint" and "operation" id values
> and this driver queries the firmware for endpoints and their available
> operations.  The operation descriptions include the scope information
> which the driver uses for scope testing.
> 
> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
Various comments inline.  I haven't looked closely at the actual interface
yet though as running out of time today.

Jonathan

> ---
>  drivers/fwctl/pds/main.c       | 369 ++++++++++++++++++++++++++++++++-
>  include/linux/pds/pds_adminq.h | 187 +++++++++++++++++
>  include/uapi/fwctl/pds.h       |  16 ++
>  3 files changed, 569 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
> index 24979fe0deea..b60a66ef1fac 100644
> --- a/drivers/fwctl/pds/main.c
> +++ b/drivers/fwctl/pds/main.c
> @@ -15,12 +15,22 @@
>  #include <linux/pds/pds_adminq.h>
>  #include <linux/pds/pds_auxbus.h>
>  
> +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T));
> +DEFINE_FREE(kvfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T));

I'm lost. These look same as the ones in slab.h for kfree and kvfree
which already handle error pointers.  Maybe based on an old kernel?

> +static struct pds_fwctl_query_data *pdsfc_get_operations(struct pdsfc_dev *pdsfc,
> +							 dma_addr_t *pa, u32 ep)
> +{
> +	struct pds_fwctl_query_data_operation *entries = NULL;

Always set before use so don't initialize here.

> +	struct device *dev = &pdsfc->fwctl.dev;
> +	union pds_core_adminq_comp comp = {0};
> +	union pds_core_adminq_cmd cmd = {0};
> +	struct pds_fwctl_query_data *data;
> +	dma_addr_t data_pa;
> +	int err;
> +	int i;
> +
> +	/* Query the operations list for the given endpoint */
> +	data = dma_alloc_coherent(dev->parent, PAGE_SIZE, &data_pa, GFP_KERNEL);
> +	err = dma_mapping_error(dev->parent, data_pa);
> +	if (err) {
> +		dev_err(dev, "Failed to map operations list\n");
> +		return ERR_PTR(err);
> +	}
> +
> +	cmd.fwctl_query.opcode = PDS_FWCTL_CMD_QUERY;
> +	cmd.fwctl_query.entity = PDS_FWCTL_RPC_ENDPOINT;
> +	cmd.fwctl_query.version = 0;
> +	cmd.fwctl_query.query_data_buf_len = cpu_to_le32(PAGE_SIZE);
> +	cmd.fwctl_query.query_data_buf_pa = cpu_to_le64(data_pa);
> +	cmd.fwctl_query.ep = cpu_to_le32(ep);
> +
> +	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
> +	if (err) {
> +		dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
> +			cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
> +		dma_free_coherent(dev->parent, PAGE_SIZE, data, data_pa);
> +		return ERR_PTR(err);
> +	}
> +
> +	*pa = data_pa;
> +
> +	entries = (struct pds_fwctl_query_data_operation *)data->entries;
> +	dev_dbg(dev, "num_entries %d\n", data->num_entries);
> +	for (i = 0; i < data->num_entries; i++)
> +		dev_dbg(dev, "endpoint %d operation: id %x scope %d\n",
> +			ep, entries[i].id, entries[i].scope);
> +
> +	return data;
> +}

> +
>  static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
>  			  void *in, size_t in_len, size_t *out_len)
>  {
> -	return NULL;
> +	struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
> +	struct fwctl_rpc_pds *rpc = (struct fwctl_rpc_pds *)in;
In is a void * so never any need to cast it to another pointer type.

> +	void *out_payload __free(kfree_errptr) = NULL;

Similar comment on style for these following documentation in cleanup.h
That is tricky in this case but you can at least declare them and set
them NULL just before they are conditionally assigned.

> +	void *in_payload __free(kfree_errptr) = NULL;
> +	struct device *dev = &uctx->fwctl->dev;
> +	union pds_core_adminq_comp comp = {0};
> +	dma_addr_t out_payload_dma_addr = 0;
> +	union pds_core_adminq_cmd cmd = {0};
> +	dma_addr_t in_payload_dma_addr = 0;
> +	void *out = NULL;
> +	int err;
> +
> +	err = pdsfc_validate_rpc(pdsfc, rpc, scope);
> +	if (err) {
> +		dev_err(dev, "Invalid RPC request\n");
> +		return ERR_PTR(err);
> +	}
> +
> +	if (rpc->in.len > 0) {
> +		in_payload = kzalloc(rpc->in.len, GFP_KERNEL);
> +		if (!in_payload) {
> +			dev_err(dev, "Failed to allocate in_payload\n");
> +			out = ERR_PTR(-ENOMEM);
> +			goto done;

As before avoid the gotos mixed with free.
Easiest might be a little helper function for this setup of
the input buffer and one for the output buffer.
Probably not combined with __free that isn't giving much advantage
here anyway.

For this particular one can just return the error anyway as
nothing to do.

> +		}
> +
> +		if (copy_from_user(in_payload, u64_to_user_ptr(rpc->in.payload),
> +				   rpc->in.len)) {
> +			dev_err(dev, "Failed to copy in_payload from user\n");
> +			out = ERR_PTR(-EFAULT);
> +			goto done;
> +		}
> +
> +		in_payload_dma_addr = dma_map_single(dev->parent, in_payload,
> +						     rpc->in.len, DMA_TO_DEVICE);
> +		err = dma_mapping_error(dev->parent, in_payload_dma_addr);
> +		if (err) {
> +			dev_err(dev, "Failed to map in_payload\n");
> +			out = ERR_PTR(err);
> +			goto done;
> +		}
> +	}
> +
> +	if (rpc->out.len > 0) {
> +		out_payload = kzalloc(rpc->out.len, GFP_KERNEL);
> +		if (!out_payload) {
> +			dev_err(dev, "Failed to allocate out_payload\n");
> +			out = ERR_PTR(-ENOMEM);
> +			goto done;
> +		}
> +
> +		out_payload_dma_addr = dma_map_single(dev->parent, out_payload,
> +						      rpc->out.len, DMA_FROM_DEVICE);
> +		err = dma_mapping_error(dev->parent, out_payload_dma_addr);
> +		if (err) {
> +			dev_err(dev, "Failed to map out_payload\n");
> +			out = ERR_PTR(err);
> +			goto done;
> +		}
> +	}
> +
> +	cmd.fwctl_rpc.opcode = PDS_FWCTL_CMD_RPC;
> +	cmd.fwctl_rpc.flags = PDS_FWCTL_RPC_IND_REQ | PDS_FWCTL_RPC_IND_RESP;
> +	cmd.fwctl_rpc.ep = cpu_to_le32(rpc->in.ep);
> +	cmd.fwctl_rpc.op = cpu_to_le32(rpc->in.op);
> +	cmd.fwctl_rpc.req_pa = cpu_to_le64(in_payload_dma_addr);
> +	cmd.fwctl_rpc.req_sz = cpu_to_le32(rpc->in.len);
> +	cmd.fwctl_rpc.resp_pa = cpu_to_le64(out_payload_dma_addr);
> +	cmd.fwctl_rpc.resp_sz = cpu_to_le32(rpc->out.len);
> +
> +	dev_dbg(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d\n",
> +		__func__, rpc->in.ep, rpc->in.op,
> +		cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
> +		cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems);
> +
> +	dynamic_hex_dump("in ", DUMP_PREFIX_OFFSET, 16, 1, in_payload, rpc->in.len, true);
> +
> +	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
> +	if (err) {
> +		dev_err(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d err %d\n",
> +			__func__, rpc->in.ep, rpc->in.op,
> +			cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
> +			cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems,
> +			err);
> +		out = ERR_PTR(err);
> +		goto done;
> +	}
> +
> +	dynamic_hex_dump("out ", DUMP_PREFIX_OFFSET, 16, 1, out_payload, rpc->out.len, true);
> +
> +	dev_dbg(dev, "%s: status %d comp_index %d err %d resp_sz %d color %d\n",
> +		__func__, comp.fwctl_rpc.status, comp.fwctl_rpc.comp_index,
> +		comp.fwctl_rpc.err, comp.fwctl_rpc.resp_sz,
> +		comp.fwctl_rpc.color);
> +
> +	if (copy_to_user(u64_to_user_ptr(rpc->out.payload), out_payload, rpc->out.len)) {
> +		dev_err(dev, "Failed to copy out_payload to user\n");
> +		out = ERR_PTR(-EFAULT);
> +		goto done;
> +	}
> +
> +	rpc->out.retval = le32_to_cpu(comp.fwctl_rpc.err);
> +	*out_len = in_len;
> +	out = in;
> +
> +done:
> +	if (in_payload_dma_addr)
> +		dma_unmap_single(dev->parent, in_payload_dma_addr,
> +				 rpc->in.len, DMA_TO_DEVICE);
> +
> +	if (out_payload_dma_addr)
> +		dma_unmap_single(dev->parent, out_payload_dma_addr,
> +				 rpc->out.len, DMA_FROM_DEVICE);
> +
> +	return out;
>  }
>  
>  static const struct fwctl_ops pdsfc_ops = {
> @@ -150,16 +504,23 @@ static int pdsfc_probe(struct auxiliary_device *adev,
>  		return err;
>  	}
>  
> +	err = pdsfc_init_endpoints(pdsfc);
> +	if (err) {
> +		dev_err(dev, "Failed to init endpoints, err %d\n", err);
> +		goto free_ident;
> +	}
> +
>  	err = fwctl_register(&pdsfc->fwctl);
>  	if (err) {
>  		dev_err(dev, "Failed to register device, err %d\n", err);
> -		return err;
> +		goto free_endpoints;

Mixing the __free() magic and gotos is 'probably' ok in this case
but high risk.

https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/cleanup.h#L135
Makes a fairly strong statement on this.  I'd suggest either figuring
out a code reorg that avoids need for gotos or stopping using __free in this
function.  This looks like similar question to earlier one of
why are these cached as opposed to done inside open/close callbacks
for specific RPC calls?

>  	}
> -

Noise that shouldn't be here.

>  	auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
>  
>  	return 0;
>  
> +free_endpoints:
> +	pdsfc_free_endpoints(pdsfc);
>  free_ident:
>  	pdsfc_free_ident(pdsfc);
>  	return err;
> @@ -170,6 +531,8 @@ static void pdsfc_remove(struct auxiliary_device *adev)
>  	struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>  
>  	fwctl_unregister(&pdsfc->fwctl);
> +	pdsfc_free_operations(pdsfc);
> +	pdsfc_free_endpoints(pdsfc);
>  	pdsfc_free_ident(pdsfc);
>  }



> +/**
> + * struct pds_fwctl_query_data - query data structure
> + * @version:     Version of the query data structure
> + * @rsvd:        Word boundary padding
> + * @num_entries: Number of entries in the union
> + * @entries:     Array of query data entries, depending on the entity type.
> + */
> +struct pds_fwctl_query_data {
> +	u8      version;
> +	u8      rsvd[3];
> +	__le32  num_entries;
> +	uint8_t entries[];
__counted_by_le(num_entries)
probably appropriate here.
> +} __packed;
> +

> +/**
> + * struct pds_sg_elem - Transmit scatter-gather (SG) descriptor element
> + * @addr:	DMA address of SG element data buffer
> + * @len:	Length of SG element data buffer, in bytes
> + * @rsvd:	Word boundary padding
> + */
> +struct pds_sg_elem {
> +	__le64 addr;
> +	__le32 len;
> +	__le16 rsvd[2];

Why not an __le32?
It's reserved and naturally aligned so who cares on type ;)

> +} __packed;

Re: [RFC PATCH fwctl 4/5] pds_fwctl: add rpc and query support

[Nelson, Shannon]

On 2/12/2025 4:47 AM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 15:48:53 -0800
> Shannon Nelson <shannon.nelson@amd.com> wrote:
> 
>> From: Brett Creeley <brett.creeley@amd.com>
>>
>> The pds_fwctl driver doesn't know what RPC operations are available
>> in the firmware, so also doesn't know what scope they might have.  The
>> userland utility supplies the firmware "endpoint" and "operation" id values
>> and this driver queries the firmware for endpoints and their available
>> operations.  The operation descriptions include the scope information
>> which the driver uses for scope testing.
>>
>> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> Various comments inline.  I haven't looked closely at the actual interface
> yet though as running out of time today.
> 
> Jonathan
> 
>> ---
>>   drivers/fwctl/pds/main.c       | 369 ++++++++++++++++++++++++++++++++-
>>   include/linux/pds/pds_adminq.h | 187 +++++++++++++++++
>>   include/uapi/fwctl/pds.h       |  16 ++
>>   3 files changed, 569 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
>> index 24979fe0deea..b60a66ef1fac 100644
>> --- a/drivers/fwctl/pds/main.c
>> +++ b/drivers/fwctl/pds/main.c
>> @@ -15,12 +15,22 @@
>>   #include <linux/pds/pds_adminq.h>
>>   #include <linux/pds/pds_auxbus.h>
>>
>> +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T));
>> +DEFINE_FREE(kvfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T));
> 
> I'm lost. These look same as the ones in slab.h for kfree and kvfree
> which already handle error pointers.  Maybe based on an old kernel?
> 
>> +static struct pds_fwctl_query_data *pdsfc_get_operations(struct pdsfc_dev *pdsfc,
>> +                                                      dma_addr_t *pa, u32 ep)
>> +{
>> +     struct pds_fwctl_query_data_operation *entries = NULL;
> 
> Always set before use so don't initialize here.

Sure.

> 
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     union pds_core_adminq_comp comp = {0};
>> +     union pds_core_adminq_cmd cmd = {0};
>> +     struct pds_fwctl_query_data *data;
>> +     dma_addr_t data_pa;
>> +     int err;
>> +     int i;
>> +
>> +     /* Query the operations list for the given endpoint */
>> +     data = dma_alloc_coherent(dev->parent, PAGE_SIZE, &data_pa, GFP_KERNEL);
>> +     err = dma_mapping_error(dev->parent, data_pa);
>> +     if (err) {
>> +             dev_err(dev, "Failed to map operations list\n");
>> +             return ERR_PTR(err);
>> +     }
>> +
>> +     cmd.fwctl_query.opcode = PDS_FWCTL_CMD_QUERY;
>> +     cmd.fwctl_query.entity = PDS_FWCTL_RPC_ENDPOINT;
>> +     cmd.fwctl_query.version = 0;
>> +     cmd.fwctl_query.query_data_buf_len = cpu_to_le32(PAGE_SIZE);
>> +     cmd.fwctl_query.query_data_buf_pa = cpu_to_le64(data_pa);
>> +     cmd.fwctl_query.ep = cpu_to_le32(ep);
>> +
>> +     err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> +     if (err) {
>> +             dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
>> +                     cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
>> +             dma_free_coherent(dev->parent, PAGE_SIZE, data, data_pa);
>> +             return ERR_PTR(err);
>> +     }
>> +
>> +     *pa = data_pa;
>> +
>> +     entries = (struct pds_fwctl_query_data_operation *)data->entries;
>> +     dev_dbg(dev, "num_entries %d\n", data->num_entries);
>> +     for (i = 0; i < data->num_entries; i++)
>> +             dev_dbg(dev, "endpoint %d operation: id %x scope %d\n",
>> +                     ep, entries[i].id, entries[i].scope);
>> +
>> +     return data;
>> +}
> 
>> +
>>   static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
>>                          void *in, size_t in_len, size_t *out_len)
>>   {
>> -     return NULL;
>> +     struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
>> +     struct fwctl_rpc_pds *rpc = (struct fwctl_rpc_pds *)in;
> In is a void * so never any need to cast it to another pointer type.
> 
>> +     void *out_payload __free(kfree_errptr) = NULL;
> 
> Similar comment on style for these following documentation in cleanup.h
> That is tricky in this case but you can at least declare them and set
> them NULL just before they are conditionally assigned.

I'll look at these.

> 
>> +     void *in_payload __free(kfree_errptr) = NULL;
>> +     struct device *dev = &uctx->fwctl->dev;
>> +     union pds_core_adminq_comp comp = {0};
>> +     dma_addr_t out_payload_dma_addr = 0;
>> +     union pds_core_adminq_cmd cmd = {0};
>> +     dma_addr_t in_payload_dma_addr = 0;
>> +     void *out = NULL;
>> +     int err;
>> +
>> +     err = pdsfc_validate_rpc(pdsfc, rpc, scope);
>> +     if (err) {
>> +             dev_err(dev, "Invalid RPC request\n");
>> +             return ERR_PTR(err);
>> +     }
>> +
>> +     if (rpc->in.len > 0) {
>> +             in_payload = kzalloc(rpc->in.len, GFP_KERNEL);
>> +             if (!in_payload) {
>> +                     dev_err(dev, "Failed to allocate in_payload\n");
>> +                     out = ERR_PTR(-ENOMEM);
>> +                     goto done;
> 
> As before avoid the gotos mixed with free.
> Easiest might be a little helper function for this setup of
> the input buffer and one for the output buffer.
> Probably not combined with __free that isn't giving much advantage
> here anyway.
> 
> For this particular one can just return the error anyway as
> nothing to do.

Thanks

> 
>> +             }
>> +
>> +             if (copy_from_user(in_payload, u64_to_user_ptr(rpc->in.payload),
>> +                                rpc->in.len)) {
>> +                     dev_err(dev, "Failed to copy in_payload from user\n");
>> +                     out = ERR_PTR(-EFAULT);
>> +                     goto done;
>> +             }
>> +
>> +             in_payload_dma_addr = dma_map_single(dev->parent, in_payload,
>> +                                                  rpc->in.len, DMA_TO_DEVICE);
>> +             err = dma_mapping_error(dev->parent, in_payload_dma_addr);
>> +             if (err) {
>> +                     dev_err(dev, "Failed to map in_payload\n");
>> +                     out = ERR_PTR(err);
>> +                     goto done;
>> +             }
>> +     }
>> +
>> +     if (rpc->out.len > 0) {
>> +             out_payload = kzalloc(rpc->out.len, GFP_KERNEL);
>> +             if (!out_payload) {
>> +                     dev_err(dev, "Failed to allocate out_payload\n");
>> +                     out = ERR_PTR(-ENOMEM);
>> +                     goto done;
>> +             }
>> +
>> +             out_payload_dma_addr = dma_map_single(dev->parent, out_payload,
>> +                                                   rpc->out.len, DMA_FROM_DEVICE);
>> +             err = dma_mapping_error(dev->parent, out_payload_dma_addr);
>> +             if (err) {
>> +                     dev_err(dev, "Failed to map out_payload\n");
>> +                     out = ERR_PTR(err);
>> +                     goto done;
>> +             }
>> +     }
>> +
>> +     cmd.fwctl_rpc.opcode = PDS_FWCTL_CMD_RPC;
>> +     cmd.fwctl_rpc.flags = PDS_FWCTL_RPC_IND_REQ | PDS_FWCTL_RPC_IND_RESP;
>> +     cmd.fwctl_rpc.ep = cpu_to_le32(rpc->in.ep);
>> +     cmd.fwctl_rpc.op = cpu_to_le32(rpc->in.op);
>> +     cmd.fwctl_rpc.req_pa = cpu_to_le64(in_payload_dma_addr);
>> +     cmd.fwctl_rpc.req_sz = cpu_to_le32(rpc->in.len);
>> +     cmd.fwctl_rpc.resp_pa = cpu_to_le64(out_payload_dma_addr);
>> +     cmd.fwctl_rpc.resp_sz = cpu_to_le32(rpc->out.len);
>> +
>> +     dev_dbg(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d\n",
>> +             __func__, rpc->in.ep, rpc->in.op,
>> +             cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
>> +             cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems);
>> +
>> +     dynamic_hex_dump("in ", DUMP_PREFIX_OFFSET, 16, 1, in_payload, rpc->in.len, true);
>> +
>> +     err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> +     if (err) {
>> +             dev_err(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d err %d\n",
>> +                     __func__, rpc->in.ep, rpc->in.op,
>> +                     cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
>> +                     cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems,
>> +                     err);
>> +             out = ERR_PTR(err);
>> +             goto done;
>> +     }
>> +
>> +     dynamic_hex_dump("out ", DUMP_PREFIX_OFFSET, 16, 1, out_payload, rpc->out.len, true);
>> +
>> +     dev_dbg(dev, "%s: status %d comp_index %d err %d resp_sz %d color %d\n",
>> +             __func__, comp.fwctl_rpc.status, comp.fwctl_rpc.comp_index,
>> +             comp.fwctl_rpc.err, comp.fwctl_rpc.resp_sz,
>> +             comp.fwctl_rpc.color);
>> +
>> +     if (copy_to_user(u64_to_user_ptr(rpc->out.payload), out_payload, rpc->out.len)) {
>> +             dev_err(dev, "Failed to copy out_payload to user\n");
>> +             out = ERR_PTR(-EFAULT);
>> +             goto done;
>> +     }
>> +
>> +     rpc->out.retval = le32_to_cpu(comp.fwctl_rpc.err);
>> +     *out_len = in_len;
>> +     out = in;
>> +
>> +done:
>> +     if (in_payload_dma_addr)
>> +             dma_unmap_single(dev->parent, in_payload_dma_addr,
>> +                              rpc->in.len, DMA_TO_DEVICE);
>> +
>> +     if (out_payload_dma_addr)
>> +             dma_unmap_single(dev->parent, out_payload_dma_addr,
>> +                              rpc->out.len, DMA_FROM_DEVICE);
>> +
>> +     return out;
>>   }
>>
>>   static const struct fwctl_ops pdsfc_ops = {
>> @@ -150,16 +504,23 @@ static int pdsfc_probe(struct auxiliary_device *adev,
>>                return err;
>>        }
>>
>> +     err = pdsfc_init_endpoints(pdsfc);
>> +     if (err) {
>> +             dev_err(dev, "Failed to init endpoints, err %d\n", err);
>> +             goto free_ident;
>> +     }
>> +
>>        err = fwctl_register(&pdsfc->fwctl);
>>        if (err) {
>>                dev_err(dev, "Failed to register device, err %d\n", err);
>> -             return err;
>> +             goto free_endpoints;
> 
> Mixing the __free() magic and gotos is 'probably' ok in this case
> but high risk.
> 
> https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/cleanup.h#L135
> Makes a fairly strong statement on this.  I'd suggest either figuring
> out a code reorg that avoids need for gotos or stopping using __free in this
> function.  This looks like similar question to earlier one of
> why are these cached as opposed to done inside open/close callbacks
> for specific RPC calls?

Thanks.

> 
>>        }
>> -
> 
> Noise that shouldn't be here.

I'll clean the earlier patch

> 
>>        auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
>>
>>        return 0;
>>
>> +free_endpoints:
>> +     pdsfc_free_endpoints(pdsfc);
>>   free_ident:
>>        pdsfc_free_ident(pdsfc);
>>        return err;
>> @@ -170,6 +531,8 @@ static void pdsfc_remove(struct auxiliary_device *adev)
>>        struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>>
>>        fwctl_unregister(&pdsfc->fwctl);
>> +     pdsfc_free_operations(pdsfc);
>> +     pdsfc_free_endpoints(pdsfc);
>>        pdsfc_free_ident(pdsfc);
>>   }
> 
> 
> 
>> +/**
>> + * struct pds_fwctl_query_data - query data structure
>> + * @version:     Version of the query data structure
>> + * @rsvd:        Word boundary padding
>> + * @num_entries: Number of entries in the union
>> + * @entries:     Array of query data entries, depending on the entity type.
>> + */
>> +struct pds_fwctl_query_data {
>> +     u8      version;
>> +     u8      rsvd[3];
>> +     __le32  num_entries;
>> +     uint8_t entries[];
> __counted_by_le(num_entries)
> probably appropriate here.

I like the counted_by() stuff, but because this interface file is used 
with FW built in a different environment, I've been hesitant to add it 
in to these obvious places.  I'll see if I can add a little macro magic 
to that environment to allow the added syntax sugar.

>> +} __packed;
>> +
> 
>> +/**
>> + * struct pds_sg_elem - Transmit scatter-gather (SG) descriptor element
>> + * @addr:    DMA address of SG element data buffer
>> + * @len:     Length of SG element data buffer, in bytes
>> + * @rsvd:    Word boundary padding
>> + */
>> +struct pds_sg_elem {
>> +     __le64 addr;
>> +     __le32 len;
>> +     __le16 rsvd[2];
> 
> Why not an __le32?
> It's reserved and naturally aligned so who cares on type ;)

I think this is a leftover from earlier implementation changes, but 
doesn't make too much difference.

sln

> 
>> +} __packed;
> 

Re: [RFC PATCH fwctl 4/5] pds_fwctl: add rpc and query support

[Dave Jiang]

On 2/11/25 4:48 PM, Shannon Nelson wrote:
> From: Brett Creeley <brett.creeley@amd.com>
> 
> The pds_fwctl driver doesn't know what RPC operations are available
> in the firmware, so also doesn't know what scope they might have.  The
> userland utility supplies the firmware "endpoint" and "operation" id values
> and this driver queries the firmware for endpoints and their available
> operations.  The operation descriptions include the scope information
> which the driver uses for scope testing.
> 
> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  drivers/fwctl/pds/main.c       | 369 ++++++++++++++++++++++++++++++++-
>  include/linux/pds/pds_adminq.h | 187 +++++++++++++++++
>  include/uapi/fwctl/pds.h       |  16 ++
>  3 files changed, 569 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
> index 24979fe0deea..b60a66ef1fac 100644
> --- a/drivers/fwctl/pds/main.c
> +++ b/drivers/fwctl/pds/main.c
> @@ -15,12 +15,22 @@
>  #include <linux/pds/pds_adminq.h>
>  #include <linux/pds/pds_auxbus.h>
>  
> +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T));
> +DEFINE_FREE(kvfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T));
> +
>  struct pdsfc_uctx {
>  	struct fwctl_uctx uctx;
>  	u32 uctx_caps;
>  	u32 uctx_uid;
>  };
>  
> +struct pdsfc_rpc_endpoint_info {
> +	u32 endpoint;
> +	dma_addr_t operations_pa;
> +	struct pds_fwctl_query_data *operations;
> +	struct mutex lock;	/* lock for endpoint info management */
> +};
> +
>  struct pdsfc_dev {
>  	struct fwctl_device fwctl;
>  	struct pds_auxiliary_dev *padev;
> @@ -28,6 +38,9 @@ struct pdsfc_dev {
>  	u32 caps;
>  	dma_addr_t ident_pa;
>  	struct pds_fwctl_ident *ident;
> +	dma_addr_t endpoints_pa;
> +	struct pds_fwctl_query_data *endpoints;
> +	struct pdsfc_rpc_endpoint_info *endpoint_info;
>  };
>  DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));
>  
> @@ -112,10 +125,351 @@ static int pdsfc_identify(struct pdsfc_dev *pdsfc)
>  	return 0;
>  }
>  
> +static void pdsfc_free_endpoints(struct pdsfc_dev *pdsfc)
> +{
> +	struct device *dev = &pdsfc->fwctl.dev;
> +
> +	if (pdsfc->endpoints) {
> +		int i;
> +
> +		for (i = 0; pdsfc->endpoint_info && i < pdsfc->endpoints->num_entries; i++)
> +			mutex_destroy(&pdsfc->endpoint_info[i].lock);
> +		vfree(pdsfc->endpoint_info);
> +		pdsfc->endpoint_info = NULL;
> +		dma_free_coherent(dev->parent, PAGE_SIZE,
> +				  pdsfc->endpoints, pdsfc->endpoints_pa);
> +		pdsfc->endpoints = NULL;
> +		pdsfc->endpoints_pa = DMA_MAPPING_ERROR;
> +	}
> +}
> +
> +static void pdsfc_free_operations(struct pdsfc_dev *pdsfc)
> +{
> +	struct device *dev = &pdsfc->fwctl.dev;
> +	int i;
> +
> +	for (i = 0; i < pdsfc->endpoints->num_entries; i++) {
> +		struct pdsfc_rpc_endpoint_info *ei = &pdsfc->endpoint_info[i];
> +
> +		if (ei->operations) {
> +			dma_free_coherent(dev->parent, PAGE_SIZE,
> +					  ei->operations, ei->operations_pa);
> +			ei->operations = NULL;
> +			ei->operations_pa = DMA_MAPPING_ERROR;
> +		}
> +	}
> +}
> +
> +static struct pds_fwctl_query_data *pdsfc_get_endpoints(struct pdsfc_dev *pdsfc,
> +							dma_addr_t *pa)
> +{
> +	struct pds_fwctl_query_data_endpoint *entries = NULL;
> +	struct device *dev = &pdsfc->fwctl.dev;
> +	union pds_core_adminq_comp comp = {0};
> +	union pds_core_adminq_cmd cmd = {0};
> +	struct pds_fwctl_query_data *data;
> +	dma_addr_t data_pa;
> +	int err;
> +	int i;
> +
> +	data = dma_alloc_coherent(dev->parent, PAGE_SIZE, &data_pa, GFP_KERNEL);
> +	err = dma_mapping_error(dev, data_pa);
> +	if (err) {
> +		dev_err(dev, "Failed to map endpoint list\n");
> +		return ERR_PTR(err);
> +	}
> +
> +	cmd.fwctl_query.opcode = PDS_FWCTL_CMD_QUERY;
> +	cmd.fwctl_query.entity = PDS_FWCTL_RPC_ROOT;
> +	cmd.fwctl_query.version = 0;
> +	cmd.fwctl_query.query_data_buf_len = cpu_to_le32(PAGE_SIZE);
> +	cmd.fwctl_query.query_data_buf_pa = cpu_to_le64(data_pa);
> +
> +	dev_dbg(dev, "cmd: opcode %d entity %d version %d query_data_buf_len %d query_data_buf_pa %llx\n",
> +		cmd.fwctl_query.opcode, cmd.fwctl_query.entity, cmd.fwctl_query.version,
> +		le32_to_cpu(cmd.fwctl_query.query_data_buf_len),
> +		le64_to_cpu(cmd.fwctl_query.query_data_buf_pa));
> +
> +	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
> +	if (err) {
> +		dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
> +			cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
> +		dma_free_coherent(dev->parent, PAGE_SIZE, data, data_pa);
> +		return ERR_PTR(err);
> +	}
> +
> +	*pa = data_pa;
> +
> +	entries = (struct pds_fwctl_query_data_endpoint *)data->entries;
> +	dev_dbg(dev, "num_entries %d\n", data->num_entries);
> +	for (i = 0; i < data->num_entries; i++)

I think you need to convert num_entries from __le32 to host for the above 2 lines?

> +		dev_dbg(dev, "endpoint: id %d\n", entries[i].id);
> +
> +	return data;
> +}
> +
> +static int pdsfc_init_endpoints(struct pdsfc_dev *pdsfc)
> +{
> +	struct pds_fwctl_query_data_endpoint *ep_entry;
> +	struct device *dev = &pdsfc->fwctl.dev;
> +	int i;
> +
> +	pdsfc->endpoints = pdsfc_get_endpoints(pdsfc, &pdsfc->endpoints_pa);
> +	if (IS_ERR(pdsfc->endpoints)) {
> +		dev_err(dev, "Failed to query endpoints\n");
> +		return PTR_ERR(pdsfc->endpoints);
> +	}
> +
> +	pdsfc->endpoint_info = vcalloc(pdsfc->endpoints->num_entries,
> +				       sizeof(*pdsfc->endpoint_info));
> +	if (!pdsfc->endpoint_info) {
> +		dev_err(dev, "Failed to allocate endpoint_info array\n");
> +		pdsfc_free_endpoints(pdsfc);
> +		return -ENOMEM;
> +	}
> +
> +	ep_entry = (struct pds_fwctl_query_data_endpoint *)pdsfc->endpoints->entries;
> +	for (i = 0; i < pdsfc->endpoints->num_entries; i++) {
> +		mutex_init(&pdsfc->endpoint_info[i].lock);
> +		pdsfc->endpoint_info[i].endpoint = ep_entry[i].id;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct pds_fwctl_query_data *pdsfc_get_operations(struct pdsfc_dev *pdsfc,
> +							 dma_addr_t *pa, u32 ep)
> +{
> +	struct pds_fwctl_query_data_operation *entries = NULL;
> +	struct device *dev = &pdsfc->fwctl.dev;
> +	union pds_core_adminq_comp comp = {0};
> +	union pds_core_adminq_cmd cmd = {0};
> +	struct pds_fwctl_query_data *data;
> +	dma_addr_t data_pa;
> +	int err;
> +	int i;
> +
> +	/* Query the operations list for the given endpoint */
> +	data = dma_alloc_coherent(dev->parent, PAGE_SIZE, &data_pa, GFP_KERNEL);
> +	err = dma_mapping_error(dev->parent, data_pa);
> +	if (err) {
> +		dev_err(dev, "Failed to map operations list\n");
> +		return ERR_PTR(err);
> +	}
> +
> +	cmd.fwctl_query.opcode = PDS_FWCTL_CMD_QUERY;
> +	cmd.fwctl_query.entity = PDS_FWCTL_RPC_ENDPOINT;
> +	cmd.fwctl_query.version = 0;
> +	cmd.fwctl_query.query_data_buf_len = cpu_to_le32(PAGE_SIZE);
> +	cmd.fwctl_query.query_data_buf_pa = cpu_to_le64(data_pa);
> +	cmd.fwctl_query.ep = cpu_to_le32(ep);
> +
> +	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
> +	if (err) {
> +		dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
> +			cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
> +		dma_free_coherent(dev->parent, PAGE_SIZE, data, data_pa);
> +		return ERR_PTR(err);
> +	}
> +
> +	*pa = data_pa;
> +
> +	entries = (struct pds_fwctl_query_data_operation *)data->entries;
> +	dev_dbg(dev, "num_entries %d\n", data->num_entries);
> +	for (i = 0; i < data->num_entries; i++)
> +		dev_dbg(dev, "endpoint %d operation: id %x scope %d\n",
> +			ep, entries[i].id, entries[i].scope);
> +
> +	return data;
> +}
> +
> +static int pdsfc_validate_rpc(struct pdsfc_dev *pdsfc,
> +			      struct fwctl_rpc_pds *rpc,
> +			      enum fwctl_rpc_scope scope)
> +{
> +	struct pds_fwctl_query_data_operation *op_entry = NULL;
> +	struct pdsfc_rpc_endpoint_info *ep_info = NULL;
> +	struct device *dev = &pdsfc->fwctl.dev;
> +	int i;
> +
> +	if (!pdsfc->ident) {
> +		dev_err(dev, "Ident not available\n");
> +		return -EINVAL;
> +	}
> +
> +	/* validate rpc in_len & out_len based
> +	 * on ident->max_req_sz & max_resp_sz
> +	 */
> +	if (rpc->in.len > pdsfc->ident->max_req_sz) {
> +		dev_err(dev, "Invalid request size %u, max %u\n",
> +			rpc->in.len, pdsfc->ident->max_req_sz);
> +		return -EINVAL;
> +	}
> +
> +	if (rpc->out.len > pdsfc->ident->max_resp_sz) {
> +		dev_err(dev, "Invalid response size %u, max %u\n",
> +			rpc->out.len, pdsfc->ident->max_resp_sz);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < pdsfc->endpoints->num_entries; i++) {
> +		if (pdsfc->endpoint_info[i].endpoint == rpc->in.ep) {
> +			ep_info = &pdsfc->endpoint_info[i];
> +			break;
> +		}
> +	}
> +	if (!ep_info) {
> +		dev_err(dev, "Invalid endpoint %d\n", rpc->in.ep);
> +		return -EINVAL;
> +	}
> +
> +	/* query and cache this endpoint's operations */
> +	mutex_lock(&ep_info->lock);
> +	if (!ep_info->operations) {
> +		ep_info->operations = pdsfc_get_operations(pdsfc,
> +							   &ep_info->operations_pa,
> +							   rpc->in.ep);
> +		if (!ep_info->operations) {
> +			mutex_unlock(&ep_info->lock);
> +			dev_err(dev, "Failed to allocate operations list\n");
> +			return -ENOMEM;
> +		}
> +	}
> +	mutex_unlock(&ep_info->lock);
> +
> +	/* reject unsupported and/or out of scope commands */
> +	op_entry = (struct pds_fwctl_query_data_operation *)ep_info->operations->entries;
> +	for (i = 0; i < ep_info->operations->num_entries; i++) {
> +		if (PDS_FWCTL_RPC_OPCODE_CMP(rpc->in.op, op_entry[i].id)) {
> +			if (scope < op_entry[i].scope)
> +				return -EPERM;
> +			return 0;
> +		}
> +	}
> +
> +	dev_err(dev, "Invalid operation %d for endpoint %d\n", rpc->in.op, rpc->in.ep);
> +
> +	return -EINVAL;
> +}
> +
>  static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
>  			  void *in, size_t in_len, size_t *out_len)
>  {
> -	return NULL;
> +	struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
> +	struct fwctl_rpc_pds *rpc = (struct fwctl_rpc_pds *)in;
> +	void *out_payload __free(kfree_errptr) = NULL;
> +	void *in_payload __free(kfree_errptr) = NULL;

__free(kfree) should work from kzalloc(). No need to define special macro.

> +	struct device *dev = &uctx->fwctl->dev;
> +	union pds_core_adminq_comp comp = {0};
> +	dma_addr_t out_payload_dma_addr = 0;
> +	union pds_core_adminq_cmd cmd = {0};
> +	dma_addr_t in_payload_dma_addr = 0;
> +	void *out = NULL;
> +	int err;
> +
> +	err = pdsfc_validate_rpc(pdsfc, rpc, scope);
> +	if (err) {
> +		dev_err(dev, "Invalid RPC request\n");
> +		return ERR_PTR(err);
> +	}
> +
> +	if (rpc->in.len > 0) {
> +		in_payload = kzalloc(rpc->in.len, GFP_KERNEL);
> +		if (!in_payload) {
> +			dev_err(dev, "Failed to allocate in_payload\n");
> +			out = ERR_PTR(-ENOMEM);
> +			goto done;
> +		}
> +
> +		if (copy_from_user(in_payload, u64_to_user_ptr(rpc->in.payload),
> +				   rpc->in.len)) {
> +			dev_err(dev, "Failed to copy in_payload from user\n");
> +			out = ERR_PTR(-EFAULT);
> +			goto done;
> +		}

So the cleanup macros and gotos here make things a bit messy. But maybe you can define a transient struct:
struct payload {
	void *data;
	dma_addr_t dma;
	size_t len;
	struct device *dev;
	enum dma_data_direction dir;
};

static inline void cleanup_payload(struct payload *payload)
{
	dma_unmap_single(payload->dev, payload->dma, payload->len, payload->dir);
	kfree(payload->data);
	kfree(payload);
}
DEFINE_FREE(free_payload, struct payload *, if (_T) cleanup_payload(_T));

static struct payload *alloc_payload(struct device *dev, size_t len, enum dma_data_direction dir)
{
	dma_addr_t dma;
	int err;

	struct payload *p __free(kfree) = kzalloc(sizeof(*p), GFP_KERNEL);
	if (!p)
		return NULL;

	void *data __free(kfree) = kzalloc(len, GFP_KERNEL);
	if (!data)
		return NULL;

	dma = dma_map_single(dev, data, len, dir);
	err = dma_mapping_error(dev, dma);
	if (err)
		return NULL;

	p->dma = dma;
	p->len = len;
	p->dir = dir;
	p->data = no_free_ptr(data); 

	return no_free_ptr(p);	
}

With that you can use __free() cleanly and then when your rpc function exits, it'll unmap and free everything automatically. 

struct payload *p __free(free_payload) = alloc_payload(...);

Just a thought....

> +
> +		in_payload_dma_addr = dma_map_single(dev->parent, in_payload,
> +						     rpc->in.len, DMA_TO_DEVICE);
> +		err = dma_mapping_error(dev->parent, in_payload_dma_addr);
> +		if (err) {
> +			dev_err(dev, "Failed to map in_payload\n");
> +			out = ERR_PTR(err);
> +			goto done;
> +		}
> +	}
> +
> +	if (rpc->out.len > 0) {
> +		out_payload = kzalloc(rpc->out.len, GFP_KERNEL);
> +		if (!out_payload) {
> +			dev_err(dev, "Failed to allocate out_payload\n");
> +			out = ERR_PTR(-ENOMEM);
> +			goto done;
> +		}
> +
> +		out_payload_dma_addr = dma_map_single(dev->parent, out_payload,
> +						      rpc->out.len, DMA_FROM_DEVICE);
> +		err = dma_mapping_error(dev->parent, out_payload_dma_addr);
> +		if (err) {
> +			dev_err(dev, "Failed to map out_payload\n");
> +			out = ERR_PTR(err);
> +			goto done;
> +		}
> +	}
> +
> +	cmd.fwctl_rpc.opcode = PDS_FWCTL_CMD_RPC;
> +	cmd.fwctl_rpc.flags = PDS_FWCTL_RPC_IND_REQ | PDS_FWCTL_RPC_IND_RESP;
> +	cmd.fwctl_rpc.ep = cpu_to_le32(rpc->in.ep);
> +	cmd.fwctl_rpc.op = cpu_to_le32(rpc->in.op);
> +	cmd.fwctl_rpc.req_pa = cpu_to_le64(in_payload_dma_addr);
> +	cmd.fwctl_rpc.req_sz = cpu_to_le32(rpc->in.len);
> +	cmd.fwctl_rpc.resp_pa = cpu_to_le64(out_payload_dma_addr);
> +	cmd.fwctl_rpc.resp_sz = cpu_to_le32(rpc->out.len);
> +
> +	dev_dbg(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d\n",
> +		__func__, rpc->in.ep, rpc->in.op,
> +		cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
> +		cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems);
> +
> +	dynamic_hex_dump("in ", DUMP_PREFIX_OFFSET, 16, 1, in_payload, rpc->in.len, true);
> +
> +	err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
> +	if (err) {
> +		dev_err(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d err %d\n",
> +			__func__, rpc->in.ep, rpc->in.op,
> +			cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
> +			cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems,
> +			err);
> +		out = ERR_PTR(err);
> +		goto done;
> +	}
> +
> +	dynamic_hex_dump("out ", DUMP_PREFIX_OFFSET, 16, 1, out_payload, rpc->out.len, true);
> +
> +	dev_dbg(dev, "%s: status %d comp_index %d err %d resp_sz %d color %d\n",
> +		__func__, comp.fwctl_rpc.status, comp.fwctl_rpc.comp_index,
> +		comp.fwctl_rpc.err, comp.fwctl_rpc.resp_sz,
> +		comp.fwctl_rpc.color);
> +
> +	if (copy_to_user(u64_to_user_ptr(rpc->out.payload), out_payload, rpc->out.len)) {
> +		dev_err(dev, "Failed to copy out_payload to user\n");
> +		out = ERR_PTR(-EFAULT);
> +		goto done;
> +	}
> +
> +	rpc->out.retval = le32_to_cpu(comp.fwctl_rpc.err);
> +	*out_len = in_len;
> +	out = in;
> +
> +done:
> +	if (in_payload_dma_addr)
> +		dma_unmap_single(dev->parent, in_payload_dma_addr,
> +				 rpc->in.len, DMA_TO_DEVICE);
> +
> +	if (out_payload_dma_addr)
> +		dma_unmap_single(dev->parent, out_payload_dma_addr,
> +				 rpc->out.len, DMA_FROM_DEVICE);
> +
> +	return out;
>  }
>  
>  static const struct fwctl_ops pdsfc_ops = {
> @@ -150,16 +504,23 @@ static int pdsfc_probe(struct auxiliary_device *adev,
>  		return err;
>  	}
>  
> +	err = pdsfc_init_endpoints(pdsfc);
> +	if (err) {
> +		dev_err(dev, "Failed to init endpoints, err %d\n", err);
> +		goto free_ident;
> +	}
> +
>  	err = fwctl_register(&pdsfc->fwctl);
>  	if (err) {
>  		dev_err(dev, "Failed to register device, err %d\n", err);
> -		return err;
> +		goto free_endpoints;
>  	}
> -
>  	auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
>  
>  	return 0;
>  
> +free_endpoints:
> +	pdsfc_free_endpoints(pdsfc);
>  free_ident:
>  	pdsfc_free_ident(pdsfc);
>  	return err;
> @@ -170,6 +531,8 @@ static void pdsfc_remove(struct auxiliary_device *adev)
>  	struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>  
>  	fwctl_unregister(&pdsfc->fwctl);
> +	pdsfc_free_operations(pdsfc);
> +	pdsfc_free_endpoints(pdsfc);
>  	pdsfc_free_ident(pdsfc);
>  }
>  
> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
> index 7fc353b63353..33cd03388b15 100644
> --- a/include/linux/pds/pds_adminq.h
> +++ b/include/linux/pds/pds_adminq.h
> @@ -1181,6 +1181,8 @@ struct pds_lm_host_vf_status_cmd {
>  
>  enum pds_fwctl_cmd_opcode {
>  	PDS_FWCTL_CMD_IDENT		= 70,
> +	PDS_FWCTL_CMD_RPC		= 71,
> +	PDS_FWCTL_CMD_QUERY		= 72,
>  };
>  
>  /**
> @@ -1251,6 +1253,187 @@ struct pds_fwctl_ident {
>  	u8     max_resp_sg_elems;
>  } __packed;
>  
> +enum pds_fwctl_query_entity {
> +	PDS_FWCTL_RPC_ROOT	= 0,
> +	PDS_FWCTL_RPC_ENDPOINT	= 1,
> +	PDS_FWCTL_RPC_OPERATION	= 2,
> +};
> +
> +#define PDS_FWCTL_RPC_OPCODE_CMD_SHIFT	0
> +#define PDS_FWCTL_RPC_OPCODE_CMD_MASK	GENMASK(15, PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)
> +#define PDS_FWCTL_RPC_OPCODE_VER_SHIFT	16
> +#define PDS_FWCTL_RPC_OPCODE_VER_MASK	GENMASK(23, PDS_FWCTL_RPC_OPCODE_VER_SHIFT)
> +
> +#define PDS_FWCTL_RPC_OPCODE_GET_CMD(op)	\
> +	(((op) & PDS_FWCTL_RPC_OPCODE_CMD_MASK) >> PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)
> +#define PDS_FWCTL_RPC_OPCODE_GET_VER(op)	\
> +	(((op) & PDS_FWCTL_RPC_OPCODE_VER_MASK) >> PDS_FWCTL_RPC_OPCODE_VER_SHIFT)
> +
> +#define PDS_FWCTL_RPC_OPCODE_CMP(op1, op2) \
> +	(PDS_FWCTL_RPC_OPCODE_GET_CMD(op1) == PDS_FWCTL_RPC_OPCODE_GET_CMD(op2) && \
> +	 PDS_FWCTL_RPC_OPCODE_GET_VER(op1) <= PDS_FWCTL_RPC_OPCODE_GET_VER(op2))
> +
> +/**
> + * struct pds_fwctl_query_cmd - Firmware control query command structure
> + * @opcode: Operation code for the command
> + * @entity:  Entity type to query (enum pds_fwctl_query_entity)
> + * @version: Version of the query data structure supported by the driver
> + * @rsvd:    Word boundary padding
> + * @query_data_buf_len: Length of the query data buffer
> + * @query_data_buf_pa:  Physical address of the query data buffer
> + * @ep:      Endpoint identifier to query  (when entity is PDS_FWCTL_RPC_ENDPOINT)
> + * @op:      Operation identifier to query (when entity is PDS_FWCTL_RPC_OPERATION)
> + *
> + * This structure is used to send a query command to the firmware control
> + * interface. The structure is packed to ensure there is no padding between
> + * the fields.
> + */
> +struct pds_fwctl_query_cmd {
> +	u8     opcode;
> +	u8     entity;
> +	u8     version;
> +	u8     rsvd;
> +	__le32 query_data_buf_len;
> +	__le64 query_data_buf_pa;
> +	union {
> +		__le32 ep;
> +		__le32 op;
> +	};
> +} __packed;
> +
> +/**
> + * struct pds_fwctl_query_comp - Firmware control query completion structure
> + * @status:     Status of the query command
> + * @rsvd:       Word boundary padding
> + * @comp_index: Completion index in little-endian format
> + * @version:    Version of the query data structure returned by firmware. This
> + *		 should be less than or equal to the version supported by the driver.
> + * @rsvd2:      Word boundary padding
> + * @color:      Color bit indicating the state of the completion
> + */
> +struct pds_fwctl_query_comp {
> +	u8     status;
> +	u8     rsvd;
> +	__le16 comp_index;
> +	u8     version;
> +	u8     rsvd2[2];
> +	u8     color;
> +} __packed;
> +
> +/**
> + * struct pds_fwctl_query_data_endpoint - query data for entity PDS_FWCTL_RPC_ROOT
> + * @id: The identifier for the data endpoint.
> + */
> +struct pds_fwctl_query_data_endpoint {
> +	__le32 id;
> +} __packed;
> +
> +/**
> + * struct pds_fwctl_query_data_operation - query data for entity PDS_FWCTL_RPC_ENDPOINT
> + * @id:    Operation identifier.
> + * @scope: Scope of the operation (enum fwctl_rpc_scope).
> + * @rsvd:  Word boundary padding
> + */
> +struct pds_fwctl_query_data_operation {
> +	__le32 id;
> +	u8     scope;
> +	u8     rsvd[3];
> +} __packed;
> +
> +/**
> + * struct pds_fwctl_query_data - query data structure
> + * @version:     Version of the query data structure
> + * @rsvd:        Word boundary padding
> + * @num_entries: Number of entries in the union
> + * @entries:     Array of query data entries, depending on the entity type.
> + */
> +struct pds_fwctl_query_data {
> +	u8      version;
> +	u8      rsvd[3];
> +	__le32  num_entries;
> +	uint8_t entries[];
Why u8 above but uint8_t here?

u8 entries[] __counted_by_le(num_entries);

DJ

> +} __packed;
> +
> +/**
> + * struct pds_fwctl_rpc_cmd - Firmware control RPC command.
> + * @opcode:        opcode PDS_FWCTL_CMD_RPC
> + * @rsvd:          Word boundary padding
> + * @flags:         Indicates indirect request and/or response handling
> + * @ep:            Endpoint identifier.
> + * @op:            Operation identifier.
> + * @inline_req0:   Buffer for inline request
> + * @inline_req1:   Buffer for inline request
> + * @req_pa:        Physical address of request data.
> + * @req_sz:        Size of the request.
> + * @req_sg_elems:  Number of request SGs
> + * @req_rsvd:      Word boundary padding
> + * @inline_req2:   Buffer for inline request
> + * @resp_pa:       Physical address of response data.
> + * @resp_sz:       Size of the response.
> + * @resp_sg_elems: Number of response SGs
> + * @resp_rsvd:     Word boundary padding
> + */
> +struct pds_fwctl_rpc_cmd {
> +	u8     opcode;
> +	u8     rsvd;
> +	__le16 flags;
> +#define PDS_FWCTL_RPC_IND_REQ		0x1
> +#define PDS_FWCTL_RPC_IND_RESP		0x2
> +	__le32 ep;
> +	__le32 op;
> +	u8 inline_req0[16];
> +	union {
> +		u8 inline_req1[16];
> +		struct {
> +			__le64 req_pa;
> +			__le32 req_sz;
> +			u8     req_sg_elems;
> +			u8     req_rsvd[3];
> +		};
> +	};
> +	union {
> +		u8 inline_req2[16];
> +		struct {
> +			__le64 resp_pa;
> +			__le32 resp_sz;
> +			u8     resp_sg_elems;
> +			u8     resp_rsvd[3];
> +		};
> +	};
> +} __packed;
> +
> +/**
> + * struct pds_sg_elem - Transmit scatter-gather (SG) descriptor element
> + * @addr:	DMA address of SG element data buffer
> + * @len:	Length of SG element data buffer, in bytes
> + * @rsvd:	Word boundary padding
> + */
> +struct pds_sg_elem {
> +	__le64 addr;
> +	__le32 len;
> +	__le16 rsvd[2];
> +} __packed;
> +
> +/**
> + * struct pds_fwctl_rpc_comp - Completion of a firmware control RPC.
> + * @status:     Status of the command
> + * @rsvd:       Word boundary padding
> + * @comp_index: Completion index of the command
> + * @err:        Error code, if any, from the RPC.
> + * @resp_sz:    Size of the response.
> + * @rsvd2:      Word boundary padding
> + * @color:      Color bit indicating the state of the completion.
> + */
> +struct pds_fwctl_rpc_comp {
> +	u8     status;
> +	u8     rsvd;
> +	__le16 comp_index;
> +	__le32 err;
> +	__le32 resp_sz;
> +	u8     rsvd2[3];
> +	u8     color;
> +} __packed;
> +
>  union pds_core_adminq_cmd {
>  	u8     opcode;
>  	u8     bytes[64];
> @@ -1291,6 +1474,8 @@ union pds_core_adminq_cmd {
>  
>  	struct pds_fwctl_cmd		  fwctl;
>  	struct pds_fwctl_ident_cmd	  fwctl_ident;
> +	struct pds_fwctl_rpc_cmd	  fwctl_rpc;
> +	struct pds_fwctl_query_cmd	  fwctl_query;
>  };
>  
>  union pds_core_adminq_comp {
> @@ -1320,6 +1505,8 @@ union pds_core_adminq_comp {
>  	struct pds_lm_dirty_status_comp	  lm_dirty_status;
>  
>  	struct pds_fwctl_comp		  fwctl;
> +	struct pds_fwctl_rpc_comp	  fwctl_rpc;
> +	struct pds_fwctl_query_comp	  fwctl_query;
>  };
>  
>  #ifndef __CHECKER__
> diff --git a/include/uapi/fwctl/pds.h b/include/uapi/fwctl/pds.h
> index a01b032cbdb1..da6cd2d1c6fa 100644
> --- a/include/uapi/fwctl/pds.h
> +++ b/include/uapi/fwctl/pds.h
> @@ -24,4 +24,20 @@ enum pds_fwctl_capabilities {
>  	PDS_FWCTL_QUERY_CAP = 0,
>  	PDS_FWCTL_SEND_CAP,
>  };
> +
> +struct fwctl_rpc_pds {
> +	struct {
> +		__u32 op;
> +		__u32 ep;
> +		__u32 rsvd;
> +		__u32 len;
> +		__u64 payload;
> +	} in;
> +	struct {
> +		__u32 retval;
> +		__u32 rsvd[2];
> +		__u32 len;
> +		__u64 payload;
> +	} out;
> +};
>  #endif /* _UAPI_FWCTL_PDS_H_ */

Re: [RFC PATCH fwctl 4/5] pds_fwctl: add rpc and query support

[Nelson, Shannon]

On 2/12/2025 5:02 PM, Dave Jiang wrote:
> 
> On 2/11/25 4:48 PM, Shannon Nelson wrote:
>> From: Brett Creeley <brett.creeley@amd.com>
>>
>> The pds_fwctl driver doesn't know what RPC operations are available
>> in the firmware, so also doesn't know what scope they might have.  The
>> userland utility supplies the firmware "endpoint" and "operation" id values
>> and this driver queries the firmware for endpoints and their available
>> operations.  The operation descriptions include the scope information
>> which the driver uses for scope testing.
>>
>> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>>   drivers/fwctl/pds/main.c       | 369 ++++++++++++++++++++++++++++++++-
>>   include/linux/pds/pds_adminq.h | 187 +++++++++++++++++
>>   include/uapi/fwctl/pds.h       |  16 ++
>>   3 files changed, 569 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
>> index 24979fe0deea..b60a66ef1fac 100644
>> --- a/drivers/fwctl/pds/main.c
>> +++ b/drivers/fwctl/pds/main.c
>> @@ -15,12 +15,22 @@
>>   #include <linux/pds/pds_adminq.h>
>>   #include <linux/pds/pds_auxbus.h>
>>
>> +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T));
>> +DEFINE_FREE(kvfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T));
>> +
>>   struct pdsfc_uctx {
>>        struct fwctl_uctx uctx;
>>        u32 uctx_caps;
>>        u32 uctx_uid;
>>   };
>>
>> +struct pdsfc_rpc_endpoint_info {
>> +     u32 endpoint;
>> +     dma_addr_t operations_pa;
>> +     struct pds_fwctl_query_data *operations;
>> +     struct mutex lock;      /* lock for endpoint info management */
>> +};
>> +
>>   struct pdsfc_dev {
>>        struct fwctl_device fwctl;
>>        struct pds_auxiliary_dev *padev;
>> @@ -28,6 +38,9 @@ struct pdsfc_dev {
>>        u32 caps;
>>        dma_addr_t ident_pa;
>>        struct pds_fwctl_ident *ident;
>> +     dma_addr_t endpoints_pa;
>> +     struct pds_fwctl_query_data *endpoints;
>> +     struct pdsfc_rpc_endpoint_info *endpoint_info;
>>   };
>>   DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));
>>
>> @@ -112,10 +125,351 @@ static int pdsfc_identify(struct pdsfc_dev *pdsfc)
>>        return 0;
>>   }
>>
>> +static void pdsfc_free_endpoints(struct pdsfc_dev *pdsfc)
>> +{
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +
>> +     if (pdsfc->endpoints) {
>> +             int i;
>> +
>> +             for (i = 0; pdsfc->endpoint_info && i < pdsfc->endpoints->num_entries; i++)
>> +                     mutex_destroy(&pdsfc->endpoint_info[i].lock);
>> +             vfree(pdsfc->endpoint_info);
>> +             pdsfc->endpoint_info = NULL;
>> +             dma_free_coherent(dev->parent, PAGE_SIZE,
>> +                               pdsfc->endpoints, pdsfc->endpoints_pa);
>> +             pdsfc->endpoints = NULL;
>> +             pdsfc->endpoints_pa = DMA_MAPPING_ERROR;
>> +     }
>> +}
>> +
>> +static void pdsfc_free_operations(struct pdsfc_dev *pdsfc)
>> +{
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     int i;
>> +
>> +     for (i = 0; i < pdsfc->endpoints->num_entries; i++) {
>> +             struct pdsfc_rpc_endpoint_info *ei = &pdsfc->endpoint_info[i];
>> +
>> +             if (ei->operations) {
>> +                     dma_free_coherent(dev->parent, PAGE_SIZE,
>> +                                       ei->operations, ei->operations_pa);
>> +                     ei->operations = NULL;
>> +                     ei->operations_pa = DMA_MAPPING_ERROR;
>> +             }
>> +     }
>> +}
>> +
>> +static struct pds_fwctl_query_data *pdsfc_get_endpoints(struct pdsfc_dev *pdsfc,
>> +                                                     dma_addr_t *pa)
>> +{
>> +     struct pds_fwctl_query_data_endpoint *entries = NULL;
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     union pds_core_adminq_comp comp = {0};
>> +     union pds_core_adminq_cmd cmd = {0};
>> +     struct pds_fwctl_query_data *data;
>> +     dma_addr_t data_pa;
>> +     int err;
>> +     int i;
>> +
>> +     data = dma_alloc_coherent(dev->parent, PAGE_SIZE, &data_pa, GFP_KERNEL);
>> +     err = dma_mapping_error(dev, data_pa);
>> +     if (err) {
>> +             dev_err(dev, "Failed to map endpoint list\n");
>> +             return ERR_PTR(err);
>> +     }
>> +
>> +     cmd.fwctl_query.opcode = PDS_FWCTL_CMD_QUERY;
>> +     cmd.fwctl_query.entity = PDS_FWCTL_RPC_ROOT;
>> +     cmd.fwctl_query.version = 0;
>> +     cmd.fwctl_query.query_data_buf_len = cpu_to_le32(PAGE_SIZE);
>> +     cmd.fwctl_query.query_data_buf_pa = cpu_to_le64(data_pa);
>> +
>> +     dev_dbg(dev, "cmd: opcode %d entity %d version %d query_data_buf_len %d query_data_buf_pa %llx\n",
>> +             cmd.fwctl_query.opcode, cmd.fwctl_query.entity, cmd.fwctl_query.version,
>> +             le32_to_cpu(cmd.fwctl_query.query_data_buf_len),
>> +             le64_to_cpu(cmd.fwctl_query.query_data_buf_pa));
>> +
>> +     err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> +     if (err) {
>> +             dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
>> +                     cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
>> +             dma_free_coherent(dev->parent, PAGE_SIZE, data, data_pa);
>> +             return ERR_PTR(err);
>> +     }
>> +
>> +     *pa = data_pa;
>> +
>> +     entries = (struct pds_fwctl_query_data_endpoint *)data->entries;
>> +     dev_dbg(dev, "num_entries %d\n", data->num_entries);
>> +     for (i = 0; i < data->num_entries; i++)
> 
> I think you need to convert num_entries from __le32 to host for the above 2 lines?

Hmm, I'll check that.

> 
>> +             dev_dbg(dev, "endpoint: id %d\n", entries[i].id);
>> +
>> +     return data;
>> +}
>> +
>> +static int pdsfc_init_endpoints(struct pdsfc_dev *pdsfc)
>> +{
>> +     struct pds_fwctl_query_data_endpoint *ep_entry;
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     int i;
>> +
>> +     pdsfc->endpoints = pdsfc_get_endpoints(pdsfc, &pdsfc->endpoints_pa);
>> +     if (IS_ERR(pdsfc->endpoints)) {
>> +             dev_err(dev, "Failed to query endpoints\n");
>> +             return PTR_ERR(pdsfc->endpoints);
>> +     }
>> +
>> +     pdsfc->endpoint_info = vcalloc(pdsfc->endpoints->num_entries,
>> +                                    sizeof(*pdsfc->endpoint_info));
>> +     if (!pdsfc->endpoint_info) {
>> +             dev_err(dev, "Failed to allocate endpoint_info array\n");
>> +             pdsfc_free_endpoints(pdsfc);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     ep_entry = (struct pds_fwctl_query_data_endpoint *)pdsfc->endpoints->entries;
>> +     for (i = 0; i < pdsfc->endpoints->num_entries; i++) {
>> +             mutex_init(&pdsfc->endpoint_info[i].lock);
>> +             pdsfc->endpoint_info[i].endpoint = ep_entry[i].id;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static struct pds_fwctl_query_data *pdsfc_get_operations(struct pdsfc_dev *pdsfc,
>> +                                                      dma_addr_t *pa, u32 ep)
>> +{
>> +     struct pds_fwctl_query_data_operation *entries = NULL;
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     union pds_core_adminq_comp comp = {0};
>> +     union pds_core_adminq_cmd cmd = {0};
>> +     struct pds_fwctl_query_data *data;
>> +     dma_addr_t data_pa;
>> +     int err;
>> +     int i;
>> +
>> +     /* Query the operations list for the given endpoint */
>> +     data = dma_alloc_coherent(dev->parent, PAGE_SIZE, &data_pa, GFP_KERNEL);
>> +     err = dma_mapping_error(dev->parent, data_pa);
>> +     if (err) {
>> +             dev_err(dev, "Failed to map operations list\n");
>> +             return ERR_PTR(err);
>> +     }
>> +
>> +     cmd.fwctl_query.opcode = PDS_FWCTL_CMD_QUERY;
>> +     cmd.fwctl_query.entity = PDS_FWCTL_RPC_ENDPOINT;
>> +     cmd.fwctl_query.version = 0;
>> +     cmd.fwctl_query.query_data_buf_len = cpu_to_le32(PAGE_SIZE);
>> +     cmd.fwctl_query.query_data_buf_pa = cpu_to_le64(data_pa);
>> +     cmd.fwctl_query.ep = cpu_to_le32(ep);
>> +
>> +     err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> +     if (err) {
>> +             dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n",
>> +                     cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err);
>> +             dma_free_coherent(dev->parent, PAGE_SIZE, data, data_pa);
>> +             return ERR_PTR(err);
>> +     }
>> +
>> +     *pa = data_pa;
>> +
>> +     entries = (struct pds_fwctl_query_data_operation *)data->entries;
>> +     dev_dbg(dev, "num_entries %d\n", data->num_entries);
>> +     for (i = 0; i < data->num_entries; i++)
>> +             dev_dbg(dev, "endpoint %d operation: id %x scope %d\n",
>> +                     ep, entries[i].id, entries[i].scope);
>> +
>> +     return data;
>> +}
>> +
>> +static int pdsfc_validate_rpc(struct pdsfc_dev *pdsfc,
>> +                           struct fwctl_rpc_pds *rpc,
>> +                           enum fwctl_rpc_scope scope)
>> +{
>> +     struct pds_fwctl_query_data_operation *op_entry = NULL;
>> +     struct pdsfc_rpc_endpoint_info *ep_info = NULL;
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     int i;
>> +
>> +     if (!pdsfc->ident) {
>> +             dev_err(dev, "Ident not available\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* validate rpc in_len & out_len based
>> +      * on ident->max_req_sz & max_resp_sz
>> +      */
>> +     if (rpc->in.len > pdsfc->ident->max_req_sz) {
>> +             dev_err(dev, "Invalid request size %u, max %u\n",
>> +                     rpc->in.len, pdsfc->ident->max_req_sz);
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (rpc->out.len > pdsfc->ident->max_resp_sz) {
>> +             dev_err(dev, "Invalid response size %u, max %u\n",
>> +                     rpc->out.len, pdsfc->ident->max_resp_sz);
>> +             return -EINVAL;
>> +     }
>> +
>> +     for (i = 0; i < pdsfc->endpoints->num_entries; i++) {
>> +             if (pdsfc->endpoint_info[i].endpoint == rpc->in.ep) {
>> +                     ep_info = &pdsfc->endpoint_info[i];
>> +                     break;
>> +             }
>> +     }
>> +     if (!ep_info) {
>> +             dev_err(dev, "Invalid endpoint %d\n", rpc->in.ep);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* query and cache this endpoint's operations */
>> +     mutex_lock(&ep_info->lock);
>> +     if (!ep_info->operations) {
>> +             ep_info->operations = pdsfc_get_operations(pdsfc,
>> +                                                        &ep_info->operations_pa,
>> +                                                        rpc->in.ep);
>> +             if (!ep_info->operations) {
>> +                     mutex_unlock(&ep_info->lock);
>> +                     dev_err(dev, "Failed to allocate operations list\n");
>> +                     return -ENOMEM;
>> +             }
>> +     }
>> +     mutex_unlock(&ep_info->lock);
>> +
>> +     /* reject unsupported and/or out of scope commands */
>> +     op_entry = (struct pds_fwctl_query_data_operation *)ep_info->operations->entries;
>> +     for (i = 0; i < ep_info->operations->num_entries; i++) {
>> +             if (PDS_FWCTL_RPC_OPCODE_CMP(rpc->in.op, op_entry[i].id)) {
>> +                     if (scope < op_entry[i].scope)
>> +                             return -EPERM;
>> +                     return 0;
>> +             }
>> +     }
>> +
>> +     dev_err(dev, "Invalid operation %d for endpoint %d\n", rpc->in.op, rpc->in.ep);
>> +
>> +     return -EINVAL;
>> +}
>> +
>>   static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
>>                          void *in, size_t in_len, size_t *out_len)
>>   {
>> -     return NULL;
>> +     struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl);
>> +     struct fwctl_rpc_pds *rpc = (struct fwctl_rpc_pds *)in;
>> +     void *out_payload __free(kfree_errptr) = NULL;
>> +     void *in_payload __free(kfree_errptr) = NULL;
> 
> __free(kfree) should work from kzalloc(). No need to define special macro.

Sure

> 
>> +     struct device *dev = &uctx->fwctl->dev;
>> +     union pds_core_adminq_comp comp = {0};
>> +     dma_addr_t out_payload_dma_addr = 0;
>> +     union pds_core_adminq_cmd cmd = {0};
>> +     dma_addr_t in_payload_dma_addr = 0;
>> +     void *out = NULL;
>> +     int err;
>> +
>> +     err = pdsfc_validate_rpc(pdsfc, rpc, scope);
>> +     if (err) {
>> +             dev_err(dev, "Invalid RPC request\n");
>> +             return ERR_PTR(err);
>> +     }
>> +
>> +     if (rpc->in.len > 0) {
>> +             in_payload = kzalloc(rpc->in.len, GFP_KERNEL);
>> +             if (!in_payload) {
>> +                     dev_err(dev, "Failed to allocate in_payload\n");
>> +                     out = ERR_PTR(-ENOMEM);
>> +                     goto done;
>> +             }
>> +
>> +             if (copy_from_user(in_payload, u64_to_user_ptr(rpc->in.payload),
>> +                                rpc->in.len)) {
>> +                     dev_err(dev, "Failed to copy in_payload from user\n");
>> +                     out = ERR_PTR(-EFAULT);
>> +                     goto done;
>> +             }
> 
> So the cleanup macros and gotos here make things a bit messy. But maybe you can define a transient struct:
> struct payload {
>          void *data;
>          dma_addr_t dma;
>          size_t len;
>          struct device *dev;
>          enum dma_data_direction dir;
> };
> 
> static inline void cleanup_payload(struct payload *payload)
> {
>          dma_unmap_single(payload->dev, payload->dma, payload->len, payload->dir);
>          kfree(payload->data);
>          kfree(payload);
> }
> DEFINE_FREE(free_payload, struct payload *, if (_T) cleanup_payload(_T));
> 
> static struct payload *alloc_payload(struct device *dev, size_t len, enum dma_data_direction dir)
> {
>          dma_addr_t dma;
>          int err;
> 
>          struct payload *p __free(kfree) = kzalloc(sizeof(*p), GFP_KERNEL);
>          if (!p)
>                  return NULL;
> 
>          void *data __free(kfree) = kzalloc(len, GFP_KERNEL);
>          if (!data)
>                  return NULL;
> 
>          dma = dma_map_single(dev, data, len, dir);
>          err = dma_mapping_error(dev, dma);
>          if (err)
>                  return NULL;
> 
>          p->dma = dma;
>          p->len = len;
>          p->dir = dir;
>          p->data = no_free_ptr(data);
> 
>          return no_free_ptr(p);
> }
> 
> With that you can use __free() cleanly and then when your rpc function exits, it'll unmap and free everything automatically.
> 
> struct payload *p __free(free_payload) = alloc_payload(...);
> 
> Just a thought....

Thanks, I'll look at that.


> 
>> +
>> +             in_payload_dma_addr = dma_map_single(dev->parent, in_payload,
>> +                                                  rpc->in.len, DMA_TO_DEVICE);
>> +             err = dma_mapping_error(dev->parent, in_payload_dma_addr);
>> +             if (err) {
>> +                     dev_err(dev, "Failed to map in_payload\n");
>> +                     out = ERR_PTR(err);
>> +                     goto done;
>> +             }
>> +     }
>> +
>> +     if (rpc->out.len > 0) {
>> +             out_payload = kzalloc(rpc->out.len, GFP_KERNEL);
>> +             if (!out_payload) {
>> +                     dev_err(dev, "Failed to allocate out_payload\n");
>> +                     out = ERR_PTR(-ENOMEM);
>> +                     goto done;
>> +             }
>> +
>> +             out_payload_dma_addr = dma_map_single(dev->parent, out_payload,
>> +                                                   rpc->out.len, DMA_FROM_DEVICE);
>> +             err = dma_mapping_error(dev->parent, out_payload_dma_addr);
>> +             if (err) {
>> +                     dev_err(dev, "Failed to map out_payload\n");
>> +                     out = ERR_PTR(err);
>> +                     goto done;
>> +             }
>> +     }
>> +
>> +     cmd.fwctl_rpc.opcode = PDS_FWCTL_CMD_RPC;
>> +     cmd.fwctl_rpc.flags = PDS_FWCTL_RPC_IND_REQ | PDS_FWCTL_RPC_IND_RESP;
>> +     cmd.fwctl_rpc.ep = cpu_to_le32(rpc->in.ep);
>> +     cmd.fwctl_rpc.op = cpu_to_le32(rpc->in.op);
>> +     cmd.fwctl_rpc.req_pa = cpu_to_le64(in_payload_dma_addr);
>> +     cmd.fwctl_rpc.req_sz = cpu_to_le32(rpc->in.len);
>> +     cmd.fwctl_rpc.resp_pa = cpu_to_le64(out_payload_dma_addr);
>> +     cmd.fwctl_rpc.resp_sz = cpu_to_le32(rpc->out.len);
>> +
>> +     dev_dbg(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d\n",
>> +             __func__, rpc->in.ep, rpc->in.op,
>> +             cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
>> +             cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems);
>> +
>> +     dynamic_hex_dump("in ", DUMP_PREFIX_OFFSET, 16, 1, in_payload, rpc->in.len, true);
>> +
>> +     err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0);
>> +     if (err) {
>> +             dev_err(dev, "%s: ep %d op %x req_pa %llx req_sz %d req_sg %d resp_pa %llx resp_sz %d resp_sg %d err %d\n",
>> +                     __func__, rpc->in.ep, rpc->in.op,
>> +                     cmd.fwctl_rpc.req_pa, cmd.fwctl_rpc.req_sz, cmd.fwctl_rpc.req_sg_elems,
>> +                     cmd.fwctl_rpc.resp_pa, cmd.fwctl_rpc.resp_sz, cmd.fwctl_rpc.resp_sg_elems,
>> +                     err);
>> +             out = ERR_PTR(err);
>> +             goto done;
>> +     }
>> +
>> +     dynamic_hex_dump("out ", DUMP_PREFIX_OFFSET, 16, 1, out_payload, rpc->out.len, true);
>> +
>> +     dev_dbg(dev, "%s: status %d comp_index %d err %d resp_sz %d color %d\n",
>> +             __func__, comp.fwctl_rpc.status, comp.fwctl_rpc.comp_index,
>> +             comp.fwctl_rpc.err, comp.fwctl_rpc.resp_sz,
>> +             comp.fwctl_rpc.color);
>> +
>> +     if (copy_to_user(u64_to_user_ptr(rpc->out.payload), out_payload, rpc->out.len)) {
>> +             dev_err(dev, "Failed to copy out_payload to user\n");
>> +             out = ERR_PTR(-EFAULT);
>> +             goto done;
>> +     }
>> +
>> +     rpc->out.retval = le32_to_cpu(comp.fwctl_rpc.err);
>> +     *out_len = in_len;
>> +     out = in;
>> +
>> +done:
>> +     if (in_payload_dma_addr)
>> +             dma_unmap_single(dev->parent, in_payload_dma_addr,
>> +                              rpc->in.len, DMA_TO_DEVICE);
>> +
>> +     if (out_payload_dma_addr)
>> +             dma_unmap_single(dev->parent, out_payload_dma_addr,
>> +                              rpc->out.len, DMA_FROM_DEVICE);
>> +
>> +     return out;
>>   }
>>
>>   static const struct fwctl_ops pdsfc_ops = {
>> @@ -150,16 +504,23 @@ static int pdsfc_probe(struct auxiliary_device *adev,
>>                return err;
>>        }
>>
>> +     err = pdsfc_init_endpoints(pdsfc);
>> +     if (err) {
>> +             dev_err(dev, "Failed to init endpoints, err %d\n", err);
>> +             goto free_ident;
>> +     }
>> +
>>        err = fwctl_register(&pdsfc->fwctl);
>>        if (err) {
>>                dev_err(dev, "Failed to register device, err %d\n", err);
>> -             return err;
>> +             goto free_endpoints;
>>        }
>> -
>>        auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
>>
>>        return 0;
>>
>> +free_endpoints:
>> +     pdsfc_free_endpoints(pdsfc);
>>   free_ident:
>>        pdsfc_free_ident(pdsfc);
>>        return err;
>> @@ -170,6 +531,8 @@ static void pdsfc_remove(struct auxiliary_device *adev)
>>        struct pdsfc_dev *pdsfc  __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>>
>>        fwctl_unregister(&pdsfc->fwctl);
>> +     pdsfc_free_operations(pdsfc);
>> +     pdsfc_free_endpoints(pdsfc);
>>        pdsfc_free_ident(pdsfc);
>>   }
>>
>> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
>> index 7fc353b63353..33cd03388b15 100644
>> --- a/include/linux/pds/pds_adminq.h
>> +++ b/include/linux/pds/pds_adminq.h
>> @@ -1181,6 +1181,8 @@ struct pds_lm_host_vf_status_cmd {
>>
>>   enum pds_fwctl_cmd_opcode {
>>        PDS_FWCTL_CMD_IDENT             = 70,
>> +     PDS_FWCTL_CMD_RPC               = 71,
>> +     PDS_FWCTL_CMD_QUERY             = 72,
>>   };
>>
>>   /**
>> @@ -1251,6 +1253,187 @@ struct pds_fwctl_ident {
>>        u8     max_resp_sg_elems;
>>   } __packed;
>>
>> +enum pds_fwctl_query_entity {
>> +     PDS_FWCTL_RPC_ROOT      = 0,
>> +     PDS_FWCTL_RPC_ENDPOINT  = 1,
>> +     PDS_FWCTL_RPC_OPERATION = 2,
>> +};
>> +
>> +#define PDS_FWCTL_RPC_OPCODE_CMD_SHIFT       0
>> +#define PDS_FWCTL_RPC_OPCODE_CMD_MASK        GENMASK(15, PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)
>> +#define PDS_FWCTL_RPC_OPCODE_VER_SHIFT       16
>> +#define PDS_FWCTL_RPC_OPCODE_VER_MASK        GENMASK(23, PDS_FWCTL_RPC_OPCODE_VER_SHIFT)
>> +
>> +#define PDS_FWCTL_RPC_OPCODE_GET_CMD(op)     \
>> +     (((op) & PDS_FWCTL_RPC_OPCODE_CMD_MASK) >> PDS_FWCTL_RPC_OPCODE_CMD_SHIFT)
>> +#define PDS_FWCTL_RPC_OPCODE_GET_VER(op)     \
>> +     (((op) & PDS_FWCTL_RPC_OPCODE_VER_MASK) >> PDS_FWCTL_RPC_OPCODE_VER_SHIFT)
>> +
>> +#define PDS_FWCTL_RPC_OPCODE_CMP(op1, op2) \
>> +     (PDS_FWCTL_RPC_OPCODE_GET_CMD(op1) == PDS_FWCTL_RPC_OPCODE_GET_CMD(op2) && \
>> +      PDS_FWCTL_RPC_OPCODE_GET_VER(op1) <= PDS_FWCTL_RPC_OPCODE_GET_VER(op2))
>> +
>> +/**
>> + * struct pds_fwctl_query_cmd - Firmware control query command structure
>> + * @opcode: Operation code for the command
>> + * @entity:  Entity type to query (enum pds_fwctl_query_entity)
>> + * @version: Version of the query data structure supported by the driver
>> + * @rsvd:    Word boundary padding
>> + * @query_data_buf_len: Length of the query data buffer
>> + * @query_data_buf_pa:  Physical address of the query data buffer
>> + * @ep:      Endpoint identifier to query  (when entity is PDS_FWCTL_RPC_ENDPOINT)
>> + * @op:      Operation identifier to query (when entity is PDS_FWCTL_RPC_OPERATION)
>> + *
>> + * This structure is used to send a query command to the firmware control
>> + * interface. The structure is packed to ensure there is no padding between
>> + * the fields.
>> + */
>> +struct pds_fwctl_query_cmd {
>> +     u8     opcode;
>> +     u8     entity;
>> +     u8     version;
>> +     u8     rsvd;
>> +     __le32 query_data_buf_len;
>> +     __le64 query_data_buf_pa;
>> +     union {
>> +             __le32 ep;
>> +             __le32 op;
>> +     };
>> +} __packed;
>> +
>> +/**
>> + * struct pds_fwctl_query_comp - Firmware control query completion structure
>> + * @status:     Status of the query command
>> + * @rsvd:       Word boundary padding
>> + * @comp_index: Completion index in little-endian format
>> + * @version:    Version of the query data structure returned by firmware. This
>> + *            should be less than or equal to the version supported by the driver.
>> + * @rsvd2:      Word boundary padding
>> + * @color:      Color bit indicating the state of the completion
>> + */
>> +struct pds_fwctl_query_comp {
>> +     u8     status;
>> +     u8     rsvd;
>> +     __le16 comp_index;
>> +     u8     version;
>> +     u8     rsvd2[2];
>> +     u8     color;
>> +} __packed;
>> +
>> +/**
>> + * struct pds_fwctl_query_data_endpoint - query data for entity PDS_FWCTL_RPC_ROOT
>> + * @id: The identifier for the data endpoint.
>> + */
>> +struct pds_fwctl_query_data_endpoint {
>> +     __le32 id;
>> +} __packed;
>> +
>> +/**
>> + * struct pds_fwctl_query_data_operation - query data for entity PDS_FWCTL_RPC_ENDPOINT
>> + * @id:    Operation identifier.
>> + * @scope: Scope of the operation (enum fwctl_rpc_scope).
>> + * @rsvd:  Word boundary padding
>> + */
>> +struct pds_fwctl_query_data_operation {
>> +     __le32 id;
>> +     u8     scope;
>> +     u8     rsvd[3];
>> +} __packed;
>> +
>> +/**
>> + * struct pds_fwctl_query_data - query data structure
>> + * @version:     Version of the query data structure
>> + * @rsvd:        Word boundary padding
>> + * @num_entries: Number of entries in the union
>> + * @entries:     Array of query data entries, depending on the entity type.
>> + */
>> +struct pds_fwctl_query_data {
>> +     u8      version;
>> +     u8      rsvd[3];
>> +     __le32  num_entries;
>> +     uint8_t entries[];
> Why u8 above but uint8_t here?
> 
> u8 entries[] __counted_by_le(num_entries);

Jonathan caught that as well - thanks.
sln

> 
> DJ
> 
>> +} __packed;
>> +
>> +/**
>> + * struct pds_fwctl_rpc_cmd - Firmware control RPC command.
>> + * @opcode:        opcode PDS_FWCTL_CMD_RPC
>> + * @rsvd:          Word boundary padding
>> + * @flags:         Indicates indirect request and/or response handling
>> + * @ep:            Endpoint identifier.
>> + * @op:            Operation identifier.
>> + * @inline_req0:   Buffer for inline request
>> + * @inline_req1:   Buffer for inline request
>> + * @req_pa:        Physical address of request data.
>> + * @req_sz:        Size of the request.
>> + * @req_sg_elems:  Number of request SGs
>> + * @req_rsvd:      Word boundary padding
>> + * @inline_req2:   Buffer for inline request
>> + * @resp_pa:       Physical address of response data.
>> + * @resp_sz:       Size of the response.
>> + * @resp_sg_elems: Number of response SGs
>> + * @resp_rsvd:     Word boundary padding
>> + */
>> +struct pds_fwctl_rpc_cmd {
>> +     u8     opcode;
>> +     u8     rsvd;
>> +     __le16 flags;
>> +#define PDS_FWCTL_RPC_IND_REQ                0x1
>> +#define PDS_FWCTL_RPC_IND_RESP               0x2
>> +     __le32 ep;
>> +     __le32 op;
>> +     u8 inline_req0[16];
>> +     union {
>> +             u8 inline_req1[16];
>> +             struct {
>> +                     __le64 req_pa;
>> +                     __le32 req_sz;
>> +                     u8     req_sg_elems;
>> +                     u8     req_rsvd[3];
>> +             };
>> +     };
>> +     union {
>> +             u8 inline_req2[16];
>> +             struct {
>> +                     __le64 resp_pa;
>> +                     __le32 resp_sz;
>> +                     u8     resp_sg_elems;
>> +                     u8     resp_rsvd[3];
>> +             };
>> +     };
>> +} __packed;
>> +
>> +/**
>> + * struct pds_sg_elem - Transmit scatter-gather (SG) descriptor element
>> + * @addr:    DMA address of SG element data buffer
>> + * @len:     Length of SG element data buffer, in bytes
>> + * @rsvd:    Word boundary padding
>> + */
>> +struct pds_sg_elem {
>> +     __le64 addr;
>> +     __le32 len;
>> +     __le16 rsvd[2];
>> +} __packed;
>> +
>> +/**
>> + * struct pds_fwctl_rpc_comp - Completion of a firmware control RPC.
>> + * @status:     Status of the command
>> + * @rsvd:       Word boundary padding
>> + * @comp_index: Completion index of the command
>> + * @err:        Error code, if any, from the RPC.
>> + * @resp_sz:    Size of the response.
>> + * @rsvd2:      Word boundary padding
>> + * @color:      Color bit indicating the state of the completion.
>> + */
>> +struct pds_fwctl_rpc_comp {
>> +     u8     status;
>> +     u8     rsvd;
>> +     __le16 comp_index;
>> +     __le32 err;
>> +     __le32 resp_sz;
>> +     u8     rsvd2[3];
>> +     u8     color;
>> +} __packed;
>> +
>>   union pds_core_adminq_cmd {
>>        u8     opcode;
>>        u8     bytes[64];
>> @@ -1291,6 +1474,8 @@ union pds_core_adminq_cmd {
>>
>>        struct pds_fwctl_cmd              fwctl;
>>        struct pds_fwctl_ident_cmd        fwctl_ident;
>> +     struct pds_fwctl_rpc_cmd          fwctl_rpc;
>> +     struct pds_fwctl_query_cmd        fwctl_query;
>>   };
>>
>>   union pds_core_adminq_comp {
>> @@ -1320,6 +1505,8 @@ union pds_core_adminq_comp {
>>        struct pds_lm_dirty_status_comp   lm_dirty_status;
>>
>>        struct pds_fwctl_comp             fwctl;
>> +     struct pds_fwctl_rpc_comp         fwctl_rpc;
>> +     struct pds_fwctl_query_comp       fwctl_query;
>>   };
>>
>>   #ifndef __CHECKER__
>> diff --git a/include/uapi/fwctl/pds.h b/include/uapi/fwctl/pds.h
>> index a01b032cbdb1..da6cd2d1c6fa 100644
>> --- a/include/uapi/fwctl/pds.h
>> +++ b/include/uapi/fwctl/pds.h
>> @@ -24,4 +24,20 @@ enum pds_fwctl_capabilities {
>>        PDS_FWCTL_QUERY_CAP = 0,
>>        PDS_FWCTL_SEND_CAP,
>>   };
>> +
>> +struct fwctl_rpc_pds {
>> +     struct {
>> +             __u32 op;
>> +             __u32 ep;
>> +             __u32 rsvd;
>> +             __u32 len;
>> +             __u64 payload;
>> +     } in;
>> +     struct {
>> +             __u32 retval;
>> +             __u32 rsvd[2];
>> +             __u32 len;
>> +             __u64 payload;
>> +     } out;
>> +};
>>   #endif /* _UAPI_FWCTL_PDS_H_ */
> 

[RFC PATCH fwctl 5/5] pds_fwctl: add Documentation entries

[Shannon Nelson]

Add pds_fwctl to the driver and fwctl documentation pages.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 Documentation/userspace-api/fwctl/fwctl.rst   |  1 +
 Documentation/userspace-api/fwctl/index.rst   |  1 +
 .../userspace-api/fwctl/pds_fwctl.rst         | 41 +++++++++++++++++++
 3 files changed, 43 insertions(+)
 create mode 100644 Documentation/userspace-api/fwctl/pds_fwctl.rst

diff --git a/Documentation/userspace-api/fwctl/fwctl.rst b/Documentation/userspace-api/fwctl/fwctl.rst
index 428f6f5bb9b4..72853b0d3dc8 100644
--- a/Documentation/userspace-api/fwctl/fwctl.rst
+++ b/Documentation/userspace-api/fwctl/fwctl.rst
@@ -150,6 +150,7 @@ fwctl User API
 
 .. kernel-doc:: include/uapi/fwctl/fwctl.h
 .. kernel-doc:: include/uapi/fwctl/mlx5.h
+.. kernel-doc:: include/uapi/fwctl/pds.h
 
 sysfs Class
 -----------
diff --git a/Documentation/userspace-api/fwctl/index.rst b/Documentation/userspace-api/fwctl/index.rst
index 06959fbf1547..12a559fcf1b2 100644
--- a/Documentation/userspace-api/fwctl/index.rst
+++ b/Documentation/userspace-api/fwctl/index.rst
@@ -10,3 +10,4 @@ to securely construct and execute RPCs inside device firmware.
    :maxdepth: 1
 
    fwctl
+   pds_fwctl
diff --git a/Documentation/userspace-api/fwctl/pds_fwctl.rst b/Documentation/userspace-api/fwctl/pds_fwctl.rst
new file mode 100644
index 000000000000..9fb1b4ac0a5e
--- /dev/null
+++ b/Documentation/userspace-api/fwctl/pds_fwctl.rst
@@ -0,0 +1,41 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+================
+fwctl pds driver
+================
+
+:Author: Shannon Nelson
+
+Overview
+========
+
+The PDS Core device makes an fwctl service available through an
+auxiliary_device named pds_core.fwctl.N.  The pds_fwctl driver binds
+to this device and registers itself with the fwctl bus.  The resulting
+userspace interface is used by an application that is a part of the
+AMD/Pensando software package for the Distributed Service Card (DSC).
+
+The pds_fwctl driver has little knowledge of the firmware's internals,
+only knows how to send adminq commands for fwctl requests.  The set of
+operations available through this interface depends on the firmware in
+the DSC, and the userspace application version must match the firmware
+so that they can talk to each other.
+
+This set of available operations is not known to the pds_fwctl driver.
+When a connection is created the pds_fwctl driver requests from the
+firmware list of endpoints and a list of operations for each endpoint.
+This list of operations includes a minumum scope level that the pds_fwctl
+driver can use for filtering privilege levels.
+
+pds_fwctl User API
+==================
+
+.. kernel-doc:: include/uapi/fwctl/pds.h
+
+Each RPC request includes the target endpoint and the operation id, and in
+and out buffer lengths and pointers.  The driver verifies the existence
+of the requested endpoint and operations, then checks the current scope
+against the required scope of the operation.  The adminq request is then
+put together with the request data and sent to the firmware, and the
+results are returned to the caller.
+
-- 
2.17.1

Re: [RFC PATCH fwctl 5/5] pds_fwctl: add Documentation entries

[Jonathan Cameron]

On Tue, 11 Feb 2025 15:48:54 -0800
Shannon Nelson <shannon.nelson@amd.com> wrote:

> Add pds_fwctl to the driver and fwctl documentation pages.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  Documentation/userspace-api/fwctl/fwctl.rst   |  1 +
>  Documentation/userspace-api/fwctl/index.rst   |  1 +
>  .../userspace-api/fwctl/pds_fwctl.rst         | 41 +++++++++++++++++++
>  3 files changed, 43 insertions(+)
>  create mode 100644 Documentation/userspace-api/fwctl/pds_fwctl.rst
> 
> diff --git a/Documentation/userspace-api/fwctl/fwctl.rst b/Documentation/userspace-api/fwctl/fwctl.rst
> index 428f6f5bb9b4..72853b0d3dc8 100644
> --- a/Documentation/userspace-api/fwctl/fwctl.rst
> +++ b/Documentation/userspace-api/fwctl/fwctl.rst
> @@ -150,6 +150,7 @@ fwctl User API
>  
>  .. kernel-doc:: include/uapi/fwctl/fwctl.h
>  .. kernel-doc:: include/uapi/fwctl/mlx5.h
> +.. kernel-doc:: include/uapi/fwctl/pds.h
>  
>  sysfs Class
>  -----------
> diff --git a/Documentation/userspace-api/fwctl/index.rst b/Documentation/userspace-api/fwctl/index.rst
> index 06959fbf1547..12a559fcf1b2 100644
> --- a/Documentation/userspace-api/fwctl/index.rst
> +++ b/Documentation/userspace-api/fwctl/index.rst
> @@ -10,3 +10,4 @@ to securely construct and execute RPCs inside device firmware.
>     :maxdepth: 1
>  
>     fwctl
> +   pds_fwctl
> diff --git a/Documentation/userspace-api/fwctl/pds_fwctl.rst b/Documentation/userspace-api/fwctl/pds_fwctl.rst
> new file mode 100644
> index 000000000000..9fb1b4ac0a5e
> --- /dev/null
> +++ b/Documentation/userspace-api/fwctl/pds_fwctl.rst
> @@ -0,0 +1,41 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +================
> +fwctl pds driver
> +================
> +
> +:Author: Shannon Nelson
> +
> +Overview
> +========
> +
> +The PDS Core device makes an fwctl service available through an
> +auxiliary_device named pds_core.fwctl.N.  The pds_fwctl driver binds
> +to this device and registers itself with the fwctl bus.  The resulting
> +userspace interface is used by an application that is a part of the
> +AMD/Pensando software package for the Distributed Service Card (DSC).

Jason, where did we get to on the question of a central open repo etc?

> +
> +The pds_fwctl driver has little knowledge of the firmware's internals,
> +only knows how to send adminq commands for fwctl requests.  The set of
> +operations available through this interface depends on the firmware in
> +the DSC, and the userspace application version must match the firmware
> +so that they can talk to each other.
> +
> +This set of available operations is not known to the pds_fwctl driver.
> +When a connection is created the pds_fwctl driver requests from the
> +firmware list of endpoints and a list of operations for each endpoint.
> +This list of operations includes a minumum scope level that the pds_fwctl
> +driver can use for filtering privilege levels.

Ah. Ok. So the scope is provided in the replies to these queries.
Do we have anything to verify that today?
Also, I wasn't sure when reading driver on whether the operations list
is dynamic or something we can read once and cache?

> +
> +pds_fwctl User API
> +==================
> +
> +.. kernel-doc:: include/uapi/fwctl/pds.h
> +
> +Each RPC request includes the target endpoint and the operation id, and in
> +and out buffer lengths and pointers.  The driver verifies the existence
> +of the requested endpoint and operations, then checks the current scope
> +against the required scope of the operation.  The adminq request is then

Spell out admin q (or is that a typo?)

> +put together with the request data and sent to the firmware, and the
> +results are returned to the caller.
> +

Re: [RFC PATCH fwctl 5/5] pds_fwctl: add Documentation entries

[Jason Gunthorpe]

On Wed, Feb 12, 2025 at 12:51:49PM +0000, Jonathan Cameron wrote:
> > +The PDS Core device makes an fwctl service available through an
> > +auxiliary_device named pds_core.fwctl.N.  The pds_fwctl driver binds
> > +to this device and registers itself with the fwctl bus.  The resulting
> > +userspace interface is used by an application that is a part of the
> > +AMD/Pensando software package for the Distributed Service Card (DSC).
> 
> Jason, where did we get to on the question of a central open repo etc?

I it is something I would like to do and support, but I haven't seen
any proposal what it would look like just yet. Right now people seem
to be porting their existing tools rather than building new stuff from
scratch.

I have some thoughts around provisioning but that's all.

Jason

Re: [RFC PATCH fwctl 5/5] pds_fwctl: add Documentation entries

[Leon Romanovsky]

On Wed, Feb 12, 2025 at 12:51:49PM +0000, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 15:48:54 -0800
> Shannon Nelson <shannon.nelson@amd.com> wrote:
> 
> > Add pds_fwctl to the driver and fwctl documentation pages.
> > 
> > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> > ---
> >  Documentation/userspace-api/fwctl/fwctl.rst   |  1 +
> >  Documentation/userspace-api/fwctl/index.rst   |  1 +
> >  .../userspace-api/fwctl/pds_fwctl.rst         | 41 +++++++++++++++++++
> >  3 files changed, 43 insertions(+)
> >  create mode 100644 Documentation/userspace-api/fwctl/pds_fwctl.rst

<...>

> > +========
> > +
> > +The PDS Core device makes an fwctl service available through an
> > +auxiliary_device named pds_core.fwctl.N.  The pds_fwctl driver binds
> > +to this device and registers itself with the fwctl bus.  The resulting
> > +userspace interface is used by an application that is a part of the
> > +AMD/Pensando software package for the Distributed Service Card (DSC).
> 
> Jason, where did we get to on the question of a central open repo etc?

I created organization for it https://github.com/linux-fwctl/ and will
transfer to anyone who will work on it.

Thanks

Re: [RFC PATCH fwctl 5/5] pds_fwctl: add Documentation entries

[Nelson, Shannon]

On 2/12/2025 4:51 AM, Jonathan Cameron wrote:
> 
> On Tue, 11 Feb 2025 15:48:54 -0800
> Shannon Nelson <shannon.nelson@amd.com> wrote:
> 
>> Add pds_fwctl to the driver and fwctl documentation pages.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>>   Documentation/userspace-api/fwctl/fwctl.rst   |  1 +
>>   Documentation/userspace-api/fwctl/index.rst   |  1 +
>>   .../userspace-api/fwctl/pds_fwctl.rst         | 41 +++++++++++++++++++
>>   3 files changed, 43 insertions(+)
>>   create mode 100644 Documentation/userspace-api/fwctl/pds_fwctl.rst
>>
>> diff --git a/Documentation/userspace-api/fwctl/fwctl.rst b/Documentation/userspace-api/fwctl/fwctl.rst
>> index 428f6f5bb9b4..72853b0d3dc8 100644
>> --- a/Documentation/userspace-api/fwctl/fwctl.rst
>> +++ b/Documentation/userspace-api/fwctl/fwctl.rst
>> @@ -150,6 +150,7 @@ fwctl User API
>>
>>   .. kernel-doc:: include/uapi/fwctl/fwctl.h
>>   .. kernel-doc:: include/uapi/fwctl/mlx5.h
>> +.. kernel-doc:: include/uapi/fwctl/pds.h
>>
>>   sysfs Class
>>   -----------
>> diff --git a/Documentation/userspace-api/fwctl/index.rst b/Documentation/userspace-api/fwctl/index.rst
>> index 06959fbf1547..12a559fcf1b2 100644
>> --- a/Documentation/userspace-api/fwctl/index.rst
>> +++ b/Documentation/userspace-api/fwctl/index.rst
>> @@ -10,3 +10,4 @@ to securely construct and execute RPCs inside device firmware.
>>      :maxdepth: 1
>>
>>      fwctl
>> +   pds_fwctl
>> diff --git a/Documentation/userspace-api/fwctl/pds_fwctl.rst b/Documentation/userspace-api/fwctl/pds_fwctl.rst
>> new file mode 100644
>> index 000000000000..9fb1b4ac0a5e
>> --- /dev/null
>> +++ b/Documentation/userspace-api/fwctl/pds_fwctl.rst
>> @@ -0,0 +1,41 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +================
>> +fwctl pds driver
>> +================
>> +
>> +:Author: Shannon Nelson
>> +
>> +Overview
>> +========
>> +
>> +The PDS Core device makes an fwctl service available through an
>> +auxiliary_device named pds_core.fwctl.N.  The pds_fwctl driver binds
>> +to this device and registers itself with the fwctl bus.  The resulting
>> +userspace interface is used by an application that is a part of the
>> +AMD/Pensando software package for the Distributed Service Card (DSC).
> 
> Jason, where did we get to on the question of a central open repo etc?
> 
>> +
>> +The pds_fwctl driver has little knowledge of the firmware's internals,
>> +only knows how to send adminq commands for fwctl requests.  The set of
>> +operations available through this interface depends on the firmware in
>> +the DSC, and the userspace application version must match the firmware
>> +so that they can talk to each other.
>> +
>> +This set of available operations is not known to the pds_fwctl driver.
>> +When a connection is created the pds_fwctl driver requests from the
>> +firmware list of endpoints and a list of operations for each endpoint.
>> +This list of operations includes a minumum scope level that the pds_fwctl
>> +driver can use for filtering privilege levels.
> 
> Ah. Ok. So the scope is provided in the replies to these queries.
> Do we have anything to verify that today?
> Also, I wasn't sure when reading driver on whether the operations list
> is dynamic or something we can read once and cache?

We request the operations list once on the first query of each endpoint 
and cache them for the next time.

> 
>> +
>> +pds_fwctl User API
>> +==================
>> +
>> +.. kernel-doc:: include/uapi/fwctl/pds.h
>> +
>> +Each RPC request includes the target endpoint and the operation id, and in
>> +and out buffer lengths and pointers.  The driver verifies the existence
>> +of the requested endpoint and operations, then checks the current scope
>> +against the required scope of the operation.  The adminq request is then
> 
> Spell out admin q (or is that a typo?)

The "adminq" is the particular object name, so I'll keep that, but I can 
certainly add a little more text around it.

Thanks for your review comments, we appreciate it!

Cheers,
sln


> 
>> +put together with the request data and sent to the firmware, and the
>> +results are returned to the caller.
>> +
> 

Re: [RFC PATCH fwctl 0/5] pds_fwctl: fwctl for AMD/Pensando core devices

[Andrew Lunn]

> existing function-specific tools.  For example, these are things that make
> the Eth PCI device appear on the PCI bus

That sounds like a common operation which many vendors will need? So
why use fwctl for this? The whole point of fwctl is things which are
highly vendor specific and not networking.

Isn't this even generic for any sort of SR-IOV? Wouldn't you need the
same sort of operation for a GPU, or anything with a pool of resources
which can be mapped to VFs?

If you really want to use this as you key selling point, you need to
clearly explain why is this highly vendor specific and cannot be
generalised.

	Andrew

Re: [RFC PATCH fwctl 0/5] pds_fwctl: fwctl for AMD/Pensando core devices

[Jason Gunthorpe]

On Wed, Feb 12, 2025 at 02:40:45PM +0100, Andrew Lunn wrote:

> Isn't this even generic for any sort of SR-IOV? Wouldn't you need the
> same sort of operation for a GPU, or anything with a pool of resources
> which can be mapped to VFs?

We've been calling this device profiling in the vfio discussions,
generally yes the general idea of profiling is common, but the actual
detail of the profile is very device specific.

In vfio land we think fwctl is a good choice here. We already have
things like libvirt and Kubernetes that have generic userspace plugin
mechanims and an existing mature ecosystem for device profiling built
up around that. All sophisticated devices have their own plugins
because they have unique capabilities. It seems to be working well in
that world.

From a kernel perspective fwctl is alot better than some of what has
been tried so far, ie various vfio drivers having questionable
device-specific sysfs, and then a libvirt/k8s plugin anyhow.

Even the better stuff like mlx5's devlink is only partially capable
and the existing mlx plugins still has to do stuff beyond that.

The kernel isn't the only point, or necessarily the most appropriate
point, to insert a consolidation layer in the stack. We don't want to
move chunks of existing k8s operator code into the kernel, for
instance.

Again, this isn't an exclusive thing, that fwctl can profile a PCI
function doesn't in any way exclude other kernel options, like
devlink, from doing that too.

Jason

Re: [RFC PATCH fwctl 0/5] pds_fwctl: fwctl for AMD/Pensando core devices

[Andrew Lunn]

On Wed, Feb 12, 2025 at 10:43:28AM -0400, Jason Gunthorpe wrote:
> On Wed, Feb 12, 2025 at 02:40:45PM +0100, Andrew Lunn wrote:
> 
> > Isn't this even generic for any sort of SR-IOV? Wouldn't you need the
> > same sort of operation for a GPU, or anything with a pool of resources
> > which can be mapped to VFs?
> 
> We've been calling this device profiling in the vfio discussions,
> generally yes the general idea of profiling is common, but the actual
> detail of the profile is very device specific.

This is your poster child for fwctl. You are trying to convince us it
is a way to configure things which are very vendor specific. Yet, as
you point out, the idea of profiling is common. So why start here? It
seems an odd choice. So i would of expected the messaging to be
clearer. You the vendors agree there is no commonality, so explain
that. Take three different vendors cards and list all the parameters
which are needed for profiling with these cards. Really show that
there is no commonality. And maybe take it a step further. Get these
vendors to work together to produce three patchset implementing device
profiling, so we can see there cannot be code sharing. Then you might
have a convincing poster child for fwctl.

Given how contentious fwctl is, i would say vendors need to work
together to show there is nothing in common, at least to start
with.

	Andrew