Re: [RFC PATCH fwctl 3/5] pds_fwctl: initial driver framework

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

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