Message-ID: <20250218195132.GB53094@unreal>
Date: Tue, 18 Feb 2025 21:51:32 +0200
From: Leon Romanovsky <leonro@...dia.com>
To: Shannon Nelson <shannon.nelson@....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 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.

> +	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@....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.

Thanks

Powered by blists - more mailing lists