[PATCH v2 0/6] pds_fwctl: fwctl for AMD/Pensando core devices

[PATCH v2 0/6] pds_fwctl: fwctl for AMD/Pensando core devices

[Shannon Nelson]

Following along from Jason's work [1] we have our initial 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 are a bit of clean up for pds_core's add and delete
of auxiliary_devices.  Those are followed by a patch to add the new
pds_core.fwctl auxiliary_device.  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-v5-642aa0c94070+4447f-fwctl_jgg@nvidia.com/

v2:
  - removed the RFC tag
  - add a patch to make pdsc_auxbus_dev_del() a void type (Jonathan)
  - fix up error handling if pdsc_auxbus_dev_add() fails in probe (Jonathan)
  - fix auxiliary spelling in commit subject header (Jonathan)
  - clean up of code around use of __free() gizmo (Jonathan, David)
  - removed extra whitespace and dev_xxx() calls (Leon)
  - copy ident info from DMA and release DMA memory in probe (Jonathan)
  - use dev_err_probe() (Jonathan)
  - add counted_by_le(num_entries) (Jonathan, David)
  - convert num_entries from __le32 to host in get_endpoints() (Jonathan)
  - remove unnecessary variable inits (Jonathan, Leon)

v1: https://lore.kernel.org/netdev/20250211234854.52277-1-shannon.nelson@amd.com/

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

Shannon Nelson (5):
  pds_core: make pdsc_auxbus_dev_del() void
  pds_core: specify auxiliary_device to be created
  pds_core: add new fwctl auxiliary_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                      | 506 ++++++++++++++++++
 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   |   7 +-
 drivers/net/ethernet/amd/pds_core/main.c      |  22 +-
 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, 936 insertions(+), 33 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

[PATCH v2 1/6] pds_core: make pdsc_auxbus_dev_del() void

[Shannon Nelson]

Since there really is no useful return, advertising a return value
is rather misleading.  Make pdsc_auxbus_dev_del() a void function.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/auxbus.c  | 8 ++------
 drivers/net/ethernet/amd/pds_core/core.h    | 2 +-
 drivers/net/ethernet/amd/pds_core/devlink.c | 6 ++++--
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
index 2babea110991..b1fab95f7eeb 100644
--- a/drivers/net/ethernet/amd/pds_core/auxbus.c
+++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
@@ -175,13 +175,9 @@ 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)
+void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
 {
 	struct pds_auxiliary_dev *padev;
-	int err = 0;
-
-	if (!cf)
-		return -ENODEV;
 
 	mutex_lock(&pf->config_lock);
 
@@ -195,7 +191,6 @@ int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
 	pf->vfs[cf->vf_id].padev = NULL;
 
 	mutex_unlock(&pf->config_lock);
-	return err;
 }
 
 int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf)
diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
index 14522d6d5f86..631a59cfdd7e 100644
--- a/drivers/net/ethernet/amd/pds_core/core.h
+++ b/drivers/net/ethernet/amd/pds_core/core.h
@@ -304,7 +304,7 @@ 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);
+void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf);
 
 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..4e2b92ddef6f 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);
+		if (ctx->val.vbool)
+			err = pdsc_auxbus_dev_add(vf, pdsc);
+		else
+			pdsc_auxbus_dev_del(vf, pdsc);
 	}
 
 	return err;
-- 
2.17.1

[PATCH v2 2/6] 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  | 37 ++++++++++-----------
 drivers/net/ethernet/amd/pds_core/core.h    |  7 ++--
 drivers/net/ethernet/amd/pds_core/devlink.c |  5 +--
 drivers/net/ethernet/amd/pds_core/main.c    | 11 +++---
 4 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
index b1fab95f7eeb..db950a9c9d30 100644
--- a/drivers/net/ethernet/amd/pds_core/auxbus.c
+++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
@@ -175,30 +175,33 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
 	return padev;
 }
 
-void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
+void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf,
+			 struct pds_auxiliary_dev **pd_ptr)
 {
 	struct pds_auxiliary_dev *padev;
 
+	if (!*pd_ptr)
+		return;
+
 	mutex_lock(&pf->config_lock);
 
-	padev = pf->vfs[cf->vf_id].padev;
-	if (padev) {
-		pds_client_unregister(pf, padev->client_id);
-		auxiliary_device_delete(&padev->aux_dev);
-		auxiliary_device_uninit(&padev->aux_dev);
-		padev->client_id = 0;
-	}
-	pf->vfs[cf->vf_id].padev = NULL;
+	padev = *pd_ptr;
+	pds_client_unregister(pf, padev->client_id);
+	auxiliary_device_delete(&padev->aux_dev);
+	auxiliary_device_uninit(&padev->aux_dev);
+	padev->client_id = 0;
+	*pd_ptr = NULL;
 
 	mutex_unlock(&pf->config_lock);
 	return;
 }
 
-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;
@@ -207,6 +210,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) |
@@ -218,17 +224,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 &&
@@ -254,7 +253,7 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf)
 		err = PTR_ERR(padev);
 		goto out_unlock;
 	}
-	pf->vfs[cf->vf_id].padev = padev;
+	*pd_ptr = padev;
 
 out_unlock:
 	mutex_unlock(&pf->config_lock);
diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
index 631a59cfdd7e..f075e68c64db 100644
--- a/drivers/net/ethernet/amd/pds_core/core.h
+++ b/drivers/net/ethernet/amd/pds_core/core.h
@@ -303,8 +303,11 @@ void pdsc_health_thread(struct work_struct *work);
 int pdsc_register_notify(struct notifier_block *nb);
 void pdsc_unregister_notify(struct notifier_block *nb);
 void pdsc_notify(unsigned long event, void *data);
-int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf);
-void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf);
+int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf,
+			enum pds_core_vif_types vt,
+			struct pds_auxiliary_dev **pd_ptr);
+void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf,
+			 struct pds_auxiliary_dev **pd_ptr);
 
 void pdsc_process_adminq(struct pdsc_qcq *qcq);
 void pdsc_work_thread(struct work_struct *work);
diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
index 4e2b92ddef6f..c5c787df61a4 100644
--- a/drivers/net/ethernet/amd/pds_core/devlink.c
+++ b/drivers/net/ethernet/amd/pds_core/devlink.c
@@ -57,9 +57,10 @@ int pdsc_dl_enable_set(struct devlink *dl, u32 id,
 		struct pdsc *vf = pdsc->vfs[vf_id].vf;
 
 		if (ctx->val.vbool)
-			err = pdsc_auxbus_dev_add(vf, pdsc);
+			err = pdsc_auxbus_dev_add(vf, pdsc, vt_entry->vif_id,
+						  &pdsc->vfs[vf_id].padev);
 		else
-			pdsc_auxbus_dev_del(vf, pdsc);
+			pdsc_auxbus_dev_del(vf, pdsc, &pdsc->vfs[vf_id].padev);
 	}
 
 	return err;
diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
index 660268ff9562..a3a68889137b 100644
--- a/drivers/net/ethernet/amd/pds_core/main.c
+++ b/drivers/net/ethernet/amd/pds_core/main.c
@@ -190,7 +190,8 @@ static int pdsc_init_vf(struct pdsc *vf)
 	devl_unlock(dl);
 
 	pf->vfs[vf->vf_id].vf = vf;
