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

["Nelson, Shannon" <shannon.nelson@amd.com>]

On 3/10/2025 11:28 AM, Dave Jiang wrote:
> 
> On 3/7/25 11:53 AM, 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
>>
>> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> 
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> 
> minor comment below.
>> ---
>>   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 |  83 ++++++++++++++++
>>   include/uapi/fwctl/fwctl.h     |   1 +
>>   include/uapi/fwctl/pds.h       |  26 +++++
>>   8 files changed, 301 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 3381e41dcf37..c63fd76a3684 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9576,6 +9576,13 @@ L:     linux-kernel@vger.kernel.org
>>   S:   Maintained
>>   F:   drivers/fwctl/mlx5/
>>
>> +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/
>> +
>>   GALAXYCORE GC0308 CAMERA SENSOR DRIVER
>>   M:   Sebastian Reichel <sre@kernel.org>
>>   L:   linux-media@vger.kernel.org
>> diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
>> index f802cf5d4951..b5583b12a011 100644
>> --- a/drivers/fwctl/Kconfig
>> +++ b/drivers/fwctl/Kconfig
>> @@ -19,5 +19,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 1c535f694d7f..c093b5f661d6 100644
>> --- a/drivers/fwctl/Makefile
>> +++ b/drivers/fwctl/Makefile
>> @@ -1,5 +1,6 @@
>>   # SPDX-License-Identifier: GPL-2.0
>>   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..cc2317c07be1
>> --- /dev/null
>> +++ b/drivers/fwctl/pds/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +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..27942315a602
>> --- /dev/null
>> +++ b/drivers/fwctl/pds/main.c
>> @@ -0,0 +1,169 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* 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;
>> +};
>> +
>> +struct pdsfc_dev {
>> +     struct fwctl_device fwctl;
>> +     struct pds_auxiliary_dev *padev;
>> +     u32 caps;
>> +     struct pds_fwctl_ident ident;
>> +};
>> +
>> +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;
>> +
>> +     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 err;
>> +}
>> +
>> +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 device *dev = &adev->dev;
>> +     struct pdsfc_dev *pdsfc;
>> +     int err;
>> +
>> +     pdsfc = fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
>> +                                struct pdsfc_dev, fwctl);
>> +     if (!pdsfc)
>> +             return dev_err_probe(dev, -ENOMEM, "Failed to allocate fwctl device struct\n");
>> +     pdsfc->padev = padev;
>> +
>> +     err = pdsfc_identify(pdsfc);
>> +     if (err) {
>> +             fwctl_put(&pdsfc->fwctl);
>> +             return dev_err_probe(dev, err, "Failed to identify device\n");
>> +     }
>> +
>> +     err = fwctl_register(&pdsfc->fwctl);
>> +     if (err) {
>> +             fwctl_put(&pdsfc->fwctl);
>> +             return dev_err_probe(dev, err, "Failed to register device\n");
>> +     }
>> +
>> +     auxiliary_set_drvdata(adev, pdsfc);
>> +
>> +     return 0;
>> +}
>> +
>> +static void pdsfc_remove(struct auxiliary_device *adev)
>> +{
>> +     struct pdsfc_dev *pdsfc = auxiliary_get_drvdata(adev);
>> +
>> +     fwctl_unregister(&pdsfc->fwctl);
>> +     fwctl_put(&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("GPL");
>> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h
>> index 4b4e9a98b37b..22c6d77b3dcb 100644
>> --- a/include/linux/pds/pds_adminq.h
>> +++ b/include/linux/pds/pds_adminq.h
>> @@ -1179,6 +1179,84 @@ 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:   Reserved
>> + * @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:       Reserved
>> + * @comp_index: Completion index in little-endian format
>> + * @rsvd2:      Reserved
>> + * @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:     Reserved
>> + * @version:  Interface version
>> + * @rsvd2:    Reserved
>> + * @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;
>> +
>> +/* future feature bits here
>> + * enum pds_fwctl_features {
>> + * };
>> + * (compilers don't like empty enums)
>> + */
>> +
>> +/**
>> + * struct pds_fwctl_ident - Firmware control identification structure
>> + * @features:    Supported features (enum pds_fwctl_features)
>> + * @version:     Interface version
>> + * @rsvd:        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;
>> +
>>   union pds_core_adminq_cmd {
>>        u8     opcode;
>>        u8     bytes[64];
>> @@ -1216,6 +1294,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 +1324,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 c2d5abc5a726..716ac0eee42d 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_CXL = 2,
>> +     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..558e030b7583
>> --- /dev/null
>> +++ b/include/uapi/fwctl/pds.h
>> @@ -0,0 +1,26 @@
>> +/* 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.
>> + */
> 
> Please use proper kdoc formatting for the comment block.

Will do, thanks,
sln

> 
>> +struct fwctl_info_pds {
>> +     __u32 uctx_caps;
>> +};
>> +
>> +enum pds_fwctl_capabilities {
>> +     PDS_FWCTL_QUERY_CAP = 0,
>> +     PDS_FWCTL_SEND_CAP,
>> +};
>> +#endif /* _UAPI_FWCTL_PDS_H_ */
>