| Message ID | 20250211234854.52277-4-shannon.nelson@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | pds_fwctl: fwctl for AMD/Pensando core devices | expand |
On Tue, 11 Feb 2025 15:48:52 -0800 Shannon Nelson <shannon.nelson@amd.com> wrote: > Initial files for adding a new fwctl driver for the AMD/Pensando PDS > devices. This sets up a simple auxiliary_bus driver that registers > with fwctl subsystem. It expects that a pds_core device has set up > the auxiliary_device pds_core.fwctl > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > --- Hi Shannon, A few comments inline Jonathan > diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c > new file mode 100644 > index 000000000000..24979fe0deea > --- /dev/null > +++ b/drivers/fwctl/pds/main.c > @@ -0,0 +1,195 @@ > + > +static int pdsfc_identify(struct pdsfc_dev *pdsfc) > +{ > + struct device *dev = &pdsfc->fwctl.dev; > + union pds_core_adminq_comp comp = {0}; > + union pds_core_adminq_cmd cmd = {0}; > + struct pds_fwctl_ident *ident; > + dma_addr_t ident_pa; > + int err = 0; > + > + ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL); > + err = dma_mapping_error(dev->parent, ident_pa); > + if (err) { > + dev_err(dev, "Failed to map ident\n"); > + return err; > + } > + > + cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT; > + cmd.fwctl_ident.version = 0; > + cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident)); > + cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa); Could save intializing cmd above and do it here where all the data is available. cmd = (union pds_core_adminq_cmd) { .fwctl_ident = { .opcode = ... etc. Up to you. } > + > + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0); > + if (err) { > + dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa); > + dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n", > + cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err); > + return err; > + } > + > + pdsfc->ident = ident; > + pdsfc->ident_pa = ident_pa; I guess it will become clear in later patches, but I'm not immediately sure why it makes sense to keep a copy of ident and the dma mappings live etc. Does it change at runtime? > + > + dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n", > + ident->version, ident->max_req_sz, ident->max_resp_sz, > + ident->max_req_sg_elems, ident->max_resp_sg_elems); > + > + return 0; > +} > +static int pdsfc_probe(struct auxiliary_device *adev, > + const struct auxiliary_device_id *id) > +{ > + struct pdsfc_dev *pdsfc __free(pdsfc_dev); Convention for these is to put the constructor and destructor definitions on one line. I'm too lazy to find the email from Linus where he specified this but Dan did add docs to cleanup.h. https://elixir.bootlin.com/linux/v6.14-rc2/source/include/linux/cleanup.h#L129 is referring to setting this to NULL, which is minimum that should be done as future code changes might mean there is a failure path between declaration and use. Anyhow, it argues in favor of inline as shown below. > + struct pds_auxiliary_dev *padev; > + struct device *dev = &adev->dev; > + int err = 0; Set in all paths where it is used so no need to set it here. > + > + padev = container_of(adev, struct pds_auxiliary_dev, aux_dev); struct pdsfc_dev *pdsfc __free(pdsfc_dev) = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops, > + struct pdsfc_dev, fwctl); > + pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops, > + struct pdsfc_dev, fwctl); > + if (!pdsfc) { > + dev_err(dev, "Failed to allocate fwctl device struct\n"); > + return -ENOMEM; > + } > + pdsfc->padev = padev; > + > + err = pdsfc_identify(pdsfc); > + if (err) { > + dev_err(dev, "Failed to identify device, err %d\n", err); > + return err; Perhaps use return dev_err_probe() just to get the pretty printing. Note though that it won't print for enomem cases. > + } > + > + err = fwctl_register(&pdsfc->fwctl); > + if (err) { > + dev_err(dev, "Failed to register device, err %d\n", err); > + return err; > + } > + > + auxiliary_set_drvdata(adev, no_free_ptr(pdsfc)); > + > + return 0; > + > +free_ident: Nothing goes here. Which is good as missing __free magic with gotos is a recipe for pain. > + pdsfc_free_ident(pdsfc); > + return err; > +} > + > +static void pdsfc_remove(struct auxiliary_device *adev) > +{ > + struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev); > + > + fwctl_unregister(&pdsfc->fwctl); > + pdsfc_free_ident(pdsfc); > +} > diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h > index 4b4e9a98b37b..7fc353b63353 100644 > --- a/include/linux/pds/pds_adminq.h > +++ b/include/linux/pds/pds_adminq.h > @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd { > u8 status; > }; > > +enum pds_fwctl_cmd_opcode { > + PDS_FWCTL_CMD_IDENT = 70, > +}; > + > +/** > + * struct pds_fwctl_cmd - Firmware control command structure > + * @opcode: Opcode > + * @rsvd: Word boundary padding > + * @ep: Endpoint identifier. > + * @op: Operation identifier. > + */ > +struct pds_fwctl_cmd { > + u8 opcode; > + u8 rsvd[3]; > + __le32 ep; > + __le32 op; > +} __packed; None of these actually need to be packed given explicit padding to natural alignment of all fields. Arguably it does no harm though so up to you. > + > +/** > + * struct pds_fwctl_comp - Firmware control completion structure > + * @status: Status of the firmware control operation > + * @rsvd: Word boundary padding > + * @comp_index: Completion index in little-endian format > + * @rsvd2: Word boundary padding > + * @color: Color bit indicating the state of the completion > + */ > +struct pds_fwctl_comp { > + u8 status; > + u8 rsvd; > + __le16 comp_index; > + u8 rsvd2[11]; > + u8 color; > +} __packed;
On 2/11/25 4:48 PM, Shannon Nelson wrote: > Initial files for adding a new fwctl driver for the AMD/Pensando PDS > devices. This sets up a simple auxiliary_bus driver that registers > with fwctl subsystem. It expects that a pds_core device has set up > the auxiliary_device pds_core.fwctl > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > --- > MAINTAINERS | 7 ++ > drivers/fwctl/Kconfig | 10 ++ > drivers/fwctl/Makefile | 1 + > drivers/fwctl/pds/Makefile | 4 + > drivers/fwctl/pds/main.c | 195 +++++++++++++++++++++++++++++++++ > include/linux/pds/pds_adminq.h | 77 +++++++++++++ > include/uapi/fwctl/fwctl.h | 1 + > include/uapi/fwctl/pds.h | 27 +++++ > 8 files changed, 322 insertions(+) > create mode 100644 drivers/fwctl/pds/Makefile > create mode 100644 drivers/fwctl/pds/main.c > create mode 100644 include/uapi/fwctl/pds.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 413ab79bf2f4..123f8a9c0b26 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9602,6 +9602,13 @@ T: git git://linuxtv.org/media.git > F: Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml > F: drivers/media/i2c/gc2145.c > > +FWCTL PDS DRIVER > +M: Brett Creeley <brett.creeley@amd.com> > +R: Shannon Nelson <shannon.nelson@amd.com> > +L: linux-kernel@vger.kernel.org > +S: Maintained > +F: drivers/fwctl/pds/ > + > GATEWORKS SYSTEM CONTROLLER (GSC) DRIVER > M: Tim Harvey <tharvey@gateworks.com> > S: Maintained > diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig > index 0a542a247303..df87ce5bd8aa 100644 > --- a/drivers/fwctl/Kconfig > +++ b/drivers/fwctl/Kconfig > @@ -28,5 +28,15 @@ config FWCTL_MLX5 > This will allow configuration and debug tools to work out of the box on > mainstream kernel. > > + If you don't know what to do here, say N. > + > +config FWCTL_PDS > + tristate "AMD/Pensando pds fwctl driver" > + depends on PDS_CORE > + help > + The pds_fwctl driver provides an fwctl interface for a user process > + to access the debug and configuration information of the AMD/Pensando > + DSC hardware family. > + > If you don't know what to do here, say N. > endif > diff --git a/drivers/fwctl/Makefile b/drivers/fwctl/Makefile > index 5fb289243286..692e4b8d7beb 100644 > --- a/drivers/fwctl/Makefile > +++ b/drivers/fwctl/Makefile > @@ -2,5 +2,6 @@ > obj-$(CONFIG_FWCTL) += fwctl.o > obj-$(CONFIG_FWCTL_BNXT) += bnxt/ > obj-$(CONFIG_FWCTL_MLX5) += mlx5/ > +obj-$(CONFIG_FWCTL_PDS) += pds/ > > fwctl-y += main.o > diff --git a/drivers/fwctl/pds/Makefile b/drivers/fwctl/pds/Makefile > new file mode 100644 > index 000000000000..c14cba128e3b > --- /dev/null > +++ b/drivers/fwctl/pds/Makefile > @@ -0,0 +1,4 @@ > +# SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause > +obj-$(CONFIG_FWCTL_PDS) += pds_fwctl.o > + > +pds_fwctl-y += main.o > diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c > new file mode 100644 > index 000000000000..24979fe0deea > --- /dev/null > +++ b/drivers/fwctl/pds/main.c > @@ -0,0 +1,195 @@ > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause > +/* Copyright(c) Advanced Micro Devices, Inc */ > + > +#include <linux/module.h> > +#include <linux/auxiliary_bus.h> > +#include <linux/pci.h> > +#include <linux/vmalloc.h> > + > +#include <uapi/fwctl/fwctl.h> > +#include <uapi/fwctl/pds.h> > +#include <linux/fwctl.h> > + > +#include <linux/pds/pds_common.h> > +#include <linux/pds/pds_core_if.h> > +#include <linux/pds/pds_adminq.h> > +#include <linux/pds/pds_auxbus.h> > + > +struct pdsfc_uctx { > + struct fwctl_uctx uctx; > + u32 uctx_caps; > + u32 uctx_uid; > +}; > + > +struct pdsfc_dev { > + struct fwctl_device fwctl; > + struct pds_auxiliary_dev *padev; > + struct pdsc *pdsc; > + u32 caps; > + dma_addr_t ident_pa; > + struct pds_fwctl_ident *ident; > +}; > +DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl)); > + > +static int pdsfc_open_uctx(struct fwctl_uctx *uctx) > +{ > + struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl); > + struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx); > + struct device *dev = &uctx->fwctl->dev; > + > + dev_dbg(dev, "%s: caps = 0x%04x\n", __func__, pdsfc->caps); > + pdsfc_uctx->uctx_caps = pdsfc->caps; > + > + return 0; > +} > + > +static void pdsfc_close_uctx(struct fwctl_uctx *uctx) > +{ > +} > + > +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length) > +{ > + struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx); > + struct fwctl_info_pds *info; > + > + info = kzalloc(sizeof(*info), GFP_KERNEL); > + if (!info) > + return ERR_PTR(-ENOMEM); > + > + info->uctx_caps = pdsfc_uctx->uctx_caps; > + > + return info; > +} > + > +static void pdsfc_free_ident(struct pdsfc_dev *pdsfc) > +{ > + struct device *dev = &pdsfc->fwctl.dev; > + > + if (pdsfc->ident) { > + dma_free_coherent(dev, sizeof(*pdsfc->ident), > + pdsfc->ident, pdsfc->ident_pa); > + pdsfc->ident = NULL; > + pdsfc->ident_pa = DMA_MAPPING_ERROR; > + } > +} > + > +static int pdsfc_identify(struct pdsfc_dev *pdsfc) > +{ > + struct device *dev = &pdsfc->fwctl.dev; > + union pds_core_adminq_comp comp = {0}; > + union pds_core_adminq_cmd cmd = {0}; > + struct pds_fwctl_ident *ident; > + dma_addr_t ident_pa; > + int err = 0; > + > + ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL); > + err = dma_mapping_error(dev->parent, ident_pa); > + if (err) { > + dev_err(dev, "Failed to map ident\n"); > + return err; > + } > + > + cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT; > + cmd.fwctl_ident.version = 0; > + cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident)); > + cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa); > + > + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0); > + if (err) { > + dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa); > + dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n", > + cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err); > + return err; > + } > + > + pdsfc->ident = ident; > + pdsfc->ident_pa = ident_pa; > + > + dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n", > + ident->version, ident->max_req_sz, ident->max_resp_sz, > + ident->max_req_sg_elems, ident->max_resp_sg_elems); > + > + return 0; > +} > + > +static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope, > + void *in, size_t in_len, size_t *out_len) > +{ > + return NULL; > +} > + > +static const struct fwctl_ops pdsfc_ops = { > + .device_type = FWCTL_DEVICE_TYPE_PDS, > + .uctx_size = sizeof(struct pdsfc_uctx), > + .open_uctx = pdsfc_open_uctx, > + .close_uctx = pdsfc_close_uctx, > + .info = pdsfc_info, > + .fw_rpc = pdsfc_fw_rpc, > +}; > + > +static int pdsfc_probe(struct auxiliary_device *adev, > + const struct auxiliary_device_id *id) > +{ > + struct pdsfc_dev *pdsfc __free(pdsfc_dev); > + struct pds_auxiliary_dev *padev; > + struct device *dev = &adev->dev; > + int err = 0; > + > + padev = container_of(adev, struct pds_auxiliary_dev, aux_dev); > + pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops, > + struct pdsfc_dev, fwctl); The suggested formatting of using scope-based cleanup is to just declare the variable inline as you need it rather than split the cleanup vs the allocation. struct pdsfc_dev *pdsfc __free(pdsfc_dev) = fwctl_alloc_device(...); > + if (!pdsfc) { > + dev_err(dev, "Failed to allocate fwctl device struct\n"); > + return -ENOMEM; > + } > + pdsfc->padev = padev; > + > + err = pdsfc_identify(pdsfc); > + if (err) { > + dev_err(dev, "Failed to identify device, err %d\n", err);> + return err; > + } > + > + err = fwctl_register(&pdsfc->fwctl); > + if (err) { > + dev_err(dev, "Failed to register device, err %d\n", err); Missing freeing of the 'ident' from dma_alloc_coherent. Although mixing gotos and __free() would get really messy in a hurry. I suggest you setup pdsfc_identify() like an allocation function with returning 'struct pds_fwctl_ident *' so you can utilize a custom __free() to free up the dma memory. and then you can do: pdsfc->ident = no_free_ptr(ident); pdsfc->ident_pa = ident_pa; /* ident_pa passed in as a ptr to pdsfc_identify() for write back */ > + return err; > + } > + > + auxiliary_set_drvdata(adev, no_free_ptr(pdsfc)); > + > + return 0; > + > +free_ident: nothing goes here. DJ > + pdsfc_free_ident(pdsfc); > + return err; > +} > + > +static void pdsfc_remove(struct auxiliary_device *adev) > +{ > + struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev); > + > + fwctl_unregister(&pdsfc->fwctl); > + pdsfc_free_ident(pdsfc); > +} > + > +static const struct auxiliary_device_id pdsfc_id_table[] = { > + {.name = PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_FWCTL_STR }, > + {} > +}; > +MODULE_DEVICE_TABLE(auxiliary, pdsfc_id_table); > + > +static struct auxiliary_driver pdsfc_driver = { > + .name = "pds_fwctl", > + .probe = pdsfc_probe, > + .remove = pdsfc_remove, > + .id_table = pdsfc_id_table, > +}; > + > +module_auxiliary_driver(pdsfc_driver); > + > +MODULE_IMPORT_NS(FWCTL); > +MODULE_DESCRIPTION("pds fwctl driver"); > +MODULE_AUTHOR("Shannon Nelson <shannon.nelson@amd.com>"); > +MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>"); > +MODULE_LICENSE("Dual BSD/GPL"); > diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h > index 4b4e9a98b37b..7fc353b63353 100644 > --- a/include/linux/pds/pds_adminq.h > +++ b/include/linux/pds/pds_adminq.h > @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd { > u8 status; > }; > > +enum pds_fwctl_cmd_opcode { > + PDS_FWCTL_CMD_IDENT = 70, > +}; > + > +/** > + * struct pds_fwctl_cmd - Firmware control command structure > + * @opcode: Opcode > + * @rsvd: Word boundary padding > + * @ep: Endpoint identifier. > + * @op: Operation identifier. > + */ > +struct pds_fwctl_cmd { > + u8 opcode; > + u8 rsvd[3]; > + __le32 ep; > + __le32 op; > +} __packed; > + > +/** > + * struct pds_fwctl_comp - Firmware control completion structure > + * @status: Status of the firmware control operation > + * @rsvd: Word boundary padding > + * @comp_index: Completion index in little-endian format > + * @rsvd2: Word boundary padding > + * @color: Color bit indicating the state of the completion > + */ > +struct pds_fwctl_comp { > + u8 status; > + u8 rsvd; > + __le16 comp_index; > + u8 rsvd2[11]; > + u8 color; > +} __packed; > + > +/** > + * struct pds_fwctl_ident_cmd - Firmware control identification command structure > + * @opcode: Operation code for the command > + * @rsvd: Word boundary padding > + * @version: Interface version > + * @rsvd2: Word boundary padding > + * @len: Length of the identification data > + * @ident_pa: Physical address of the identification data > + */ > +struct pds_fwctl_ident_cmd { > + u8 opcode; > + u8 rsvd; > + u8 version; > + u8 rsvd2; > + __le32 len; > + __le64 ident_pa; > +} __packed; > + > +/** > + * struct pds_fwctl_ident - Firmware control identification structure > + * @features: Supported features > + * @version: Interface version > + * @rsvd: Word boundary padding > + * @max_req_sz: Maximum request size > + * @max_resp_sz: Maximum response size > + * @max_req_sg_elems: Maximum number of request SGs > + * @max_resp_sg_elems: Maximum number of response SGs > + */ > +struct pds_fwctl_ident { > + __le64 features; > + u8 version; > + u8 rsvd[3]; > + __le32 max_req_sz; > + __le32 max_resp_sz; > + u8 max_req_sg_elems; > + u8 max_resp_sg_elems; > +} __packed; > + > union pds_core_adminq_cmd { > u8 opcode; > u8 bytes[64]; > @@ -1216,6 +1288,9 @@ union pds_core_adminq_cmd { > struct pds_lm_dirty_enable_cmd lm_dirty_enable; > struct pds_lm_dirty_disable_cmd lm_dirty_disable; > struct pds_lm_dirty_seq_ack_cmd lm_dirty_seq_ack; > + > + struct pds_fwctl_cmd fwctl; > + struct pds_fwctl_ident_cmd fwctl_ident; > }; > > union pds_core_adminq_comp { > @@ -1243,6 +1318,8 @@ union pds_core_adminq_comp { > > struct pds_lm_state_size_comp lm_state_size; > struct pds_lm_dirty_status_comp lm_dirty_status; > + > + struct pds_fwctl_comp fwctl; > }; > > #ifndef __CHECKER__ > diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h > index 518f054f02d2..a884e9f6dc2c 100644 > --- a/include/uapi/fwctl/fwctl.h > +++ b/include/uapi/fwctl/fwctl.h > @@ -44,6 +44,7 @@ enum fwctl_device_type { > FWCTL_DEVICE_TYPE_ERROR = 0, > FWCTL_DEVICE_TYPE_MLX5 = 1, > FWCTL_DEVICE_TYPE_BNXT = 3, > + FWCTL_DEVICE_TYPE_PDS = 4, > }; > > /** > diff --git a/include/uapi/fwctl/pds.h b/include/uapi/fwctl/pds.h > new file mode 100644 > index 000000000000..a01b032cbdb1 > --- /dev/null > +++ b/include/uapi/fwctl/pds.h > @@ -0,0 +1,27 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > +/* Copyright(c) Advanced Micro Devices, Inc */ > + > +/* > + * fwctl interface info for pds_fwctl > + */ > + > +#ifndef _UAPI_FWCTL_PDS_H_ > +#define _UAPI_FWCTL_PDS_H_ > + > +#include <linux/types.h> > + > +/* > + * struct fwctl_info_pds > + * > + * Return basic information about the FW interface available. > + */ > +struct fwctl_info_pds { > + __u32 uid; > + __u32 uctx_caps; > +}; > + > +enum pds_fwctl_capabilities { > + PDS_FWCTL_QUERY_CAP = 0, > + PDS_FWCTL_SEND_CAP, > +}; > +#endif /* _UAPI_FWCTL_PDS_H_ */
On 2/12/2025 4:22 AM, Jonathan Cameron wrote: > On Tue, 11 Feb 2025 15:48:52 -0800 > Shannon Nelson <shannon.nelson@amd.com> wrote: > >> Initial files for adding a new fwctl driver for the AMD/Pensando PDS >> devices. This sets up a simple auxiliary_bus driver that registers >> with fwctl subsystem. It expects that a pds_core device has set up >> the auxiliary_device pds_core.fwctl >> >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >> --- > Hi Shannon, > A few comments inline > > Jonathan > > > >> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c >> new file mode 100644 >> index 000000000000..24979fe0deea >> --- /dev/null >> +++ b/drivers/fwctl/pds/main.c >> @@ -0,0 +1,195 @@ > >> + >> +static int pdsfc_identify(struct pdsfc_dev *pdsfc) >> +{ >> + struct device *dev = &pdsfc->fwctl.dev; >> + union pds_core_adminq_comp comp = {0}; >> + union pds_core_adminq_cmd cmd = {0}; >> + struct pds_fwctl_ident *ident; >> + dma_addr_t ident_pa; >> + int err = 0; >> + >> + ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL); >> + err = dma_mapping_error(dev->parent, ident_pa); >> + if (err) { >> + dev_err(dev, "Failed to map ident\n"); >> + return err; >> + } >> + >> + cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT; >> + cmd.fwctl_ident.version = 0; >> + cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident)); >> + cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa); > > Could save intializing cmd above and do it here where all > the data is available. > > cmd = (union pds_core_adminq_cmd) { > .fwctl_ident = { > .opcode = ... > etc. Up to you. > > > } Yeah, there are a lot of ways to do this. In a lot of the ionic code we do a bunch of the init in the declaration and add to the more dynamic field values later in the function. Let me think on this a little... >> + >> + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0); >> + if (err) { >> + dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa); >> + dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n", >> + cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err); >> + return err; >> + } >> + >> + pdsfc->ident = ident; >> + pdsfc->ident_pa = ident_pa; > > I guess it will become clear in later patches, but I'm not immediately sure why > it makes sense to keep a copy of ident and the dma mappings live etc. > Does it change at runtime? No, actually this doesn't change at runtime, we could copy it into a local storage and drop the DMA mapping, which will relieve us of the need to remember to clean it up later. > > >> + >> + dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n", >> + ident->version, ident->max_req_sz, ident->max_resp_sz, >> + ident->max_req_sg_elems, ident->max_resp_sg_elems); >> + >> + return 0; >> +} > >> +static int pdsfc_probe(struct auxiliary_device *adev, >> + const struct auxiliary_device_id *id) >> +{ >> + struct pdsfc_dev *pdsfc __free(pdsfc_dev); > Convention for these is to put the constructor and destructor definitions > on one line. I'm too lazy to find the email from Linus where he > specified this but Dan did add docs to cleanup.h. > https://elixir.bootlin.com/linux/v6.14-rc2/source/include/linux/cleanup.h#L129 > is referring to setting this to NULL, which is minimum that should be done > as future code changes might mean there is a failure path between > declaration and use. Anyhow, it argues in favor of inline as shown > below. Obviously we're still new to this pattern - I'll look at fixing these things up. > > >> + struct pds_auxiliary_dev *padev; >> + struct device *dev = &adev->dev; >> + int err = 0; > Set in all paths where it is used so no need to set it here. Got it. > >> + >> + padev = container_of(adev, struct pds_auxiliary_dev, aux_dev); > struct pdsfc_dev *pdsfc __free(pdsfc_dev) = > fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops, >> + struct pdsfc_dev, fwctl); >> + pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops, >> + struct pdsfc_dev, fwctl); >> + if (!pdsfc) { >> + dev_err(dev, "Failed to allocate fwctl device struct\n"); >> + return -ENOMEM; >> + } >> + pdsfc->padev = padev; >> + >> + err = pdsfc_identify(pdsfc); >> + if (err) { >> + dev_err(dev, "Failed to identify device, err %d\n", err); >> + return err; > > Perhaps use return dev_err_probe() just to get the pretty printing. > Note though that it won't print for enomem cases. Sure > >> + } >> + >> + err = fwctl_register(&pdsfc->fwctl); >> + if (err) { >> + dev_err(dev, "Failed to register device, err %d\n", err); >> + return err; >> + } >> + >> + auxiliary_set_drvdata(adev, no_free_ptr(pdsfc)); >> + >> + return 0; >> + >> +free_ident: > > Nothing goes here. Which is good as missing __free magic with gotos > is a recipe for pain. The above ident change will remove the need for this. > >> + pdsfc_free_ident(pdsfc); >> + return err; >> +} >> + >> +static void pdsfc_remove(struct auxiliary_device *adev) >> +{ >> + struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev); >> + >> + fwctl_unregister(&pdsfc->fwctl); >> + pdsfc_free_ident(pdsfc); >> +} > >> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h >> index 4b4e9a98b37b..7fc353b63353 100644 >> --- a/include/linux/pds/pds_adminq.h >> +++ b/include/linux/pds/pds_adminq.h >> @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd { >> u8 status; >> }; >> >> +enum pds_fwctl_cmd_opcode { >> + PDS_FWCTL_CMD_IDENT = 70, >> +}; >> + >> +/** >> + * struct pds_fwctl_cmd - Firmware control command structure >> + * @opcode: Opcode >> + * @rsvd: Word boundary padding >> + * @ep: Endpoint identifier. >> + * @op: Operation identifier. >> + */ >> +struct pds_fwctl_cmd { >> + u8 opcode; >> + u8 rsvd[3]; >> + __le32 ep; >> + __le32 op; >> +} __packed; > None of these actually need to be packed given explicit padding to > natural alignment of all fields. Arguably it does no harm though > so up to you. Old belt-and-suspenders habits... Since this is the common style on the rest of the interface definitions, I'd prefer to keep it for now. sln > >> + >> +/** >> + * struct pds_fwctl_comp - Firmware control completion structure >> + * @status: Status of the firmware control operation >> + * @rsvd: Word boundary padding >> + * @comp_index: Completion index in little-endian format >> + * @rsvd2: Word boundary padding >> + * @color: Color bit indicating the state of the completion >> + */ >> +struct pds_fwctl_comp { >> + u8 status; >> + u8 rsvd; >> + __le16 comp_index; >> + u8 rsvd2[11]; >> + u8 color; >> +} __packed; >
On 2/12/2025 3:26 PM, Dave Jiang wrote: > > On 2/11/25 4:48 PM, Shannon Nelson wrote: >> Initial files for adding a new fwctl driver for the AMD/Pensando PDS >> devices. This sets up a simple auxiliary_bus driver that registers >> with fwctl subsystem. It expects that a pds_core device has set up >> the auxiliary_device pds_core.fwctl >> >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >> --- >> MAINTAINERS | 7 ++ >> drivers/fwctl/Kconfig | 10 ++ >> drivers/fwctl/Makefile | 1 + >> drivers/fwctl/pds/Makefile | 4 + >> drivers/fwctl/pds/main.c | 195 +++++++++++++++++++++++++++++++++ >> include/linux/pds/pds_adminq.h | 77 +++++++++++++ >> include/uapi/fwctl/fwctl.h | 1 + >> include/uapi/fwctl/pds.h | 27 +++++ >> 8 files changed, 322 insertions(+) >> create mode 100644 drivers/fwctl/pds/Makefile >> create mode 100644 drivers/fwctl/pds/main.c >> create mode 100644 include/uapi/fwctl/pds.h >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 413ab79bf2f4..123f8a9c0b26 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -9602,6 +9602,13 @@ T: git git://linuxtv.org/media.git >> F: Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml >> F: drivers/media/i2c/gc2145.c >> >> +FWCTL PDS DRIVER >> +M: Brett Creeley <brett.creeley@amd.com> >> +R: Shannon Nelson <shannon.nelson@amd.com> >> +L: linux-kernel@vger.kernel.org >> +S: Maintained >> +F: drivers/fwctl/pds/ >> + >> GATEWORKS SYSTEM CONTROLLER (GSC) DRIVER >> M: Tim Harvey <tharvey@gateworks.com> >> S: Maintained >> diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig >> index 0a542a247303..df87ce5bd8aa 100644 >> --- a/drivers/fwctl/Kconfig >> +++ b/drivers/fwctl/Kconfig >> @@ -28,5 +28,15 @@ config FWCTL_MLX5 >> This will allow configuration and debug tools to work out of the box on >> mainstream kernel. >> >> + If you don't know what to do here, say N. >> + >> +config FWCTL_PDS >> + tristate "AMD/Pensando pds fwctl driver" >> + depends on PDS_CORE >> + help >> + The pds_fwctl driver provides an fwctl interface for a user process >> + to access the debug and configuration information of the AMD/Pensando >> + DSC hardware family. >> + >> If you don't know what to do here, say N. >> endif >> diff --git a/drivers/fwctl/Makefile b/drivers/fwctl/Makefile >> index 5fb289243286..692e4b8d7beb 100644 >> --- a/drivers/fwctl/Makefile >> +++ b/drivers/fwctl/Makefile >> @@ -2,5 +2,6 @@ >> obj-$(CONFIG_FWCTL) += fwctl.o >> obj-$(CONFIG_FWCTL_BNXT) += bnxt/ >> obj-$(CONFIG_FWCTL_MLX5) += mlx5/ >> +obj-$(CONFIG_FWCTL_PDS) += pds/ >> >> fwctl-y += main.o >> diff --git a/drivers/fwctl/pds/Makefile b/drivers/fwctl/pds/Makefile >> new file mode 100644 >> index 000000000000..c14cba128e3b >> --- /dev/null >> +++ b/drivers/fwctl/pds/Makefile >> @@ -0,0 +1,4 @@ >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause >> +obj-$(CONFIG_FWCTL_PDS) += pds_fwctl.o >> + >> +pds_fwctl-y += main.o >> diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c >> new file mode 100644 >> index 000000000000..24979fe0deea >> --- /dev/null >> +++ b/drivers/fwctl/pds/main.c >> @@ -0,0 +1,195 @@ >> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause >> +/* Copyright(c) Advanced Micro Devices, Inc */ >> + >> +#include <linux/module.h> >> +#include <linux/auxiliary_bus.h> >> +#include <linux/pci.h> >> +#include <linux/vmalloc.h> >> + >> +#include <uapi/fwctl/fwctl.h> >> +#include <uapi/fwctl/pds.h> >> +#include <linux/fwctl.h> >> + >> +#include <linux/pds/pds_common.h> >> +#include <linux/pds/pds_core_if.h> >> +#include <linux/pds/pds_adminq.h> >> +#include <linux/pds/pds_auxbus.h> >> + >> +struct pdsfc_uctx { >> + struct fwctl_uctx uctx; >> + u32 uctx_caps; >> + u32 uctx_uid; >> +}; >> + >> +struct pdsfc_dev { >> + struct fwctl_device fwctl; >> + struct pds_auxiliary_dev *padev; >> + struct pdsc *pdsc; >> + u32 caps; >> + dma_addr_t ident_pa; >> + struct pds_fwctl_ident *ident; >> +}; >> +DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl)); >> + >> +static int pdsfc_open_uctx(struct fwctl_uctx *uctx) >> +{ >> + struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl); >> + struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx); >> + struct device *dev = &uctx->fwctl->dev; >> + >> + dev_dbg(dev, "%s: caps = 0x%04x\n", __func__, pdsfc->caps); >> + pdsfc_uctx->uctx_caps = pdsfc->caps; >> + >> + return 0; >> +} >> + >> +static void pdsfc_close_uctx(struct fwctl_uctx *uctx) >> +{ >> +} >> + >> +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length) >> +{ >> + struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx); >> + struct fwctl_info_pds *info; >> + >> + info = kzalloc(sizeof(*info), GFP_KERNEL); >> + if (!info) >> + return ERR_PTR(-ENOMEM); >> + >> + info->uctx_caps = pdsfc_uctx->uctx_caps; >> + >> + return info; >> +} >> + >> +static void pdsfc_free_ident(struct pdsfc_dev *pdsfc) >> +{ >> + struct device *dev = &pdsfc->fwctl.dev; >> + >> + if (pdsfc->ident) { >> + dma_free_coherent(dev, sizeof(*pdsfc->ident), >> + pdsfc->ident, pdsfc->ident_pa); >> + pdsfc->ident = NULL; >> + pdsfc->ident_pa = DMA_MAPPING_ERROR; >> + } >> +} >> + >> +static int pdsfc_identify(struct pdsfc_dev *pdsfc) >> +{ >> + struct device *dev = &pdsfc->fwctl.dev; >> + union pds_core_adminq_comp comp = {0}; >> + union pds_core_adminq_cmd cmd = {0}; >> + struct pds_fwctl_ident *ident; >> + dma_addr_t ident_pa; >> + int err = 0; >> + >> + ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL); >> + err = dma_mapping_error(dev->parent, ident_pa); >> + if (err) { >> + dev_err(dev, "Failed to map ident\n"); >> + return err; >> + } >> + >> + cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT; >> + cmd.fwctl_ident.version = 0; >> + cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident)); >> + cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa); >> + >> + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0); >> + if (err) { >> + dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa); >> + dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n", >> + cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err); >> + return err; >> + } >> + >> + pdsfc->ident = ident; >> + pdsfc->ident_pa = ident_pa; >> + >> + dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n", >> + ident->version, ident->max_req_sz, ident->max_resp_sz, >> + ident->max_req_sg_elems, ident->max_resp_sg_elems); >> + >> + return 0; >> +} >> + >> +static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope, >> + void *in, size_t in_len, size_t *out_len) >> +{ >> + return NULL; >> +} >> + >> +static const struct fwctl_ops pdsfc_ops = { >> + .device_type = FWCTL_DEVICE_TYPE_PDS, >> + .uctx_size = sizeof(struct pdsfc_uctx), >> + .open_uctx = pdsfc_open_uctx, >> + .close_uctx = pdsfc_close_uctx, >> + .info = pdsfc_info, >> + .fw_rpc = pdsfc_fw_rpc, >> +}; >> + >> +static int pdsfc_probe(struct auxiliary_device *adev, >> + const struct auxiliary_device_id *id) >> +{ >> + struct pdsfc_dev *pdsfc __free(pdsfc_dev); >> + struct pds_auxiliary_dev *padev; >> + struct device *dev = &adev->dev; >> + int err = 0; >> + >> + padev = container_of(adev, struct pds_auxiliary_dev, aux_dev); >> + pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops, >> + struct pdsfc_dev, fwctl); > > The suggested formatting of using scope-based cleanup is to just declare the variable inline as you need it rather than split the cleanup vs the allocation. I'm old - I have old patterns of distain for mid-function declarations... I'll try to do better :-) > > struct pdsfc_dev *pdsfc __free(pdsfc_dev) = > fwctl_alloc_device(...); > >> + if (!pdsfc) { >> + dev_err(dev, "Failed to allocate fwctl device struct\n"); >> + return -ENOMEM; >> + } >> + pdsfc->padev = padev; >> + >> + err = pdsfc_identify(pdsfc); >> + if (err) { >> + dev_err(dev, "Failed to identify device, err %d\n", err);> + return err; >> + } >> + >> + err = fwctl_register(&pdsfc->fwctl); >> + if (err) { >> + dev_err(dev, "Failed to register device, err %d\n", err); > > Missing freeing of the 'ident' from dma_alloc_coherent. Although mixing gotos and __free() would get really messy in a hurry. I suggest you setup pdsfc_identify() like an allocation function with returning 'struct pds_fwctl_ident *' so you can utilize a custom __free() to free up the dma memory. and then you can do: > pdsfc->ident = no_free_ptr(ident); > pdsfc->ident_pa = ident_pa; /* ident_pa passed in as a ptr to pdsfc_identify() for write back */ As discussed with Jonathan, this will get reworked to not need to keep the DMA mapping around. > >> + return err; >> + } >> + >> + auxiliary_set_drvdata(adev, no_free_ptr(pdsfc)); >> + >> + return 0; >> + >> +free_ident: > > nothing goes here. Thanks! sln > > DJ > >> + pdsfc_free_ident(pdsfc); >> + return err; >> +} >> + >> +static void pdsfc_remove(struct auxiliary_device *adev) >> +{ >> + struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev); >> + >> + fwctl_unregister(&pdsfc->fwctl); >> + pdsfc_free_ident(pdsfc); >> +} >> + >> +static const struct auxiliary_device_id pdsfc_id_table[] = { >> + {.name = PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_FWCTL_STR }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(auxiliary, pdsfc_id_table); >> + >> +static struct auxiliary_driver pdsfc_driver = { >> + .name = "pds_fwctl", >> + .probe = pdsfc_probe, >> + .remove = pdsfc_remove, >> + .id_table = pdsfc_id_table, >> +}; >> + >> +module_auxiliary_driver(pdsfc_driver); >> + >> +MODULE_IMPORT_NS(FWCTL); >> +MODULE_DESCRIPTION("pds fwctl driver"); >> +MODULE_AUTHOR("Shannon Nelson <shannon.nelson@amd.com>"); >> +MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>"); >> +MODULE_LICENSE("Dual BSD/GPL"); >> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h >> index 4b4e9a98b37b..7fc353b63353 100644 >> --- a/include/linux/pds/pds_adminq.h >> +++ b/include/linux/pds/pds_adminq.h >> @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd { >> u8 status; >> }; >> >> +enum pds_fwctl_cmd_opcode { >> + PDS_FWCTL_CMD_IDENT = 70, >> +}; >> + >> +/** >> + * struct pds_fwctl_cmd - Firmware control command structure >> + * @opcode: Opcode >> + * @rsvd: Word boundary padding >> + * @ep: Endpoint identifier. >> + * @op: Operation identifier. >> + */ >> +struct pds_fwctl_cmd { >> + u8 opcode; >> + u8 rsvd[3]; >> + __le32 ep; >> + __le32 op; >> +} __packed; >> + >> +/** >> + * struct pds_fwctl_comp - Firmware control completion structure >> + * @status: Status of the firmware control operation >> + * @rsvd: Word boundary padding >> + * @comp_index: Completion index in little-endian format >> + * @rsvd2: Word boundary padding >> + * @color: Color bit indicating the state of the completion >> + */ >> +struct pds_fwctl_comp { >> + u8 status; >> + u8 rsvd; >> + __le16 comp_index; >> + u8 rsvd2[11]; >> + u8 color; >> +} __packed; >> + >> +/** >> + * struct pds_fwctl_ident_cmd - Firmware control identification command structure >> + * @opcode: Operation code for the command >> + * @rsvd: Word boundary padding >> + * @version: Interface version >> + * @rsvd2: Word boundary padding >> + * @len: Length of the identification data >> + * @ident_pa: Physical address of the identification data >> + */ >> +struct pds_fwctl_ident_cmd { >> + u8 opcode; >> + u8 rsvd; >> + u8 version; >> + u8 rsvd2; >> + __le32 len; >> + __le64 ident_pa; >> +} __packed; >> + >> +/** >> + * struct pds_fwctl_ident - Firmware control identification structure >> + * @features: Supported features >> + * @version: Interface version >> + * @rsvd: Word boundary padding >> + * @max_req_sz: Maximum request size >> + * @max_resp_sz: Maximum response size >> + * @max_req_sg_elems: Maximum number of request SGs >> + * @max_resp_sg_elems: Maximum number of response SGs >> + */ >> +struct pds_fwctl_ident { >> + __le64 features; >> + u8 version; >> + u8 rsvd[3]; >> + __le32 max_req_sz; >> + __le32 max_resp_sz; >> + u8 max_req_sg_elems; >> + u8 max_resp_sg_elems; >> +} __packed; >> + >> union pds_core_adminq_cmd { >> u8 opcode; >> u8 bytes[64]; >> @@ -1216,6 +1288,9 @@ union pds_core_adminq_cmd { >> struct pds_lm_dirty_enable_cmd lm_dirty_enable; >> struct pds_lm_dirty_disable_cmd lm_dirty_disable; >> struct pds_lm_dirty_seq_ack_cmd lm_dirty_seq_ack; >> + >> + struct pds_fwctl_cmd fwctl; >> + struct pds_fwctl_ident_cmd fwctl_ident; >> }; >> >> union pds_core_adminq_comp { >> @@ -1243,6 +1318,8 @@ union pds_core_adminq_comp { >> >> struct pds_lm_state_size_comp lm_state_size; >> struct pds_lm_dirty_status_comp lm_dirty_status; >> + >> + struct pds_fwctl_comp fwctl; >> }; >> >> #ifndef __CHECKER__ >> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h >> index 518f054f02d2..a884e9f6dc2c 100644 >> --- a/include/uapi/fwctl/fwctl.h >> +++ b/include/uapi/fwctl/fwctl.h >> @@ -44,6 +44,7 @@ enum fwctl_device_type { >> FWCTL_DEVICE_TYPE_ERROR = 0, >> FWCTL_DEVICE_TYPE_MLX5 = 1, >> FWCTL_DEVICE_TYPE_BNXT = 3, >> + FWCTL_DEVICE_TYPE_PDS = 4, >> }; >> >> /** >> diff --git a/include/uapi/fwctl/pds.h b/include/uapi/fwctl/pds.h >> new file mode 100644 >> index 000000000000..a01b032cbdb1 >> --- /dev/null >> +++ b/include/uapi/fwctl/pds.h >> @@ -0,0 +1,27 @@ >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ >> +/* Copyright(c) Advanced Micro Devices, Inc */ >> + >> +/* >> + * fwctl interface info for pds_fwctl >> + */ >> + >> +#ifndef _UAPI_FWCTL_PDS_H_ >> +#define _UAPI_FWCTL_PDS_H_ >> + >> +#include <linux/types.h> >> + >> +/* >> + * struct fwctl_info_pds >> + * >> + * Return basic information about the FW interface available. >> + */ >> +struct fwctl_info_pds { >> + __u32 uid; >> + __u32 uctx_caps; >> +}; >> + >> +enum pds_fwctl_capabilities { >> + PDS_FWCTL_QUERY_CAP = 0, >> + PDS_FWCTL_SEND_CAP, >> +}; >> +#endif /* _UAPI_FWCTL_PDS_H_ */ >
On Thu, Feb 13, 2025 at 03:06:14PM -0800, Nelson, Shannon wrote: > > > +/** > > > + * struct pds_fwctl_cmd - Firmware control command structure > > > + * @opcode: Opcode > > > + * @rsvd: Word boundary padding > > > + * @ep: Endpoint identifier. > > > + * @op: Operation identifier. > > > + */ > > > +struct pds_fwctl_cmd { > > > + u8 opcode; > > > + u8 rsvd[3]; > > > + __le32 ep; > > > + __le32 op; > > > +} __packed; > > None of these actually need to be packed given explicit padding to > > natural alignment of all fields. Arguably it does no harm though > > so up to you. > > Old belt-and-suspenders habits... In that case it is worth knowing that __packed also changes the assumed alignment of the struct. You can access a __packed struct at any byte. On x86 this is mostly meaningless, but on other arches it can effect code generation as the compiler will have to assume that, say, a 64 bit load is not naturally aligned and emit a more expensive sequence to load it. Which is why you occasionally see things like: __attribute__ ((packed,aligned(8))); Which says that there is no padding inside the struct, but also that the compiler can assume a guaranteed starting alignment for the memory. Jason
On Tue, Feb 11, 2025 at 03:48:52PM -0800, Shannon Nelson wrote: > Initial files for adding a new fwctl driver for the AMD/Pensando PDS > devices. This sets up a simple auxiliary_bus driver that registers > with fwctl subsystem. It expects that a pds_core device has set up > the auxiliary_device pds_core.fwctl > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > --- > MAINTAINERS | 7 ++ > drivers/fwctl/Kconfig | 10 ++ > drivers/fwctl/Makefile | 1 + > drivers/fwctl/pds/Makefile | 4 + > drivers/fwctl/pds/main.c | 195 +++++++++++++++++++++++++++++++++ > include/linux/pds/pds_adminq.h | 77 +++++++++++++ > include/uapi/fwctl/fwctl.h | 1 + > include/uapi/fwctl/pds.h | 27 +++++ > 8 files changed, 322 insertions(+) > create mode 100644 drivers/fwctl/pds/Makefile > create mode 100644 drivers/fwctl/pds/main.c > create mode 100644 include/uapi/fwctl/pds.h <...> > +static int pdsfc_open_uctx(struct fwctl_uctx *uctx) > +{ > + struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl); > + struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx); > + struct device *dev = &uctx->fwctl->dev; > + > + dev_dbg(dev, "%s: caps = 0x%04x\n", __func__, pdsfc->caps); This driver is too noisy and has too many debug/err prints. > + pdsfc_uctx->uctx_caps = pdsfc->caps; > + > + return 0; > +} > + > +static void pdsfc_close_uctx(struct fwctl_uctx *uctx) > +{ > +} > + > +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length) > +{ > + struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx); > + struct fwctl_info_pds *info; > + > + info = kzalloc(sizeof(*info), GFP_KERNEL); > + if (!info) > + return ERR_PTR(-ENOMEM); > + > + info->uctx_caps = pdsfc_uctx->uctx_caps; > + > + return info; > +} > + > +static void pdsfc_free_ident(struct pdsfc_dev *pdsfc) > +{ > + struct device *dev = &pdsfc->fwctl.dev; > + > + if (pdsfc->ident) { It is not kernel style, which is success oriented. If (!pdsfc->ident) return; However I don't know how can this happen. You shouldn't call to pdsfc_free_ident if ident wasn't set. > + dma_free_coherent(dev, sizeof(*pdsfc->ident), > + pdsfc->ident, pdsfc->ident_pa); > + pdsfc->ident = NULL; Please don't assign NULL to pointers if they are not reused. > + pdsfc->ident_pa = DMA_MAPPING_ERROR; Same. > + } > +} > + > +static int pdsfc_identify(struct pdsfc_dev *pdsfc) > +{ > + struct device *dev = &pdsfc->fwctl.dev; > + union pds_core_adminq_comp comp = {0}; > + union pds_core_adminq_cmd cmd = {0}; > + struct pds_fwctl_ident *ident; > + dma_addr_t ident_pa; > + int err = 0; There is no need to assign 0 to err. > + > + ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL); > + err = dma_mapping_error(dev->parent, ident_pa); > + if (err) { > + dev_err(dev, "Failed to map ident\n"); This is one of the examples of such extra prints. > + return err; > + } > + > + cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT; > + cmd.fwctl_ident.version = 0; How will you manage this version field? > + cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident)); > + cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa); > + > + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0); > + if (err) { > + dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa); > + dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n", > + cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err); > + return err; > + } > + > + pdsfc->ident = ident; > + pdsfc->ident_pa = ident_pa; > + > + dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n", > + ident->version, ident->max_req_sz, ident->max_resp_sz, > + ident->max_req_sg_elems, ident->max_resp_sg_elems); > + > + return 0; > +} > + > +static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope, > + void *in, size_t in_len, size_t *out_len) > +{ > + return NULL; > +} > + > +static const struct fwctl_ops pdsfc_ops = { > + .device_type = FWCTL_DEVICE_TYPE_PDS, > + .uctx_size = sizeof(struct pdsfc_uctx), > + .open_uctx = pdsfc_open_uctx, > + .close_uctx = pdsfc_close_uctx, > + .info = pdsfc_info, > + .fw_rpc = pdsfc_fw_rpc, > +}; > + > +static int pdsfc_probe(struct auxiliary_device *adev, > + const struct auxiliary_device_id *id) > +{ > + struct pdsfc_dev *pdsfc __free(pdsfc_dev); > + struct pds_auxiliary_dev *padev; > + struct device *dev = &adev->dev; > + int err = 0; > + > + padev = container_of(adev, struct pds_auxiliary_dev, aux_dev); > + pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops, > + struct pdsfc_dev, fwctl); > + if (!pdsfc) { > + dev_err(dev, "Failed to allocate fwctl device struct\n"); > + return -ENOMEM; > + } > + pdsfc->padev = padev; > + > + err = pdsfc_identify(pdsfc); > + if (err) { > + dev_err(dev, "Failed to identify device, err %d\n", err); > + return err; > + } > + > + err = fwctl_register(&pdsfc->fwctl); > + if (err) { > + dev_err(dev, "Failed to register device, err %d\n", err); > + return err; > + } > + > + auxiliary_set_drvdata(adev, no_free_ptr(pdsfc)); > + > + return 0; > + > +free_ident: > + pdsfc_free_ident(pdsfc); > + return err; > +} > + > +static void pdsfc_remove(struct auxiliary_device *adev) > +{ > + struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev); > + > + fwctl_unregister(&pdsfc->fwctl); > + pdsfc_free_ident(pdsfc); > +} > + > +static const struct auxiliary_device_id pdsfc_id_table[] = { > + {.name = PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_FWCTL_STR }, > + {} > +}; > +MODULE_DEVICE_TABLE(auxiliary, pdsfc_id_table); > + > +static struct auxiliary_driver pdsfc_driver = { > + .name = "pds_fwctl", > + .probe = pdsfc_probe, > + .remove = pdsfc_remove, > + .id_table = pdsfc_id_table, > +}; > + > +module_auxiliary_driver(pdsfc_driver); > + > +MODULE_IMPORT_NS(FWCTL); > +MODULE_DESCRIPTION("pds fwctl driver"); > +MODULE_AUTHOR("Shannon Nelson <shannon.nelson@amd.com>"); > +MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>"); > +MODULE_LICENSE("Dual BSD/GPL"); > diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h > index 4b4e9a98b37b..7fc353b63353 100644 > --- a/include/linux/pds/pds_adminq.h > +++ b/include/linux/pds/pds_adminq.h > @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd { > u8 status; > }; > > +enum pds_fwctl_cmd_opcode { > + PDS_FWCTL_CMD_IDENT = 70, Please try to avoid from vertical space alignment. It doesn't survive time and at some point you will need to reformat it, which will cause to churn and harm backporting/stable without any reason. Thanks
On 2/18/2025 11:51 AM, Leon Romanovsky wrote: > > On Tue, Feb 11, 2025 at 03:48:52PM -0800, Shannon Nelson wrote: >> Initial files for adding a new fwctl driver for the AMD/Pensando PDS >> devices. This sets up a simple auxiliary_bus driver that registers >> with fwctl subsystem. It expects that a pds_core device has set up >> the auxiliary_device pds_core.fwctl >> >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >> --- >> MAINTAINERS | 7 ++ >> drivers/fwctl/Kconfig | 10 ++ >> drivers/fwctl/Makefile | 1 + >> drivers/fwctl/pds/Makefile | 4 + >> drivers/fwctl/pds/main.c | 195 +++++++++++++++++++++++++++++++++ >> include/linux/pds/pds_adminq.h | 77 +++++++++++++ >> include/uapi/fwctl/fwctl.h | 1 + >> include/uapi/fwctl/pds.h | 27 +++++ >> 8 files changed, 322 insertions(+) >> create mode 100644 drivers/fwctl/pds/Makefile >> create mode 100644 drivers/fwctl/pds/main.c >> create mode 100644 include/uapi/fwctl/pds.h > > <...> > >> +static int pdsfc_open_uctx(struct fwctl_uctx *uctx) >> +{ >> + struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl); >> + struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx); >> + struct device *dev = &uctx->fwctl->dev; >> + >> + dev_dbg(dev, "%s: caps = 0x%04x\n", __func__, pdsfc->caps); > > This driver is too noisy and has too many debug/err prints. Yes, still being worked on, but also we used dbp prints to keep the noise to a minimum. Most of these will disappear as we move out of RFC mode. > >> + pdsfc_uctx->uctx_caps = pdsfc->caps; >> + >> + return 0; >> +} >> + >> +static void pdsfc_close_uctx(struct fwctl_uctx *uctx) >> +{ >> +} >> + >> +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length) >> +{ >> + struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx); >> + struct fwctl_info_pds *info; >> + >> + info = kzalloc(sizeof(*info), GFP_KERNEL); >> + if (!info) >> + return ERR_PTR(-ENOMEM); >> + >> + info->uctx_caps = pdsfc_uctx->uctx_caps; >> + >> + return info; >> +} >> + >> +static void pdsfc_free_ident(struct pdsfc_dev *pdsfc) >> +{ >> + struct device *dev = &pdsfc->fwctl.dev; >> + >> + if (pdsfc->ident) { > > It is not kernel style, which is success oriented. > If (!pdsfc->ident) > return; > > However I don't know how can this happen. You shouldn't call to pdsfc_free_ident > if ident wasn't set. This will change as we rework the ident handling to simply cache it and immediately drop the DMA linkage as suggested by an earlier review. > >> + dma_free_coherent(dev, sizeof(*pdsfc->ident), >> + pdsfc->ident, pdsfc->ident_pa); >> + pdsfc->ident = NULL; > > Please don't assign NULL to pointers if they are not reused. > >> + pdsfc->ident_pa = DMA_MAPPING_ERROR; > > Same. > >> + } >> +} >> + >> +static int pdsfc_identify(struct pdsfc_dev *pdsfc) >> +{ >> + struct device *dev = &pdsfc->fwctl.dev; >> + union pds_core_adminq_comp comp = {0}; >> + union pds_core_adminq_cmd cmd = {0}; >> + struct pds_fwctl_ident *ident; >> + dma_addr_t ident_pa; >> + int err = 0; > > There is no need to assign 0 to err. > >> + >> + ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL); >> + err = dma_mapping_error(dev->parent, ident_pa); >> + if (err) { >> + dev_err(dev, "Failed to map ident\n"); > > This is one of the examples of such extra prints. > >> + return err; >> + } >> + >> + cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT; >> + cmd.fwctl_ident.version = 0; > > How will you manage this version field? Since there is only version 0 at this point, there is not much to manage. I wanted to explicitly show the setting to version 0, but maybe that can be assumed by the basic struct init. > >> + cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident)); >> + cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa); >> + >> + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0); >> + if (err) { >> + dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa); >> + dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n", >> + cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err); >> + return err; >> + } >> + >> + pdsfc->ident = ident; >> + pdsfc->ident_pa = ident_pa; >> + >> + dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n", >> + ident->version, ident->max_req_sz, ident->max_resp_sz, >> + ident->max_req_sg_elems, ident->max_resp_sg_elems); >> + >> + return 0; >> +} >> + >> +static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope, >> + void *in, size_t in_len, size_t *out_len) >> +{ >> + return NULL; >> +} >> + >> +static const struct fwctl_ops pdsfc_ops = { >> + .device_type = FWCTL_DEVICE_TYPE_PDS, >> + .uctx_size = sizeof(struct pdsfc_uctx), >> + .open_uctx = pdsfc_open_uctx, >> + .close_uctx = pdsfc_close_uctx, >> + .info = pdsfc_info, >> + .fw_rpc = pdsfc_fw_rpc, >> +}; >> + >> +static int pdsfc_probe(struct auxiliary_device *adev, >> + const struct auxiliary_device_id *id) >> +{ >> + struct pdsfc_dev *pdsfc __free(pdsfc_dev); >> + struct pds_auxiliary_dev *padev; >> + struct device *dev = &adev->dev; >> + int err = 0; >> + >> + padev = container_of(adev, struct pds_auxiliary_dev, aux_dev); >> + pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops, >> + struct pdsfc_dev, fwctl); >> + if (!pdsfc) { >> + dev_err(dev, "Failed to allocate fwctl device struct\n"); >> + return -ENOMEM; >> + } >> + pdsfc->padev = padev; >> + >> + err = pdsfc_identify(pdsfc); >> + if (err) { >> + dev_err(dev, "Failed to identify device, err %d\n", err); >> + return err; >> + } >> + >> + err = fwctl_register(&pdsfc->fwctl); >> + if (err) { >> + dev_err(dev, "Failed to register device, err %d\n", err); >> + return err; >> + } >> + >> + auxiliary_set_drvdata(adev, no_free_ptr(pdsfc)); >> + >> + return 0; >> + >> +free_ident: >> + pdsfc_free_ident(pdsfc); >> + return err; >> +} >> + >> +static void pdsfc_remove(struct auxiliary_device *adev) >> +{ >> + struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev); >> + >> + fwctl_unregister(&pdsfc->fwctl); >> + pdsfc_free_ident(pdsfc); >> +} >> + >> +static const struct auxiliary_device_id pdsfc_id_table[] = { >> + {.name = PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_FWCTL_STR }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(auxiliary, pdsfc_id_table); >> + >> +static struct auxiliary_driver pdsfc_driver = { >> + .name = "pds_fwctl", >> + .probe = pdsfc_probe, >> + .remove = pdsfc_remove, >> + .id_table = pdsfc_id_table, >> +}; >> + >> +module_auxiliary_driver(pdsfc_driver); >> + >> +MODULE_IMPORT_NS(FWCTL); >> +MODULE_DESCRIPTION("pds fwctl driver"); >> +MODULE_AUTHOR("Shannon Nelson <shannon.nelson@amd.com>"); >> +MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>"); >> +MODULE_LICENSE("Dual BSD/GPL"); >> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h >> index 4b4e9a98b37b..7fc353b63353 100644 >> --- a/include/linux/pds/pds_adminq.h >> +++ b/include/linux/pds/pds_adminq.h >> @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd { >> u8 status; >> }; >> >> +enum pds_fwctl_cmd_opcode { >> + PDS_FWCTL_CMD_IDENT = 70, > > Please try to avoid from vertical space alignment. It doesn't survive > time and at some point you will need to reformat it, which will cause > to churn and harm backporting/stable without any reason. Yeah, that was inherited from earlier work not necessary here. > > Thanks Thanks for your time amd comments, sln
On Tue, Feb 18, 2025 at 02:19:03PM -0800, Nelson, Shannon wrote: > On 2/18/2025 11:51 AM, Leon Romanovsky wrote: > > > > On Tue, Feb 11, 2025 at 03:48:52PM -0800, Shannon Nelson wrote: > > > Initial files for adding a new fwctl driver for the AMD/Pensando PDS > > > devices. This sets up a simple auxiliary_bus driver that registers > > > with fwctl subsystem. It expects that a pds_core device has set up > > > the auxiliary_device pds_core.fwctl > > > > > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > > --- > > > MAINTAINERS | 7 ++ > > > drivers/fwctl/Kconfig | 10 ++ > > > drivers/fwctl/Makefile | 1 + > > > drivers/fwctl/pds/Makefile | 4 + > > > drivers/fwctl/pds/main.c | 195 +++++++++++++++++++++++++++++++++ > > > include/linux/pds/pds_adminq.h | 77 +++++++++++++ > > > include/uapi/fwctl/fwctl.h | 1 + > > > include/uapi/fwctl/pds.h | 27 +++++ > > > 8 files changed, 322 insertions(+) > > > create mode 100644 drivers/fwctl/pds/Makefile > > > create mode 100644 drivers/fwctl/pds/main.c > > > create mode 100644 include/uapi/fwctl/pds.h > > > > <...> <...> > > > + return err; > > > + } > > > + > > > + cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT; > > > + cmd.fwctl_ident.version = 0; > > > > How will you manage this version field? > > Since there is only version 0 at this point, there is not much to manage. I > wanted to explicitly show the setting to version 0, but maybe that can be > assumed by the basic struct init. But the question is slightly different "How will you manage this version field?" Thanks
On 2/19/2025 12:25 AM, Leon Romanovsky wrote: > > On Tue, Feb 18, 2025 at 02:19:03PM -0800, Nelson, Shannon wrote: >> On 2/18/2025 11:51 AM, Leon Romanovsky wrote: >>> >>> On Tue, Feb 11, 2025 at 03:48:52PM -0800, Shannon Nelson wrote: >>>> Initial files for adding a new fwctl driver for the AMD/Pensando PDS >>>> devices. This sets up a simple auxiliary_bus driver that registers >>>> with fwctl subsystem. It expects that a pds_core device has set up >>>> the auxiliary_device pds_core.fwctl >>>> >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >>>> --- >>>> MAINTAINERS | 7 ++ >>>> drivers/fwctl/Kconfig | 10 ++ >>>> drivers/fwctl/Makefile | 1 + >>>> drivers/fwctl/pds/Makefile | 4 + >>>> drivers/fwctl/pds/main.c | 195 +++++++++++++++++++++++++++++++++ >>>> include/linux/pds/pds_adminq.h | 77 +++++++++++++ >>>> include/uapi/fwctl/fwctl.h | 1 + >>>> include/uapi/fwctl/pds.h | 27 +++++ >>>> 8 files changed, 322 insertions(+) >>>> create mode 100644 drivers/fwctl/pds/Makefile >>>> create mode 100644 drivers/fwctl/pds/main.c >>>> create mode 100644 include/uapi/fwctl/pds.h >>> >>> <...> > > <...> > >>>> + return err; >>>> + } >>>> + >>>> + cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT; >>>> + cmd.fwctl_ident.version = 0; >>> >>> How will you manage this version field? >> >> Since there is only version 0 at this point, there is not much to manage. I >> wanted to explicitly show the setting to version 0, but maybe that can be >> assumed by the basic struct init. > > But the question is slightly different "How will you manage this version field?" If we find we have to change the interface in a non-backward-compatable way, we'll increment the version number that we support, and watch for the version number supported by the firmware as reported in the ident struct data and interpret the data appropriately. Similarly, if the firmware sees that the host driver is at a lower version number, it will handle data in the older format. sln
diff --git a/MAINTAINERS b/MAINTAINERS index 413ab79bf2f4..123f8a9c0b26 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9602,6 +9602,13 @@ T: git git://linuxtv.org/media.git F: Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml F: drivers/media/i2c/gc2145.c +FWCTL PDS DRIVER +M: Brett Creeley <brett.creeley@amd.com> +R: Shannon Nelson <shannon.nelson@amd.com> +L: linux-kernel@vger.kernel.org +S: Maintained +F: drivers/fwctl/pds/ + GATEWORKS SYSTEM CONTROLLER (GSC) DRIVER M: Tim Harvey <tharvey@gateworks.com> S: Maintained diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig index 0a542a247303..df87ce5bd8aa 100644 --- a/drivers/fwctl/Kconfig +++ b/drivers/fwctl/Kconfig @@ -28,5 +28,15 @@ config FWCTL_MLX5 This will allow configuration and debug tools to work out of the box on mainstream kernel. + If you don't know what to do here, say N. + +config FWCTL_PDS + tristate "AMD/Pensando pds fwctl driver" + depends on PDS_CORE + help + The pds_fwctl driver provides an fwctl interface for a user process + to access the debug and configuration information of the AMD/Pensando + DSC hardware family. + If you don't know what to do here, say N. endif diff --git a/drivers/fwctl/Makefile b/drivers/fwctl/Makefile index 5fb289243286..692e4b8d7beb 100644 --- a/drivers/fwctl/Makefile +++ b/drivers/fwctl/Makefile @@ -2,5 +2,6 @@ obj-$(CONFIG_FWCTL) += fwctl.o obj-$(CONFIG_FWCTL_BNXT) += bnxt/ obj-$(CONFIG_FWCTL_MLX5) += mlx5/ +obj-$(CONFIG_FWCTL_PDS) += pds/ fwctl-y += main.o diff --git a/drivers/fwctl/pds/Makefile b/drivers/fwctl/pds/Makefile new file mode 100644 index 000000000000..c14cba128e3b --- /dev/null +++ b/drivers/fwctl/pds/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause +obj-$(CONFIG_FWCTL_PDS) += pds_fwctl.o + +pds_fwctl-y += main.o diff --git a/drivers/fwctl/pds/main.c b/drivers/fwctl/pds/main.c new file mode 100644 index 000000000000..24979fe0deea --- /dev/null +++ b/drivers/fwctl/pds/main.c @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause +/* Copyright(c) Advanced Micro Devices, Inc */ + +#include <linux/module.h> +#include <linux/auxiliary_bus.h> +#include <linux/pci.h> +#include <linux/vmalloc.h> + +#include <uapi/fwctl/fwctl.h> +#include <uapi/fwctl/pds.h> +#include <linux/fwctl.h> + +#include <linux/pds/pds_common.h> +#include <linux/pds/pds_core_if.h> +#include <linux/pds/pds_adminq.h> +#include <linux/pds/pds_auxbus.h> + +struct pdsfc_uctx { + struct fwctl_uctx uctx; + u32 uctx_caps; + u32 uctx_uid; +}; + +struct pdsfc_dev { + struct fwctl_device fwctl; + struct pds_auxiliary_dev *padev; + struct pdsc *pdsc; + u32 caps; + dma_addr_t ident_pa; + struct pds_fwctl_ident *ident; +}; +DEFINE_FREE(pdsfc_dev, struct pdsfc_dev *, if (_T) fwctl_put(&_T->fwctl)); + +static int pdsfc_open_uctx(struct fwctl_uctx *uctx) +{ + struct pdsfc_dev *pdsfc = container_of(uctx->fwctl, struct pdsfc_dev, fwctl); + struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx); + struct device *dev = &uctx->fwctl->dev; + + dev_dbg(dev, "%s: caps = 0x%04x\n", __func__, pdsfc->caps); + pdsfc_uctx->uctx_caps = pdsfc->caps; + + return 0; +} + +static void pdsfc_close_uctx(struct fwctl_uctx *uctx) +{ +} + +static void *pdsfc_info(struct fwctl_uctx *uctx, size_t *length) +{ + struct pdsfc_uctx *pdsfc_uctx = container_of(uctx, struct pdsfc_uctx, uctx); + struct fwctl_info_pds *info; + + info = kzalloc(sizeof(*info), GFP_KERNEL); + if (!info) + return ERR_PTR(-ENOMEM); + + info->uctx_caps = pdsfc_uctx->uctx_caps; + + return info; +} + +static void pdsfc_free_ident(struct pdsfc_dev *pdsfc) +{ + struct device *dev = &pdsfc->fwctl.dev; + + if (pdsfc->ident) { + dma_free_coherent(dev, sizeof(*pdsfc->ident), + pdsfc->ident, pdsfc->ident_pa); + pdsfc->ident = NULL; + pdsfc->ident_pa = DMA_MAPPING_ERROR; + } +} + +static int pdsfc_identify(struct pdsfc_dev *pdsfc) +{ + struct device *dev = &pdsfc->fwctl.dev; + union pds_core_adminq_comp comp = {0}; + union pds_core_adminq_cmd cmd = {0}; + struct pds_fwctl_ident *ident; + dma_addr_t ident_pa; + int err = 0; + + ident = dma_alloc_coherent(dev->parent, sizeof(*ident), &ident_pa, GFP_KERNEL); + err = dma_mapping_error(dev->parent, ident_pa); + if (err) { + dev_err(dev, "Failed to map ident\n"); + return err; + } + + cmd.fwctl_ident.opcode = PDS_FWCTL_CMD_IDENT; + cmd.fwctl_ident.version = 0; + cmd.fwctl_ident.len = cpu_to_le32(sizeof(*ident)); + cmd.fwctl_ident.ident_pa = cpu_to_le64(ident_pa); + + err = pds_client_adminq_cmd(pdsfc->padev, &cmd, sizeof(cmd), &comp, 0); + if (err) { + dma_free_coherent(dev->parent, PAGE_SIZE, ident, ident_pa); + dev_err(dev, "Failed to send adminq cmd opcode: %u entity: %u err: %d\n", + cmd.fwctl_query.opcode, cmd.fwctl_query.entity, err); + return err; + } + + pdsfc->ident = ident; + pdsfc->ident_pa = ident_pa; + + dev_dbg(dev, "ident: version %u max_req_sz %u max_resp_sz %u max_req_sg_elems %u max_resp_sg_elems %u\n", + ident->version, ident->max_req_sz, ident->max_resp_sz, + ident->max_req_sg_elems, ident->max_resp_sg_elems); + + return 0; +} + +static void *pdsfc_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope, + void *in, size_t in_len, size_t *out_len) +{ + return NULL; +} + +static const struct fwctl_ops pdsfc_ops = { + .device_type = FWCTL_DEVICE_TYPE_PDS, + .uctx_size = sizeof(struct pdsfc_uctx), + .open_uctx = pdsfc_open_uctx, + .close_uctx = pdsfc_close_uctx, + .info = pdsfc_info, + .fw_rpc = pdsfc_fw_rpc, +}; + +static int pdsfc_probe(struct auxiliary_device *adev, + const struct auxiliary_device_id *id) +{ + struct pdsfc_dev *pdsfc __free(pdsfc_dev); + struct pds_auxiliary_dev *padev; + struct device *dev = &adev->dev; + int err = 0; + + padev = container_of(adev, struct pds_auxiliary_dev, aux_dev); + pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops, + struct pdsfc_dev, fwctl); + if (!pdsfc) { + dev_err(dev, "Failed to allocate fwctl device struct\n"); + return -ENOMEM; + } + pdsfc->padev = padev; + + err = pdsfc_identify(pdsfc); + if (err) { + dev_err(dev, "Failed to identify device, err %d\n", err); + return err; + } + + err = fwctl_register(&pdsfc->fwctl); + if (err) { + dev_err(dev, "Failed to register device, err %d\n", err); + return err; + } + + auxiliary_set_drvdata(adev, no_free_ptr(pdsfc)); + + return 0; + +free_ident: + pdsfc_free_ident(pdsfc); + return err; +} + +static void pdsfc_remove(struct auxiliary_device *adev) +{ + struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev); + + fwctl_unregister(&pdsfc->fwctl); + pdsfc_free_ident(pdsfc); +} + +static const struct auxiliary_device_id pdsfc_id_table[] = { + {.name = PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_FWCTL_STR }, + {} +}; +MODULE_DEVICE_TABLE(auxiliary, pdsfc_id_table); + +static struct auxiliary_driver pdsfc_driver = { + .name = "pds_fwctl", + .probe = pdsfc_probe, + .remove = pdsfc_remove, + .id_table = pdsfc_id_table, +}; + +module_auxiliary_driver(pdsfc_driver); + +MODULE_IMPORT_NS(FWCTL); +MODULE_DESCRIPTION("pds fwctl driver"); +MODULE_AUTHOR("Shannon Nelson <shannon.nelson@amd.com>"); +MODULE_AUTHOR("Brett Creeley <brett.creeley@amd.com>"); +MODULE_LICENSE("Dual BSD/GPL"); diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h index 4b4e9a98b37b..7fc353b63353 100644 --- a/include/linux/pds/pds_adminq.h +++ b/include/linux/pds/pds_adminq.h @@ -1179,6 +1179,78 @@ struct pds_lm_host_vf_status_cmd { u8 status; }; +enum pds_fwctl_cmd_opcode { + PDS_FWCTL_CMD_IDENT = 70, +}; + +/** + * struct pds_fwctl_cmd - Firmware control command structure + * @opcode: Opcode + * @rsvd: Word boundary padding + * @ep: Endpoint identifier. + * @op: Operation identifier. + */ +struct pds_fwctl_cmd { + u8 opcode; + u8 rsvd[3]; + __le32 ep; + __le32 op; +} __packed; + +/** + * struct pds_fwctl_comp - Firmware control completion structure + * @status: Status of the firmware control operation + * @rsvd: Word boundary padding + * @comp_index: Completion index in little-endian format + * @rsvd2: Word boundary padding + * @color: Color bit indicating the state of the completion + */ +struct pds_fwctl_comp { + u8 status; + u8 rsvd; + __le16 comp_index; + u8 rsvd2[11]; + u8 color; +} __packed; + +/** + * struct pds_fwctl_ident_cmd - Firmware control identification command structure + * @opcode: Operation code for the command + * @rsvd: Word boundary padding + * @version: Interface version + * @rsvd2: Word boundary padding + * @len: Length of the identification data + * @ident_pa: Physical address of the identification data + */ +struct pds_fwctl_ident_cmd { + u8 opcode; + u8 rsvd; + u8 version; + u8 rsvd2; + __le32 len; + __le64 ident_pa; +} __packed; + +/** + * struct pds_fwctl_ident - Firmware control identification structure + * @features: Supported features + * @version: Interface version + * @rsvd: Word boundary padding + * @max_req_sz: Maximum request size + * @max_resp_sz: Maximum response size + * @max_req_sg_elems: Maximum number of request SGs + * @max_resp_sg_elems: Maximum number of response SGs + */ +struct pds_fwctl_ident { + __le64 features; + u8 version; + u8 rsvd[3]; + __le32 max_req_sz; + __le32 max_resp_sz; + u8 max_req_sg_elems; + u8 max_resp_sg_elems; +} __packed; + union pds_core_adminq_cmd { u8 opcode; u8 bytes[64]; @@ -1216,6 +1288,9 @@ union pds_core_adminq_cmd { struct pds_lm_dirty_enable_cmd lm_dirty_enable; struct pds_lm_dirty_disable_cmd lm_dirty_disable; struct pds_lm_dirty_seq_ack_cmd lm_dirty_seq_ack; + + struct pds_fwctl_cmd fwctl; + struct pds_fwctl_ident_cmd fwctl_ident; }; union pds_core_adminq_comp { @@ -1243,6 +1318,8 @@ union pds_core_adminq_comp { struct pds_lm_state_size_comp lm_state_size; struct pds_lm_dirty_status_comp lm_dirty_status; + + struct pds_fwctl_comp fwctl; }; #ifndef __CHECKER__ diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h index 518f054f02d2..a884e9f6dc2c 100644 --- a/include/uapi/fwctl/fwctl.h +++ b/include/uapi/fwctl/fwctl.h @@ -44,6 +44,7 @@ enum fwctl_device_type { FWCTL_DEVICE_TYPE_ERROR = 0, FWCTL_DEVICE_TYPE_MLX5 = 1, FWCTL_DEVICE_TYPE_BNXT = 3, + FWCTL_DEVICE_TYPE_PDS = 4, }; /** diff --git a/include/uapi/fwctl/pds.h b/include/uapi/fwctl/pds.h new file mode 100644 index 000000000000..a01b032cbdb1 --- /dev/null +++ b/include/uapi/fwctl/pds.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +/* Copyright(c) Advanced Micro Devices, Inc */ + +/* + * fwctl interface info for pds_fwctl + */ + +#ifndef _UAPI_FWCTL_PDS_H_ +#define _UAPI_FWCTL_PDS_H_ + +#include <linux/types.h> + +/* + * struct fwctl_info_pds + * + * Return basic information about the FW interface available. + */ +struct fwctl_info_pds { + __u32 uid; + __u32 uctx_caps; +}; + +enum pds_fwctl_capabilities { + PDS_FWCTL_QUERY_CAP = 0, + PDS_FWCTL_SEND_CAP, +}; +#endif /* _UAPI_FWCTL_PDS_H_ */
Initial files for adding a new fwctl driver for the AMD/Pensando PDS devices. This sets up a simple auxiliary_bus driver that registers with fwctl subsystem. It expects that a pds_core device has set up the auxiliary_device pds_core.fwctl Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> --- MAINTAINERS | 7 ++ drivers/fwctl/Kconfig | 10 ++ drivers/fwctl/Makefile | 1 + drivers/fwctl/pds/Makefile | 4 + drivers/fwctl/pds/main.c | 195 +++++++++++++++++++++++++++++++++ include/linux/pds/pds_adminq.h | 77 +++++++++++++ include/uapi/fwctl/fwctl.h | 1 + include/uapi/fwctl/pds.h | 27 +++++ 8 files changed, 322 insertions(+) create mode 100644 drivers/fwctl/pds/Makefile create mode 100644 drivers/fwctl/pds/main.c create mode 100644 include/uapi/fwctl/pds.h