-	err = pdsc_auxbus_dev_add(vf, pf);
+	err = pdsc_auxbus_dev_add(vf, pf, PDS_DEV_TYPE_VDPA,
+				  &pf->vfs[vf->vf_id].padev);
 	if (err) {
 		devl_lock(dl);
 		devl_unregister(dl);
@@ -417,7 +418,7 @@ static void pdsc_remove(struct pci_dev *pdev)
 
 		pf = pdsc_get_pf_struct(pdsc->pdev);
 		if (!IS_ERR(pf)) {
-			pdsc_auxbus_dev_del(pdsc, pf);
+			pdsc_auxbus_dev_del(pdsc, pf, &pf->vfs[pdsc->vf_id].padev);
 			pf->vfs[pdsc->vf_id].vf = NULL;
 		}
 	} else {
@@ -482,7 +483,8 @@ static void pdsc_reset_prepare(struct pci_dev *pdev)
 
 		pf = pdsc_get_pf_struct(pdsc->pdev);
 		if (!IS_ERR(pf))
-			pdsc_auxbus_dev_del(pdsc, pf);
+			pdsc_auxbus_dev_del(pdsc, pf,
+					    &pf->vfs[pdsc->vf_id].padev);
 	}
 
 	pdsc_unmap_bars(pdsc);
@@ -527,7 +529,8 @@ static void pdsc_reset_done(struct pci_dev *pdev)
 
 		pf = pdsc_get_pf_struct(pdsc->pdev);
 		if (!IS_ERR(pf))
-			pdsc_auxbus_dev_add(pdsc, pf);
+			pdsc_auxbus_dev_add(pdsc, pf, PDS_DEV_TYPE_VDPA,
+					    &pf->vfs[pdsc->vf_id].padev);
 	}
 }
 
-- 
2.17.1

[PATCH v2 3/6] pds_core: add new fwctl auxiliary_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   | 11 +++++++++++
 include/linux/pds/pds_common.h             |  2 ++
 5 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
index db950a9c9d30..ac6f76c161f2 100644
--- a/drivers/net/ethernet/amd/pds_core/auxbus.c
+++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
@@ -225,8 +225,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 f075e68c64db..0bf320c43083 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..41575c7a148d 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_stop;
+
 	dl = priv_to_devlink(pdsc);
 	devl_lock(dl);
 	err = devl_params_register(dl, pdsc_dl_params,
@@ -297,6 +301,7 @@ static int pdsc_init_pf(struct pdsc *pdsc)
 	devlink_params_unregister(dl, pdsc_dl_params,
 				  ARRAY_SIZE(pdsc_dl_params));
 err_out_stop:
+	pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
 	pdsc_stop(pdsc);
 err_out_teardown:
 	pdsc_teardown(pdsc, PDSC_TEARDOWN_REMOVING);
@@ -427,6 +432,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 +491,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 +539,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

[PATCH v2 4/6] 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       | 169 +++++++++++++++++++++++++++++++++
 include/linux/pds/pds_adminq.h |  77 +++++++++++++++
 include/uapi/fwctl/fwctl.h     |   1 +
 include/uapi/fwctl/pds.h       |  27 ++++++
 8 files changed, 296 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 7d2700d1ba0f..a396726feb6f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9601,6 +9601,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,4 +2,5 @@
 obj-$(CONFIG_FWCTL) += fwctl.o
 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..afc7dc283ad5
--- /dev/null
+++ b/drivers/fwctl/pds/main.c
@@ -0,0 +1,169 @@
+// 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;
+	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);
+
+	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 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;
+	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 buffer\n");
+		return err;
+	}
+
+	cmd = (union pds_core_adminq_cmd) {
+		.fwctl_ident = {
+			.opcode = PDS_FWCTL_CMD_IDENT,
+			.version = 0,
+			.len = cpu_to_le32(sizeof(*ident)),
+			.ident_pa = cpu_to_le64(ident_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 err: %d\n",
+			cmd.fwctl_ident.opcode, err);
+	else
+		pdsfc->ident = *ident;
+
+	dma_free_coherent(dev->parent, sizeof(*ident), ident, ident_pa);
+
+	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 pds_auxiliary_dev *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);
+	struct device *dev = &adev->dev;
+	int err;
+
+	if (!pdsfc) {
+		dev_err(dev, "Failed to allocate fwctl device struct\n");
+		return -ENOMEM;
+	}
+	pdsfc->padev = padev;
+
+	err = pdsfc_identify(pdsfc);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to identify device\n");
+
+	err = fwctl_register(&pdsfc->fwctl);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to register device\n");
+
+	auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
+
+	return 0;
+}
+
+static void pdsfc_remove(struct auxiliary_device *adev)
+{
+	struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
+
+	fwctl_unregister(&pdsfc->fwctl);
+}
+
+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..ae6886ebc841 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,5 +44,6 @@ enum fwctl_device_type {
 	FWCTL_DEVICE_TYPE_ERROR = 0,
 	FWCTL_DEVICE_TYPE_MLX5 = 1,
+	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

[PATCH v2 5/6] 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       | 343 ++++++++++++++++++++++++++++++++-
 include/linux/pds/pds_adminq.h | 187 ++++++++++++++++++
 include/uapi/fwctl/pds.h       |  16 ++
 3 files changed, 544 insertions(+), 2 deletions(-)

diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
index afc7dc283ad5..bb2ac0ca3963 100644
--- a/drivers/fwctl/pds/main.c
+++ b/drivers/fwctl/pds/main.c
@@ -21,12 +21,22 @@ struct pdsfc_uctx {
 	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;
 	struct pdsc *pdsc;
 	u32 caps;
 	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));
 
