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@xxxxxxx> --- 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@xxxxxxx>"); +MODULE_AUTHOR("Brett Creeley <brett.creeley@xxxxxxx>"); +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
