On Tue, 14 Oct 2025 01:10:32 -0700
Pavan Chebbi [off-list ref] 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 <redacted>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>

A few things inline.

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 [off-list ref]");
+MODULE_AUTHOR("Andy Gospodarek [off-list ref]");
+MODULE_LICENSE("GPL");