@@ -95,10 +105,331 @@ 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;
+	int i;
+
+	if (!pdsfc->endpoints)
+		return;
+
+	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;
+	u32 num_endpoints;
+	int i;
+
+	num_endpoints = le32_to_cpu(pdsfc->endpoints->num_entries);
+	for (i = 0; i < num_endpoints; 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 device *dev = &pdsfc->fwctl.dev;
+	union pds_core_adminq_comp comp = {0};
+	struct pds_fwctl_query_data *data;
+	union pds_core_adminq_cmd cmd;
+	dma_addr_t data_pa;
+	int err;
+
+	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 = (union pds_core_adminq_cmd) {
+		.fwctl_query = {
+			.opcode = PDS_FWCTL_CMD_QUERY,
+			.entity = PDS_FWCTL_RPC_ROOT,
+			.version = 0,
+			.query_data_buf_len = cpu_to_le32(PAGE_SIZE),
+			.query_data_buf_pa = cpu_to_le64(data_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;
+
+	return data;
+}
+
+static int pdsfc_init_endpoints(struct pdsfc_dev *pdsfc)
+{
+	struct pds_fwctl_query_data_endpoint *ep_entry;
+	u32 num_endpoints;
+	int i;
+
+	pdsfc->endpoints = pdsfc_get_endpoints(pdsfc, &pdsfc->endpoints_pa);
+	if (IS_ERR(pdsfc->endpoints))
+		return PTR_ERR(pdsfc->endpoints);
+
+	num_endpoints = le32_to_cpu(pdsfc->endpoints->num_entries);
+	pdsfc->endpoint_info = vcalloc(num_endpoints,
+				       sizeof(*pdsfc->endpoint_info));
+	if (!pdsfc->endpoint_info) {
+		pdsfc_free_endpoints(pdsfc);
+		return -ENOMEM;
+	}
+
+	ep_entry = (struct pds_fwctl_query_data_endpoint *)pdsfc->endpoints->entries;
+	for (i = 0; i < num_endpoints; 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 device *dev = &pdsfc->fwctl.dev;
+	union pds_core_adminq_comp comp = {0};
+	struct pds_fwctl_query_data *data;
+	union pds_core_adminq_cmd cmd;
+	dma_addr_t data_pa;
+	int err;
+
+	/* 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 = (union pds_core_adminq_cmd) {
+		.fwctl_query = {
+			.opcode = PDS_FWCTL_CMD_QUERY,
+			.entity = PDS_FWCTL_RPC_ENDPOINT,
+			.version = 0,
+			.query_data_buf_len = cpu_to_le32(PAGE_SIZE),
+			.query_data_buf_pa = cpu_to_le64(data_pa),
+			.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;
+
+	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;
+	struct pdsfc_rpc_endpoint_info *ep_info = NULL;
+	struct device *dev = &pdsfc->fwctl.dev;
+	int i;
+
+	/* 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;
+	struct device *dev = &uctx->fwctl->dev;
+	union pds_core_adminq_comp comp = {0};
+	dma_addr_t out_payload_dma_addr = 0;
+	dma_addr_t in_payload_dma_addr = 0;
+	union pds_core_adminq_cmd cmd;
+	void *out_payload = NULL;
+	void *in_payload = NULL;
+	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");
+			in_payload_dma_addr = 0;
+			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_payload_dma_addr = 0;
+			out = ERR_PTR(err);
+			goto done;
+		}
+	}
+
+	cmd = (union pds_core_adminq_cmd) {
+		.fwctl_rpc = {
+			.opcode = PDS_FWCTL_CMD_RPC,
+			.flags = PDS_FWCTL_RPC_IND_REQ | PDS_FWCTL_RPC_IND_RESP,
+			.ep = cpu_to_le32(rpc->in.ep),
+			.op = cpu_to_le32(rpc->in.op),
+			.req_pa = cpu_to_le64(in_payload_dma_addr),
+			.req_sz = cpu_to_le32(rpc->in.len),
+			.resp_pa = cpu_to_le64(out_payload_dma_addr),
+			.resp_sz = cpu_to_le32(rpc->out.len),
+		}
+	};
+
+	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);
+
+	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);
+
+	kfree(in_payload);
+	kfree(out_payload);
+
+	return out;
 }
 
 static const struct fwctl_ops pdsfc_ops = {
@@ -131,9 +462,15 @@ static int pdsfc_probe(struct auxiliary_device *adev,
 	if (err)
 		return dev_err_probe(dev, err, "Failed to identify device\n");
 
-	err = fwctl_register(&pdsfc->fwctl);
+	err = pdsfc_init_endpoints(pdsfc);
 	if (err)
+		return dev_err_probe(dev, err, "Failed to init endpoints\n");
+
+	err = fwctl_register(&pdsfc->fwctl);
+	if (err) {
+		pdsfc_free_endpoints(pdsfc);
 		return dev_err_probe(dev, err, "Failed to register device\n");
+	}
 
 	auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
 
@@ -145,6 +482,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);
 }
 
 static const struct auxiliary_device_id pdsfc_id_table[] = {
diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
index ae6886ebc841..03bca2d27de0 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

[PATCH v2 6/6] 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..f34645dbf5ea
--- /dev/null
+++ b/Documentation/userspace-api/fwctl/pds_fwctl.rst
@@ -0,0 +1,40 @@
+.. 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 commands through pds_core's message queue to the
+firmware 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 minimum 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 request is then put
+together with the request data and sent through pds_core's message queue
+to the firmware, and the results are returned to the caller.
-- 
2.17.1

Re: [PATCH v2 0/6] pds_fwctl: fwctl for AMD/Pensando core devices

[Leon Romanovsky]

On Fri, Feb 28, 2025 at 05:35:48PM -0800, Shannon Nelson wrote:
> Following along from Jason's work [1] we have our initial 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 are a bit of clean up for pds_core's add and delete
> of auxiliary_devices.  Those are followed by a patch to add the new
> pds_core.fwctl auxiliary_device.  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-v5-642aa0c94070+4447f-fwctl_jgg@nvidia.com/
> 
> v2:
>   - removed the RFC tag
>   - add a patch to make pdsc_auxbus_dev_del() a void type (Jonathan)
>   - fix up error handling if pdsc_auxbus_dev_add() fails in probe (Jonathan)
>   - fix auxiliary spelling in commit subject header (Jonathan)
>   - clean up of code around use of __free() gizmo (Jonathan, David)
>   - removed extra whitespace and dev_xxx() calls (Leon)
>   - copy ident info from DMA and release DMA memory in probe (Jonathan)
>   - use dev_err_probe() (Jonathan)
>   - add counted_by_le(num_entries) (Jonathan, David)
>   - convert num_entries from __le32 to host in get_endpoints() (Jonathan)
>   - remove unnecessary variable inits (Jonathan, Leon)
> 
> v1: https://lore.kernel.org/netdev/20250211234854.52277-1-shannon.nelson@amd.com/
> 
> Brett Creeley (1):
>   pds_fwctl: add rpc and query support
> 
> Shannon Nelson (5):
>   pds_core: make pdsc_auxbus_dev_del() void
>   pds_core: specify auxiliary_device to be created
>   pds_core: add new fwctl auxiliary_device
>   pds_fwctl: initial driver framework
>   pds_fwctl: add Documentation entries
> 

Unfortunately, you didn't remove useless defines :(

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

[Dave Jiang]

On 2/28/25 6:35 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       | 169 +++++++++++++++++++++++++++++++++
>  include/linux/pds/pds_adminq.h |  77 +++++++++++++++
>  include/uapi/fwctl/fwctl.h     |   1 +
>  include/uapi/fwctl/pds.h       |  27 ++++++
>  8 files changed, 296 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 7d2700d1ba0f..a396726feb6f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9601,6 +9601,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,4 +2,5 @@
>  obj-$(CONFIG_FWCTL) += fwctl.o
>  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..afc7dc283ad5
> --- /dev/null
> +++ b/drivers/fwctl/pds/main.c
> @@ -0,0 +1,169 @@
> +// 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;
> +	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);
> +
> +	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 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;
> +	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 buffer\n");
> +		return err;
> +	}
> +
> +	cmd = (union pds_core_adminq_cmd) {
> +		.fwctl_ident = {
> +			.opcode = PDS_FWCTL_CMD_IDENT,
> +			.version = 0,
> +			.len = cpu_to_le32(sizeof(*ident)),
> +			.ident_pa = cpu_to_le64(ident_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 err: %d\n",
> +			cmd.fwctl_ident.opcode, err);
> +	else
> +		pdsfc->ident = *ident;
> +
> +	dma_free_coherent(dev->parent, sizeof(*ident), ident, ident_pa);
> +
> +	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 pds_auxiliary_dev *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);
> +	struct device *dev = &adev->dev;
> +	int err;
> +

It's ok to move the pdsfc declaration inline right before this check below. That would help prevent any accidental usages of pdsfc before the check. This is an exception to the coding style guidelines with the introduction of cleanup mechanisms. 
> +	if (!pdsfc) {
> +		dev_err(dev, "Failed to allocate fwctl device struct\n");
> +		return -ENOMEM;
> +	}
> +	pdsfc->padev = padev;
> +
> +	err = pdsfc_identify(pdsfc);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to identify device\n");
> +
> +	err = fwctl_register(&pdsfc->fwctl);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to register device\n");
> +
> +	auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
> +
> +	return 0;
> +}
> +
> +static void pdsfc_remove(struct auxiliary_device *adev)
> +{
> +	struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
> +
> +	fwctl_unregister(&pdsfc->fwctl);

Missing fwctl_put(). See fwctl_unregister() header comments. Caller must still call fwctl_put() to free the fwctl after calling fwctl_unregister().

DJ
 
> +}
> +
> +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..ae6886ebc841 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,5 +44,6 @@ enum fwctl_device_type {
>  	FWCTL_DEVICE_TYPE_ERROR = 0,
>  	FWCTL_DEVICE_TYPE_MLX5 = 1,
> +	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: [PATCH v2 4/6] pds_fwctl: initial driver framework

[Jason Gunthorpe]

On Mon, Mar 03, 2025 at 09:45:52AM -0700, Dave Jiang wrote:
> > +static int pdsfc_probe(struct auxiliary_device *adev,
> > +		       const struct auxiliary_device_id *id)
> > +{
> > +	struct pds_auxiliary_dev *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);
> > +	struct device *dev = &adev->dev;
> > +	int err;
> > +
> 
> It's ok to move the pdsfc declaration inline right before this check
> below. That would help prevent any accidental usages of pdsfc before
> the check. This is an exception to the coding style guidelines with
> the introduction of cleanup mechanisms.

Yeah.. I'm starting to feel negative about cleanup.h - there are too
many special style notes that seem to only be known by hearsay :\

> > +static void pdsfc_remove(struct auxiliary_device *adev)
> > +{
> > +	struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
> > +
> > +	fwctl_unregister(&pdsfc->fwctl);
> 
> Missing fwctl_put(). See fwctl_unregister() header comments. Caller
> must still call fwctl_put() to free the fwctl after calling
> fwctl_unregister().

The code is correct, the put is hidden in the __free

However I think we decided not to use __free in this context and open
code the put as a style choice

Thanks,
Jason

Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

[Nelson, Shannon]

On 3/3/2025 8:45 AM, Dave Jiang wrote:
> 
> On 2/28/25 6:35 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       | 169 +++++++++++++++++++++++++++++++++
>>   include/linux/pds/pds_adminq.h |  77 +++++++++++++++
>>   include/uapi/fwctl/fwctl.h     |   1 +
>>   include/uapi/fwctl/pds.h       |  27 ++++++
>>   8 files changed, 296 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 7d2700d1ba0f..a396726feb6f 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9601,6 +9601,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,4 +2,5 @@
>>   obj-$(CONFIG_FWCTL) += fwctl.o
>>   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..afc7dc283ad5
>> --- /dev/null
>> +++ b/drivers/fwctl/pds/main.c
>> @@ -0,0 +1,169 @@
>> +// 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;
>> +     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);
>> +
>> +     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 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;
>> +     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 buffer\n");
>> +             return err;
>> +     }
>> +
>> +     cmd = (union pds_core_adminq_cmd) {
>> +             .fwctl_ident = {
>> +                     .opcode = PDS_FWCTL_CMD_IDENT,
>> +                     .version = 0,
>> +                     .len = cpu_to_le32(sizeof(*ident)),
>> +                     .ident_pa = cpu_to_le64(ident_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 err: %d\n",
>> +                     cmd.fwctl_ident.opcode, err);
>> +     else
>> +             pdsfc->ident = *ident;
>> +
>> +     dma_free_coherent(dev->parent, sizeof(*ident), ident, ident_pa);
>> +
>> +     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 pds_auxiliary_dev *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);
>> +     struct device *dev = &adev->dev;
>> +     int err;
>> +
> 
> It's ok to move the pdsfc declaration inline right before this check below. That would help prevent any accidental usages of pdsfc before the check. This is an exception to the coding style guidelines with the introduction of cleanup mechanisms.




>> +     if (!pdsfc) {
>> +             dev_err(dev, "Failed to allocate fwctl device struct\n");
>> +             return -ENOMEM;
>> +     }
>> +     pdsfc->padev = padev;
>> +
>> +     err = pdsfc_identify(pdsfc);
>> +     if (err)
>> +             return dev_err_probe(dev, err, "Failed to identify device\n");
>> +
>> +     err = fwctl_register(&pdsfc->fwctl);
>> +     if (err)
>> +             return dev_err_probe(dev, err, "Failed to register device\n");
>> +
>> +     auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
>> +
>> +     return 0;
>> +}
>> +
>> +static void pdsfc_remove(struct auxiliary_device *adev)
>> +{
>> +     struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>> +
>> +     fwctl_unregister(&pdsfc->fwctl);
> 
> Missing fwctl_put(). See fwctl_unregister() header comments. Caller must still call fwctl_put() to free the fwctl after calling fwctl_unregister().

Is this not covered by the pdsfc_dev cleanup we defined earlier?
sln

> 
> DJ
> 
>> +}
>> +
>> +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..ae6886ebc841 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,5 +44,6 @@ enum fwctl_device_type {
>>        FWCTL_DEVICE_TYPE_ERROR = 0,
>>        FWCTL_DEVICE_TYPE_MLX5 = 1,
>> +     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: [PATCH v2 4/6] pds_fwctl: initial driver framework

[Nelson, Shannon]

On 3/3/2025 9:29 AM, Jason Gunthorpe wrote:
> 
> On Mon, Mar 03, 2025 at 09:45:52AM -0700, Dave Jiang wrote:
>>> +static int pdsfc_probe(struct auxiliary_device *adev,
>>> +                  const struct auxiliary_device_id *id)
>>> +{
>>> +   struct pds_auxiliary_dev *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);
>>> +   struct device *dev = &adev->dev;
>>> +   int err;
>>> +
>>
>> It's ok to move the pdsfc declaration inline right before this check
>> below. That would help prevent any accidental usages of pdsfc before
>> the check. This is an exception to the coding style guidelines with
>> the introduction of cleanup mechanisms.
> 
> Yeah.. I'm starting to feel negative about cleanup.h - there are too
> many special style notes that seem to only be known by hearsay :\
> 
>>> +static void pdsfc_remove(struct auxiliary_device *adev)
>>> +{
>>> +   struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>>> +
>>> +   fwctl_unregister(&pdsfc->fwctl);
>>
>> Missing fwctl_put(). See fwctl_unregister() header comments. Caller
>> must still call fwctl_put() to free the fwctl after calling
>> fwctl_unregister().
> 
> The code is correct, the put is hidden in the __free
> 
> However I think we decided not to use __free in this context and open
> code the put as a style choice

I'm of the same opinion.  I pulled this pattern out of the other 
functions, but left it here as it seemed less 'different'.  Maybe this 
will go away as well if there are other reasons to rework the code.

sln

Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

[Dave Jiang]

On 3/3/25 10:31 AM, Nelson, Shannon wrote:
> On 3/3/2025 8:45 AM, Dave Jiang wrote:
>>
>> On 2/28/25 6:35 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       | 169 +++++++++++++++++++++++++++++++++
>>>   include/linux/pds/pds_adminq.h |  77 +++++++++++++++
>>>   include/uapi/fwctl/fwctl.h     |   1 +
>>>   include/uapi/fwctl/pds.h       |  27 ++++++
>>>   8 files changed, 296 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 7d2700d1ba0f..a396726feb6f 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -9601,6 +9601,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,4 +2,5 @@
>>>   obj-$(CONFIG_FWCTL) += fwctl.o
>>>   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..afc7dc283ad5
>>> --- /dev/null
>>> +++ b/drivers/fwctl/pds/main.c
>>> @@ -0,0 +1,169 @@
>>> +// 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;
>>> +     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);
>>> +
>>> +     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 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;
>>> +     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 buffer\n");
>>> +             return err;
>>> +     }
>>> +
>>> +     cmd = (union pds_core_adminq_cmd) {
>>> +             .fwctl_ident = {
>>> +                     .opcode = PDS_FWCTL_CMD_IDENT,
>>> +                     .version = 0,
>>> +                     .len = cpu_to_le32(sizeof(*ident)),
>>> +                     .ident_pa = cpu_to_le64(ident_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 err: %d\n",
>>> +                     cmd.fwctl_ident.opcode, err);
>>> +     else
>>> +             pdsfc->ident = *ident;
>>> +
>>> +     dma_free_coherent(dev->parent, sizeof(*ident), ident, ident_pa);
>>> +
>>> +     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 pds_auxiliary_dev *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);
>>> +     struct device *dev = &adev->dev;
>>> +     int err;
>>> +
>>
>> It's ok to move the pdsfc declaration inline right before this check below. That would help prevent any accidental usages of pdsfc before the check. This is an exception to the coding style guidelines with the introduction of cleanup mechanisms.
> 
> 
> 
> 
>>> +     if (!pdsfc) {
>>> +             dev_err(dev, "Failed to allocate fwctl device struct\n");
>>> +             return -ENOMEM;
>>> +     }
>>> +     pdsfc->padev = padev;
>>> +
>>> +     err = pdsfc_identify(pdsfc);
>>> +     if (err)
>>> +             return dev_err_probe(dev, err, "Failed to identify device\n");
>>> +
>>> +     err = fwctl_register(&pdsfc->fwctl);
>>> +     if (err)
>>> +             return dev_err_probe(dev, err, "Failed to register device\n");
>>> +
>>> +     auxiliary_set_drvdata(adev, no_free_ptr(pdsfc));
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static void pdsfc_remove(struct auxiliary_device *adev)
>>> +{
>>> +     struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>>> +
>>> +     fwctl_unregister(&pdsfc->fwctl);
>>
>> Missing fwctl_put(). See fwctl_unregister() header comments. Caller must still call fwctl_put() to free the fwctl after calling fwctl_unregister().
> 
> Is this not covered by the pdsfc_dev cleanup we defined earlier?

Yup. I missed the __free(). Sorry about the noise. 

> sln
> 
>>
>> DJ
>>
>>> +}
>>> +
>>> +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..ae6886ebc841 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,5 +44,6 @@ enum fwctl_device_type {
>>>        FWCTL_DEVICE_TYPE_ERROR = 0,
>>>        FWCTL_DEVICE_TYPE_MLX5 = 1,
>>> +     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: [PATCH v2 4/6] pds_fwctl: initial driver framework

[Jonathan Cameron]

On Mon, 3 Mar 2025 13:29:53 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Mon, Mar 03, 2025 at 09:45:52AM -0700, Dave Jiang wrote:
> > > +static int pdsfc_probe(struct auxiliary_device *adev,
> > > +		       const struct auxiliary_device_id *id)
> > > +{
> > > +	struct pds_auxiliary_dev *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);
> > > +	struct device *dev = &adev->dev;
> > > +	int err;
> > > +  
> > 
> > It's ok to move the pdsfc declaration inline right before this check
> > below. That would help prevent any accidental usages of pdsfc before
> > the check. This is an exception to the coding style guidelines with
> > the introduction of cleanup mechanisms.  
> 
> Yeah.. I'm starting to feel negative about cleanup.h - there are too
> many special style notes that seem to only be known by hearsay :\

I think this one is more about the classic risk of undefined
behavior meaning the compiler removes the check.  So if we had a
bob = pdsfc->fred;

but didn't use bob before the check on pdsfc != NULL then
the compiler is allowed to remove the check on pdsfc existing
as the dereferencce can be assume to mean pdsfc is always
!= NULL.

So not really a cleanup.h related issue at all (and not in my
view suitable for adding to the nice help Dan put in that file).

Doesn't matter too much though.  Personally I'm kind of content
now with cleanup.h usage but it was a bit of time to teach
myself the new way of thinking about things.

Jonathan


> 
> > > +static void pdsfc_remove(struct auxiliary_device *adev)
> > > +{
> > > +	struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
> > > +
> > > +	fwctl_unregister(&pdsfc->fwctl);  
> > 
> > Missing fwctl_put(). See fwctl_unregister() header comments. Caller
> > must still call fwctl_put() to free the fwctl after calling
> > fwctl_unregister().  
> 
> The code is correct, the put is hidden in the __free
> 
> However I think we decided not to use __free in this context and open
> code the put as a style choice
> 
> Thanks,
> Jason

Re: [PATCH v2 1/6] pds_core: make pdsc_auxbus_dev_del() void

[Jonathan Cameron]

On Fri, 28 Feb 2025 17:35:49 -0800
Shannon Nelson <shannon.nelson@amd.com> wrote:

> Since there really is no useful return, advertising a return value
> is rather misleading.  Make pdsc_auxbus_dev_del() a void function.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v2 1/6] pds_core: make pdsc_auxbus_dev_del() void

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On Sat, Mar 1, 2025 at 7:07 AM Shannon Nelson <shannon.nelson@amd.com> wrote:
>
> Since there really is no useful return, advertising a return value
> is rather misleading.  Make pdsc_auxbus_dev_del() a void function.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>

LGTM,
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>


-- 
Regards,
Kalesh AP

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

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

[Jonathan Cameron]

On Fri, 28 Feb 2025 17:35: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>
Hi Shannon,

Seems fine to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v2 3/6] pds_core: add new fwctl auxiliary_device

[Jonathan Cameron]

On Fri, 28 Feb 2025 17:35: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,

A few really minor comments inline from a fresh read through.

Thanks,

Jonathan

> ---
>  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   | 11 +++++++++++
>  include/linux/pds/pds_common.h             |  2 ++
>  5 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
> index db950a9c9d30..ac6f76c161f2 100644
> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
> @@ -225,8 +225,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.

Comment feels a bit general. Is this no auxbus support for this device
or none at all in the kernel?

>  	 */
>  	vt_support = !!le16_to_cpu(pf->dev_ident.vif_types[vt]);
>  	if (!(vt_support &&

> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
> index a3a68889137b..41575c7a148d 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_stop;
> +
>  	dl = priv_to_devlink(pdsc);
>  	devl_lock(dl);
>  	err = devl_params_register(dl, pdsc_dl_params,
> @@ -297,6 +301,7 @@ static int pdsc_init_pf(struct pdsc *pdsc)
>  	devlink_params_unregister(dl, pdsc_dl_params,
>  				  ARRAY_SIZE(pdsc_dl_params));
>  err_out_stop:
> +	pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);

This doesn't smell right (by which I mean I had to go look at the
implementation to be sure it wasn't a bug) In my ideal
world that should be obvious on a more local basis.
I'd expect a new label here. pdsc_auxbus_dev_add() should be and is
side effect free if it fails. That is it should not make sense
to call pdsc_auxbus_dev_del() if it fails.

It isn't a bug today as that becomes a noop due to
&pdsc->padev being NULL but that is a detail I shouldn't
ideally need to know when reading this code.

I'd put err_out_stop label here and rename previous
one to err_out_auxbus_del + replace the existing
goto err_out_stop; 
with
goto err_out_auxbus_del;

>  	pdsc_stop(pdsc);
>  err_out_teardown:
>  	pdsc_teardown(pdsc, PDSC_TEARDOWN_REMOVING);
> @@ -427,6 +432,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 +491,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 +539,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);
>  	}
>  }

Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

[Jonathan Cameron]

On Fri, 28 Feb 2025 17:35: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>
A few minor comments inline,

> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
> new file mode 100644
> index 000000000000..afc7dc283ad5
> --- /dev/null
> +++ b/drivers/fwctl/pds/main.c
> @@ -0,0 +1,169 @@
> +struct pdsfc_dev {
> +	struct fwctl_device fwctl;
> +	struct pds_auxiliary_dev *padev;
> +	struct pdsc *pdsc;

Not used in this patch that I can see.  I was curious why it is
here as I would expect that to match with the padev parent.

> +	u32 caps;
> +	struct pds_fwctl_ident ident;
> +};
> +DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));


> +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;

What about the UID? If it is always zero, what is it for?

> +
> +	return info;
> +}

> +/**
> + * 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

11 bytes is some extreme word padding.  I'd just call it reserved
space.

> + * @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 - Firmware control identification structure
> + * @features:    Supported features

Nice to have some defines or similar that give meaning to these
features, but maybe that comes in later patches.

> + * @version:     Interface version
> + * @rsvd:        Word boundary padding

word size seems to be varying between structures. I'd just document
it as "reserved"

> + * @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;

Jonathan

Re: [PATCH v2 5/6] pds_fwctl: add rpc and query support

[Jonathan Cameron]

On Fri, 28 Feb 2025 17:35: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>
Hi

A few minor suggestions inline,

Jonathan




> +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;
> +	struct pdsfc_rpc_endpoint_info *ep_info = NULL;
> +	struct device *dev = &pdsfc->fwctl.dev;
> +	int i;
> +
> +	/* 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);

Perhaps a little noisy as I think userspace can trigger this easily.  dev_dbg()
might be better.  -EINVAL should be all userspace needs under most circumstances.

> +
> +	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;

in is a void * and the C spec always allows implicit conversion for those
so no cast here.

> +	struct device *dev = &uctx->fwctl->dev;
> +	union pds_core_adminq_comp comp = {0};
> +	dma_addr_t out_payload_dma_addr = 0;
> +	dma_addr_t in_payload_dma_addr = 0;
> +	union pds_core_adminq_cmd cmd;
> +	void *out_payload = NULL;
> +	void *in_payload = NULL;
> +	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");
> +			in_payload_dma_addr = 0;

See below for comment on ordering and why ideally this zero setting should
not be needed in error path.  Really small point though!

> +			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_payload_dma_addr = 0;

As above. Slight ordering tweaks and more labels would avoid need
to zero this on error as we would never use it again.

> +			out = ERR_PTR(err);
> +			goto done;
> +		}
> +	}
> +
> +	cmd = (union pds_core_adminq_cmd) {
> +		.fwctl_rpc = {
> +			.opcode = PDS_FWCTL_CMD_RPC,
> +			.flags = PDS_FWCTL_RPC_IND_REQ | PDS_FWCTL_RPC_IND_RESP,
> +			.ep = cpu_to_le32(rpc->in.ep),
> +			.op = cpu_to_le32(rpc->in.op),
> +			.req_pa = cpu_to_le64(in_payload_dma_addr),
> +			.req_sz = cpu_to_le32(rpc->in.len),
> +			.resp_pa = cpu_to_le64(out_payload_dma_addr),
> +			.resp_sz = cpu_to_le32(rpc->out.len),
> +		}
> +	};
> +
> +	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);
> +
> +	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)

Can we do these in reverse order of the setup path?
It obviously makes limited difference in practice.
With them in strict reverse order you could avoid need to
zero out_payload_dma_addr and similar in error paths by
using multiple labels.

> +		dma_unmap_single(dev->parent, out_payload_dma_addr,
> +				 rpc->out.len, DMA_FROM_DEVICE);
> +
> +	kfree(in_payload);
> +	kfree(out_payload);
> +
> +	return out;

At least some cases are simplified if you do
	if (err)
		return ERR_PTR(err);

	return in;

and handle all errors via err integer until here.
>  }

>  static const struct auxiliary_device_id pdsfc_id_table[] = {
> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
> index ae6886ebc841..03bca2d27de0 100644
> --- a/include/linux/pds/pds_adminq.h
> +++ b/include/linux/pds/pds_adminq.h

> +/**
> + * 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() probably a nice to have 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];

__le32 rsvd; ?  Or stick to u8 only.


> +} __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.

Odd mix of full stop and not.  Make it more consistent.

> + * @resp_sz:    Size of the response.
> + * @rsvd2:      Word boundary padding

words changing size in one structure?  Just stick to "Reserved" for
the docs. Never any reason to explicitly talk about 'why' things
are reserved.

> + * @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: [PATCH v2 6/6] pds_fwctl: add Documentation entries

[Jonathan Cameron]

On Fri, 28 Feb 2025 17:35: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>
Really minor stuff inline.

Thanks,

Jonathan

> ---
>  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..f34645dbf5ea
> --- /dev/null
> +++ b/Documentation/userspace-api/fwctl/pds_fwctl.rst
> @@ -0,0 +1,40 @@
> +.. 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

fwctl is a class not a bus though here I'd be tempted to say subsystem.

> +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 commands through pds_core's message queue to the
> +firmware 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.
requests from the firmware both a list of endpoints and a list of operations for
each endpoint. 

As currently written the sentence suggest that we are asking for something
unspecified from the "firmware list of endpoints..."

> +This list of operations includes a minimum 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 request is then put
> +together with the request data and sent through pds_core's message queue
> +to the firmware, and the results are returned to the caller.

Re: [PATCH v2 5/6] pds_fwctl: add rpc and query support

[Jason Gunthorpe]

On Tue, Mar 04, 2025 at 05:08:08PM +0800, Jonathan Cameron wrote:
> > +	dev_err(dev, "Invalid operation %d for endpoint %d\n", rpc->in.op, rpc->in.ep);
> 
> Perhaps a little noisy as I think userspace can trigger this easily.  dev_dbg()
> might be better.  -EINVAL should be all userspace needs under most circumstances.

Yes, please remove or degrade to dbg all the prints that userspace
could trigger.

> > +	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");

kzalloc is already super noisy if it fails

> > +			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");
> > +			in_payload_dma_addr = 0;

etc

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

Triggerable by a malformed RPC?

> > +		out = ERR_PTR(err);
> > +		goto done;
> > +	}
> > +
> > +	dynamic_hex_dump("out ", DUMP_PREFIX_OFFSET, 16, 1, out_payload, rpc->out.len, true);
> > +
> > +	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");

Triggerable by a malformed user provided pointer

Jason

Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

[Jason Gunthorpe]

On Fri, Feb 28, 2025 at 05:35:52PM -0800, Shannon Nelson wrote:
> +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;
> +	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 buffer\n");
> +		return err;
> +	}
> +
> +	cmd = (union pds_core_adminq_cmd) {
> +		.fwctl_ident = {
> +			.opcode = PDS_FWCTL_CMD_IDENT,
> +			.version = 0,
> +			.len = cpu_to_le32(sizeof(*ident)),
> +			.ident_pa = cpu_to_le64(ident_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 err: %d\n",
> +			cmd.fwctl_ident.opcode, err);
> +	else
> +		pdsfc->ident = *ident;
> +
> +	dma_free_coherent(dev->parent, sizeof(*ident), ident, ident_pa);
> +
> +	return 0;

Is it intential to loose the pds_client_adminq_cmd err? Maybe needs a
comment if so

> +/**
> + * 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;

What's your plan for the scope indication? Right now this would be
restricted to the most restricted FWCTL_RPC_CONFIGURATION scope in FW.

> +/* 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;

I think Jonathon remarked, the uid should go since it isn't used.

Jason

Re: [PATCH v2 5/6] pds_fwctl: add rpc and query support

[Jason Gunthorpe]

On Fri, Feb 28, 2025 at 05:35:53PM -0800, Shannon Nelson wrote:

> +	/* 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;

Oh I see, you are doing the scope like CXL with a "command intent's
report". That is a good idea.

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

This wasn't really the intention to have a payload pointer inside the
existing payload pointer, is it the same issue as CXL where you wanted
a header?

I see your FW API also can't take a scatterlist, so it makes sense it
needs to be linearized like this.

I don't think it makes a difference to have the indirection or not, it
would be a bunch of stuff to keep the header seperate from the payload
and then still also end up with memcpy in the driver, so leave it.

> +#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)

Isn't that FIELD_GET?

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

Use __aligned_u64 in the uapi headers

Jason

Re: [PATCH v2 3/6] pds_core: add new fwctl auxiliary_device

[Nelson, Shannon]

On 3/4/2025 12:03 AM, Jonathan Cameron wrote:
> On Fri, 28 Feb 2025 17:35: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,
> 
> A few really minor comments inline from a fresh read through.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>   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   | 11 +++++++++++
>>   include/linux/pds/pds_common.h             |  2 ++
>>   5 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
>> index db950a9c9d30..ac6f76c161f2 100644
>> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
>> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
>> @@ -225,8 +225,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.
> 
> Comment feels a bit general. Is this no auxbus support for this device
> or none at all in the kernel?

Either one of no CONFIG_AUXILIARY_BUS or no fwctl service from the DSC. 
I'll make that more clear.


> 
>>         */
>>        vt_support = !!le16_to_cpu(pf->dev_ident.vif_types[vt]);
>>        if (!(vt_support &&
> 
>> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
>> index a3a68889137b..41575c7a148d 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_stop;
>> +
>>        dl = priv_to_devlink(pdsc);
>>        devl_lock(dl);
>>        err = devl_params_register(dl, pdsc_dl_params,
>> @@ -297,6 +301,7 @@ static int pdsc_init_pf(struct pdsc *pdsc)
>>        devlink_params_unregister(dl, pdsc_dl_params,
>>                                  ARRAY_SIZE(pdsc_dl_params));
>>   err_out_stop:
>> +     pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
> 
> This doesn't smell right (by which I mean I had to go look at the
> implementation to be sure it wasn't a bug) In my ideal
> world that should be obvious on a more local basis.
> I'd expect a new label here. pdsc_auxbus_dev_add() should be and is
> side effect free if it fails. That is it should not make sense
> to call pdsc_auxbus_dev_del() if it fails.
> 
> It isn't a bug today as that becomes a noop due to
> &pdsc->padev being NULL but that is a detail I shouldn't
> ideally need to know when reading this code.
> 
> I'd put err_out_stop label here and rename previous
> one to err_out_auxbus_del + replace the existing
> goto err_out_stop;
> with
> goto err_out_auxbus_del;

Yes, I can break that out to be more clear.

Thanks,
sln

> 
>>        pdsc_stop(pdsc);
>>   err_out_teardown:
>>        pdsc_teardown(pdsc, PDSC_TEARDOWN_REMOVING);
>> @@ -427,6 +432,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 +491,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 +539,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);
>>        }
>>   }
> 

Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

[Nelson, Shannon]

On 3/4/2025 12:30 AM, Jonathan Cameron wrote:
> On Fri, 28 Feb 2025 17:35: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>
> A few minor comments inline,
> 
>> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c
>> new file mode 100644
>> index 000000000000..afc7dc283ad5
>> --- /dev/null
>> +++ b/drivers/fwctl/pds/main.c
>> @@ -0,0 +1,169 @@
>> +struct pdsfc_dev {
>> +     struct fwctl_device fwctl;
>> +     struct pds_auxiliary_dev *padev;
>> +     struct pdsc *pdsc;
> 
> Not used in this patch that I can see.  I was curious why it is
> here as I would expect that to match with the padev parent.

I'll take it out.

> 
>> +     u32 caps;
>> +     struct pds_fwctl_ident ident;
>> +};
>> +DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl));
> 
> 
>> +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;
> 
> What about the UID? If it is always zero, what is it for?

Early code, thought we'd use it.  I'll take it out.

> 
>> +
>> +     return info;
>> +}
> 
>> +/**
>> + * 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
> 
> 11 bytes is some extreme word padding.  I'd just call it reserved
> space.

This is the same wording we've used in the other interface files, so at 
least we're consistently odd.

I'll tweak these.

> 
>> + * @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 - Firmware control identification structure
>> + * @features:    Supported features
> 
> Nice to have some defines or similar that give meaning to these
> features, but maybe that comes in later patches.

We don't have any defined yet, but we have found it is useful to define 
this field for future growth without breaking APIs.  I'll make that more 
clear.

> 
>> + * @version:     Interface version
>> + * @rsvd:        Word boundary padding
> 
> word size seems to be varying between structures. I'd just document
> it as "reserved"

Sure

Thanks,
sln

> 
>> + * @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;
> 
> Jonathan

Re: [PATCH v2 5/6] pds_fwctl: add rpc and query support

[Nelson, Shannon]

On 3/4/2025 1:08 AM, Jonathan Cameron wrote:
> On Fri, 28 Feb 2025 17:35: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>
> Hi
> 
> A few minor suggestions inline,
> 
> Jonathan
> 
> 
> 
> 
>> +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;
>> +     struct pdsfc_rpc_endpoint_info *ep_info = NULL;
>> +     struct device *dev = &pdsfc->fwctl.dev;
>> +     int i;
>> +
>> +     /* 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);
> 
> Perhaps a little noisy as I think userspace can trigger this easily.  dev_dbg()
> might be better.  -EINVAL should be all userspace needs under most circumstances.

Sure

> 
>> +
>> +     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;
> 
> in is a void * and the C spec always allows implicit conversion for those
> so no cast here.

Will fix

> 
>> +     struct device *dev = &uctx->fwctl->dev;
>> +     union pds_core_adminq_comp comp = {0};
>> +     dma_addr_t out_payload_dma_addr = 0;
>> +     dma_addr_t in_payload_dma_addr = 0;
>> +     union pds_core_adminq_cmd cmd;
>> +     void *out_payload = NULL;
>> +     void *in_payload = NULL;
>> +     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");
>> +                     in_payload_dma_addr = 0;
> 
> See below for comment on ordering and why ideally this zero setting should
> not be needed in error path.  Really small point though!
> 
>> +                     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_payload_dma_addr = 0;
> 
> As above. Slight ordering tweaks and more labels would avoid need
> to zero this on error as we would never use it again.
> 
>> +                     out = ERR_PTR(err);
>> +                     goto done;
>> +             }
>> +     }
>> +
>> +     cmd = (union pds_core_adminq_cmd) {
>> +             .fwctl_rpc = {
>> +                     .opcode = PDS_FWCTL_CMD_RPC,
>> +                     .flags = PDS_FWCTL_RPC_IND_REQ | PDS_FWCTL_RPC_IND_RESP,
>> +                     .ep = cpu_to_le32(rpc->in.ep),
>> +                     .op = cpu_to_le32(rpc->in.op),
>> +                     .req_pa = cpu_to_le64(in_payload_dma_addr),
>> +                     .req_sz = cpu_to_le32(rpc->in.len),
>> +                     .resp_pa = cpu_to_le64(out_payload_dma_addr),
>> +                     .resp_sz = cpu_to_le32(rpc->out.len),
>> +             }
>> +     };
>> +
>> +     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);
>> +
>> +     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)
> 
> Can we do these in reverse order of the setup path?
> It obviously makes limited difference in practice.
> With them in strict reverse order you could avoid need to
> zero out_payload_dma_addr and similar in error paths by
> using multiple labels.

Yeah, I'll reorder this with a couple more labels.

> 
>> +             dma_unmap_single(dev->parent, out_payload_dma_addr,
>> +                              rpc->out.len, DMA_FROM_DEVICE);
>> +
>> +     kfree(in_payload);
>> +     kfree(out_payload);
>> +
>> +     return out;
> 
> At least some cases are simplified if you do
>          if (err)
>                  return ERR_PTR(err);
> 
>          return in;
> 
> and handle all errors via err integer until here.
>>   }
> 
>>   static const struct auxiliary_device_id pdsfc_id_table[] = {
>> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
>> index ae6886ebc841..03bca2d27de0 100644
>> --- a/include/linux/pds/pds_adminq.h
>> +++ b/include/linux/pds/pds_adminq.h
> 
>> +/**
>> + * 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() probably a nice to have here.

Yes.


> 
> 
>> +} __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];
> 
> __le32 rsvd; ?  Or stick to u8 only.

I'll use u8

> 
> 
>> +} __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.
> 
> Odd mix of full stop and not.  Make it more consistent.

Sure


> 
>> + * @resp_sz:    Size of the response.
>> + * @rsvd2:      Word boundary padding
> 
> words changing size in one structure?  Just stick to "Reserved" for
> the docs. Never any reason to explicitly talk about 'why' things
> are reserved.

Sure

Thanks,
sln

> 
>> + * @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: [PATCH v2 6/6] pds_fwctl: add Documentation entries

[Nelson, Shannon]

On 3/4/2025 1:12 AM, Jonathan Cameron wrote:
> On Fri, 28 Feb 2025 17:35: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>
> Really minor stuff inline.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>   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..f34645dbf5ea
>> --- /dev/null
>> +++ b/Documentation/userspace-api/fwctl/pds_fwctl.rst
>> @@ -0,0 +1,40 @@
>> +.. 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
> 
> fwctl is a class not a bus though here I'd be tempted to say subsystem.

Subsystem works for me, unless Jason has any preference.

> 
>> +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 commands through pds_core's message queue to the
>> +firmware 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.
> requests from the firmware both a list of endpoints and a list of operations for
> each endpoint.
> 
> As currently written the sentence suggest that we are asking for something
> unspecified from the "firmware list of endpoints..."

I'll clean this up.

Thanks,
sln

> 
>> +This list of operations includes a minimum 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 request is then put
>> +together with the request data and sent through pds_core's message queue
>> +to the firmware, and the results are returned to the caller.
> 

Re: [PATCH v2 5/6] pds_fwctl: add rpc and query support

[Nelson, Shannon]

On 3/4/2025 9:24 AM, Jason Gunthorpe wrote:
> On Tue, Mar 04, 2025 at 05:08:08PM +0800, Jonathan Cameron wrote:
>>> +   dev_err(dev, "Invalid operation %d for endpoint %d\n", rpc->in.op, rpc->in.ep);
>>
>> Perhaps a little noisy as I think userspace can trigger this easily.  dev_dbg()
>> might be better.  -EINVAL should be all userspace needs under most circumstances.
> 
> Yes, please remove or degrade to dbg all the prints that userspace
> could trigger.

Sure

> 
>>> +   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");
> 
> kzalloc is already super noisy if it fails

I know ... I get dinged on this all the time, but I still add these 
because there are multiple allocs in this one function and it isn't 
immediately obvious in the trace which alloc failed.  I could push each 
alloc off to separate functions to get that info in the trace, but it 
seems to me that's extra unnecessary code to get the same one more line.


> 
>>> +                   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");
>>> +                   in_payload_dma_addr = 0;
> 
> etc
> 
>>> +   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);
> 
> Triggerable by a malformed RPC?

That or misbehaving firmware.

> 
>>> +           out = ERR_PTR(err);
>>> +           goto done;
>>> +   }
>>> +
>>> +   dynamic_hex_dump("out ", DUMP_PREFIX_OFFSET, 16, 1, out_payload, rpc->out.len, true);
>>> +
>>> +   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");
> 
> Triggerable by a malformed user provided pointer

yes

> 
> Jason

Thanks,
sln

Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

[Nelson, Shannon]

On 3/4/2025 11:39 AM, Jason Gunthorpe wrote:
> On Fri, Feb 28, 2025 at 05:35:52PM -0800, Shannon Nelson wrote:
>> +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;
>> +     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 buffer\n");
>> +             return err;
>> +     }
>> +
>> +     cmd = (union pds_core_adminq_cmd) {
>> +             .fwctl_ident = {
>> +                     .opcode = PDS_FWCTL_CMD_IDENT,
>> +                     .version = 0,
>> +                     .len = cpu_to_le32(sizeof(*ident)),
>> +                     .ident_pa = cpu_to_le64(ident_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 err: %d\n",
>> +                     cmd.fwctl_ident.opcode, err);
>> +     else
>> +             pdsfc->ident = *ident;
>> +
>> +     dma_free_coherent(dev->parent, sizeof(*ident), ident, ident_pa);
>> +
>> +     return 0;
> 
> Is it intential to loose the pds_client_adminq_cmd err? Maybe needs a
> comment if so

Good catch - thanks, will fix.

> 
>> +/**
>> + * 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;
> 
> What's your plan for the scope indication? Right now this would be
> restricted to the most restricted FWCTL_RPC_CONFIGURATION scope in FW.

As you noticed in the next patch, the scope restrictions will come from 
the FW when we request endpoint and operation information.

> 
>> +/* 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;
> 
> I think Jonathon remarked, the uid should go since it isn't used.

yep

Thanks,
sln

> 
> Jason

Re: [PATCH v2 5/6] pds_fwctl: add rpc and query support

[Nelson, Shannon]

On 3/4/2025 11:52 AM, Jason Gunthorpe wrote:
> On Fri, Feb 28, 2025 at 05:35:53PM -0800, Shannon Nelson wrote:
> 
>> +     /* 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;
> 
> Oh I see, you are doing the scope like CXL with a "command intent's
> report". That is a good idea.
> 
>> +     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;
>> +             }
> 
> This wasn't really the intention to have a payload pointer inside the
> existing payload pointer, is it the same issue as CXL where you wanted
> a header?
> 
> I see your FW API also can't take a scatterlist, so it makes sense it
> needs to be linearized like this.
> 
> I don't think it makes a difference to have the indirection or not, it
> would be a bunch of stuff to keep the header seperate from the payload
> and then still also end up with memcpy in the driver, so leave it.
> 
>> +#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)
> 
> Isn't that FIELD_GET?

Hmm... I think so.  I'll poke at that.

> 
>> +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;
>> +};
> 
> Use __aligned_u64 in the uapi headers

Sure.

Thanks,
sln



> 
> Jason