| Message ID | 20250301013554.49511-5-shannon.nelson@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pds_fwctl: fwctl for AMD/Pensando core devices | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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_ */
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
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_ */ >
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
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_ */ >> >
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
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
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
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
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
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_ */
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