Message-ID: <20251028164410.00002156@huawei.com>
Date: Tue, 28 Oct 2025 16:44:10 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Pavan Chebbi <pavan.chebbi@...adcom.com>
CC: <jgg@...pe.ca>, <michael.chan@...adcom.com>, <dave.jiang@...el.com>,
	<saeedm@...dia.com>, <davem@...emloft.net>, <corbet@....net>,
	<edumazet@...gle.com>, <gospo@...adcom.com>, <kuba@...nel.org>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>, <andrew+netdev@...n.ch>,
	<selvin.xavier@...adcom.com>, <leon@...nel.org>,
	<kalesh-anakkur.purayil@...adcom.com>
Subject: Re: [PATCH net-next v5 4/5] bnxt_fwctl: Add bnxt fwctl device

On Tue, 14 Oct 2025 01:10:32 -0700
Pavan Chebbi <pavan.chebbi@...adcom.com> wrote:

> Create bnxt_fwctl device. This will bind to bnxt's aux device.
> On the upper edge, it will register with the fwctl subsystem.
> It will make use of bnxt's ULP functions to send FW commands.
> 
> Reviewed-by: Andy Gospodarek <gospo@...adcom.com>
> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@...adcom.com>
A few things inline.


> diff --git a/drivers/fwctl/bnxt/main.c b/drivers/fwctl/bnxt/main.c
> new file mode 100644
> index 000000000000..b31f34c1cc3c
> --- /dev/null
> +++ b/drivers/fwctl/bnxt/main.c
> @@ -0,0 +1,453 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025, Broadcom Corporation
> + */
> +
> +#include <linux/kernel.h>

Anything actually in kernel.h used in here?  There is a considerable
effort going on to not include that unless absolutely necessary. Instead
figure out which actual headers are needed.  In general follow 
include what you use (IWYU) principles.  A few things obviously missing
are cleanup.h and mutex.h for the guard(mutex). 


> +#include <linux/auxiliary_bus.h>
Pick an order.
> +#include <linux/slab.h>
> +#include <linux/pci.h>
> +#include <linux/fwctl.h>
> +#include <uapi/fwctl/fwctl.h>
> +#include <uapi/fwctl/bnxt.h>

I'd put the uapi after, but not sure if this is a convention for some kernel code.

> +#include <linux/bnxt/hsi.h>
> +#include <linux/bnxt/ulp.h>


