| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <39ec35d0-56a2-4473-a67d-9a6727c61693@amd.com>
Date: Tue, 18 Feb 2025 14:19:03 -0800
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Leon Romanovsky <leonro@...dia.com>
Cc: jgg@...dia.com, andrew.gospodarek@...adcom.com,
aron.silverton@...cle.com, dan.j.williams@...el.com, daniel.vetter@...ll.ch,
dave.jiang@...el.com, dsahern@...nel.org, gospo@...adcom.com,
hch@...radead.org, itayavr@...dia.com, jiri@...dia.com,
Jonathan.Cameron@...wei.com, kuba@...nel.org, lbloch@...dia.com,
saeedm@...dia.com, linux-cxl@...r.kernel.org, linux-rdma@...r.kernel.org,
netdev@...r.kernel.org, brett.creeley@....com
Subject: Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework
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@....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@....com>");
>> +MODULE_AUTHOR("Brett Creeley <brett.creeley@....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
Powered by blists - more mailing lists