> +static int bnxt_fw_setup_input_dma(struct bnxtctl_dev *bnxt_dev,
> +				   struct device *dev,
> +				   struct fwctl_dma_info_bnxt *msg,
> +				   struct bnxt_fw_msg *fw_msg,
> +				   int num_dma,
> +				   void **dma_virt_addr,
> +				   dma_addr_t *dma_addr)
> +{
> +	u8 i, num_allocated = 0;
> +	void *dma_ptr;
> +	int rc;
> +
> +	for (i = 0; i < num_dma; i++) {
> +		if (msg->len == 0 || msg->len > MAX_DMA_MEM_SIZE) {
> +			rc = -EINVAL;
> +			goto err;
> +		}
> +		dma_virt_addr[i] = dma_alloc_coherent(dev->parent, msg->len,
> +						      &dma_addr[i], GFP_KERNEL);
> +		if (!dma_virt_addr[i]) {
> +			rc = -ENOMEM;
> +			goto err;
> +		}
> +		num_allocated++;
> +		if (msg->dma_direction == DEVICE_WRITE) {
> +			if (copy_from_user(dma_virt_addr[i],
> +					   u64_to_user_ptr(msg->data),
> +					   msg->len)) {
> +				rc = -EFAULT;
> +				goto err;
> +			}
> +		}
> +		dma_ptr = fw_msg->msg + msg->offset;
> +
> +		if ((PTR_ALIGN(dma_ptr, 8) == dma_ptr) &&
> +		    msg->offset < fw_msg->msg_len) {
> +			__le64 *dmap = dma_ptr;
> +
> +			*dmap = cpu_to_le64(dma_addr[i]);
> +		} else {
I would flip the error condition so that only that is out of line as
generally that makes for easier flow.

		if (!(PTR_ALIGN(dma_ptr, 8) == dma_ptr) ||
		      msg->offset >= fw_msg->msg_len)) {
			rc = -EINVAL;
			goto err;
		}

		*(__le64)(dmap_ptr) = cpu_to_le64(dma_addr[i]);

or something like that.

> +			rc = -EINVAL;
> +			goto err;
> +		}
> +		msg += 1;
		msg++;
or do that in the loop update.


> +	}
> +
> +	return 0;
> +err:
> +	for (i = 0; i < num_allocated; i++)
> +		dma_free_coherent(dev->parent, msg->len, dma_virt_addr[i],
> +				  dma_addr[i]);
> +
> +	return rc;
> +}
> +
> +static void *bnxtctl_fw_rpc(struct fwctl_uctx *uctx,
> +			    enum fwctl_rpc_scope scope,
> +			    void *in, size_t in_len, size_t *out_len)
> +{
> +	struct bnxtctl_dev *bnxtctl =
> +		container_of(uctx->fwctl, struct bnxtctl_dev, fwctl);
> +	struct bnxt_aux_priv *bnxt_aux_priv = bnxtctl->aux_priv;
> +	void *dma_virt_addr[MAX_NUM_DMA_INDICATIONS];
> +	dma_addr_t dma_addr[MAX_NUM_DMA_INDICATIONS];
> +	struct fwctl_dma_info_bnxt *dma_buf = NULL;
> +	struct device *dev = &uctx->fwctl->dev;
> +	struct fwctl_rpc_bnxt *msg = in;
> +	struct bnxt_fw_msg rpc_in;
> +	int i, rc, err = 0;
> +
> +	rpc_in.msg = memdup_user(u64_to_user_ptr(msg->req), msg->req_len);
> +	if (IS_ERR(rpc_in.msg))
> +		return rpc_in.msg;
> +
> +	if (!bnxtctl_validate_rpc(bnxt_aux_priv->edev, &rpc_in, scope)) {
> +		err = -EPERM;
> +		goto free_msg_out;

If you follow this path, you will free rpc_in.resp which hasn't
been allocated yet.  Likely not a bug, but definitely a problem for
easy understanding. 

I can understand why you want to keep the if (err) freeing of rpc_in.resp
for the end of the exit path, but if so move 
	rpc_in.resp = kzalloc();
to be the first thing we need to unwind, before rpc_in.msg is allocated above.


> +	}
> +
> +	rpc_in.msg_len = msg->req_len;
> +	rpc_in.resp = kzalloc(*out_len, GFP_KERNEL);
> +	if (!rpc_in.resp) {
> +		err = -ENOMEM;
> +		goto free_msg_out;
> +	}
> +
> +	rpc_in.resp_max_len = *out_len;
> +	if (!msg->timeout)
> +		rpc_in.timeout = DFLT_HWRM_CMD_TIMEOUT;
> +	else
> +		rpc_in.timeout = msg->timeout;
> +
> +	if (msg->num_dma) {
> +		if (msg->num_dma > MAX_NUM_DMA_INDICATIONS) {
> +			dev_err(dev, "DMA buffers exceed the number supported\n");
> +			err = -EINVAL;
> +			goto free_msg_out;
> +		}
> +
> +		dma_buf = kcalloc(msg->num_dma, sizeof(*dma_buf), GFP_KERNEL);
> +		if (!dma_buf) {
> +			err = -ENOMEM;
> +			goto free_msg_out;
> +		}
> +
> +		if (copy_from_user(dma_buf, u64_to_user_ptr(msg->payload),
> +				   msg->num_dma * sizeof(*dma_buf))) {
> +			dev_dbg(dev, "Failed to copy payload from user\n");
> +			err = -EFAULT;
> +			goto free_dmabuf_out;
> +		}
> +
> +		err = bnxt_fw_setup_input_dma(bnxtctl, dev, dma_buf, &rpc_in,
> +					      msg->num_dma, &dma_virt_addr[0],
> +					      &dma_addr[0]);
> +		if (err)
> +			goto free_dmabuf_out;
> +	}
> +
> +	rc = bnxt_send_msg(bnxt_aux_priv->edev, &rpc_in);
> +	if (rc) {
> +		struct output *resp = rpc_in.resp;
> +
> +		/* Copy the response to user always, as it contains
> +		 * detailed status of the command failure
> +		 */
> +		if (!resp->error_code)
> +			/* bnxt_send_msg() returned much before FW
> +			 * received the command.
> +			 */
> +			resp->error_code = rc;
> +
> +		goto free_dma_out;
> +	}
> +
> +	for (i = 0; i < msg->num_dma; i++) {
> +		if (dma_buf[i].dma_direction == DEVICE_READ) {

Consider flipping this to reduce indent and slightly improve readability.

		if (dma_buf[i].dma_direction != DEVICE_READ)
			continue;

		if (copy_to_user(...)

> +			if (copy_to_user(u64_to_user_ptr(dma_buf[i].data),
> +					 dma_virt_addr[i],
> +					 dma_buf[i].len)) {
> +				dev_dbg(dev, "Failed to copy resp to user\n");
> +				err = -EFAULT;
> +				break;
> +			}
> +		}
> +	}
> +free_dma_out:
> +	for (i = 0; i < msg->num_dma; i++)
> +		dma_free_coherent(dev->parent, dma_buf[i].len, dma_virt_addr[i],
> +				  dma_addr[i]);

I'd prefer a little helper function with this to make it clear it's
underdoing stuff in bnxt_fw_setup_input_dma()  Probably only call that
if (msg->num_dma)
to simplify reasoning on whether this is always safe or not.

> +free_dmabuf_out:
> +	kfree(dma_buf);
> +free_msg_out:
> +	kfree(rpc_in.msg);
> +
> +	if (err) {
> +		kfree(rpc_in.resp);
> +		return ERR_PTR(err);
> +	}
> +
> +	return rpc_in.resp;
> +}



> +static const struct auxiliary_device_id bnxtctl_id_table[] = {
> +	{ .name = "bnxt_en.fwctl", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, bnxtctl_id_table);
> +
> +static struct auxiliary_driver bnxtctl_driver = {
> +	.name = "bnxt_fwctl",
> +	.probe = bnxtctl_probe,
> +	.remove = bnxtctl_remove,
> +	.id_table = bnxtctl_id_table,
> +};
> +
> +module_auxiliary_driver(bnxtctl_driver);
> +
> +MODULE_IMPORT_NS("FWCTL");
> +MODULE_DESCRIPTION("BNXT fwctl driver");
> +MODULE_AUTHOR("Pavan Chebbi <pavan.chebbi@...adcom.com>");
> +MODULE_AUTHOR("Andy Gospodarek <gospo@...adcom.com>");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists