Message-ID: <20260130120357.0000639b@huawei.com>
Date: Fri, 30 Jan 2026 12:03:57 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Pavan Chebbi <pavan.chebbi@...adcom.com>
CC: <jgg@...pe.ca>, <michael.chan@...adcom.com>,
	<linux-kernel@...r.kernel.org>, <dave.jiang@...el.com>, <saeedm@...dia.com>,
	<gospo@...adcom.com>, <selvin.xavier@...adcom.com>, <leon@...nel.org>,
	<kalesh-anakkur.purayil@...adcom.com>
Subject: Re: [PATCH v3 fwctl 4/5] fwctl/bnxt_fwctl: Add bnxt fwctl device

On Thu, 29 Jan 2026 07:54:52 -0800
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>
Hi

Various comments inline,

Thanks,

Jonathan

> diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
> index b5583b12a011..b3795a17f8f2 100644
> --- a/drivers/fwctl/Kconfig
> +++ b/drivers/fwctl/Kconfig
> @@ -29,5 +29,16 @@ config FWCTL_PDS
>  	  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.
> +
> +config FWCTL_BNXT
> +	tristate "bnxt control fwctl driver"
> +	depends on BNXT
> +	help
> +	  BNXT provides interface for the user process to access the debug and
> +	  configuration registers of the Broadcom NIC hardware family.
> +	  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.
Same thing on ordering as for the make file.  Just adding at the end doesn't
make it easy to find entries once we have lots of them!

>  endif
> diff --git a/drivers/fwctl/Makefile b/drivers/fwctl/Makefile
> index c093b5f661d6..fdd46f3a0e4e 100644
> --- a/drivers/fwctl/Makefile
> +++ b/drivers/fwctl/Makefile
> @@ -2,5 +2,6 @@
>  obj-$(CONFIG_FWCTL) += fwctl.o
>  obj-$(CONFIG_FWCTL_MLX5) += mlx5/
>  obj-$(CONFIG_FWCTL_PDS) += pds/
> +obj-$(CONFIG_FWCTL_BNXT) += bnxt/
How are we ordering these?  Alphabetical maybe?

>  

> diff --git a/drivers/fwctl/bnxt/main.c b/drivers/fwctl/bnxt/main.c
> new file mode 100644
> index 000000000000..67d65b36cb38
> --- /dev/null
> +++ b/drivers/fwctl/bnxt/main.c

> +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 = {0};
> +	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;
> +	}
> +
> +	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))) {

Why not memdup_user() for this one?

> +			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)
> +			continue;
> +		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:
> +	/* Cleanup any dma memory that bnxt_fw_setup_input_dma() may have
> +	 * allocated
> +	 */
> +	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 have a tear down function to undo what bnxt_fw_setup_input_dma()
does so that the two functions can be easily matched against each other.

> +free_dmabuf_out:
> +	kfree(dma_buf);
> +free_msg_out:
> +	kfree(rpc_in.msg);
Free in reverse order of allocation. Even if that means
a dance with multiple if (err).

Personally I'd probably split the good and bad paths so we can have a nice
clean normal path and avoid if (err) dance when we know it is always set.
The extra clarity would be worth duplicating a few lines.
Would also allow extra labels to avoid kfree() of things that haven't
been allocated (obviously that's safe but still not nice)

I think you could reorder the allocations to do rpc_in.resp first
in which case this style would work better.
> +
> +	if (err) {
> +		kfree(rpc_in.resp);
> +		return ERR_PTR(err);
> +	}
> +
> +	return rpc_in.resp;
> +}

> +}

> diff --git a/include/uapi/fwctl/bnxt.h b/include/uapi/fwctl/bnxt.h
> new file mode 100644
> index 000000000000..5620812d839c
> --- /dev/null
> +++ b/include/uapi/fwctl/bnxt.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2026, Broadcom Inc
> + */
> +
> +#ifndef _UAPI_FWCTL_BNXT_H_
> +#define _UAPI_FWCTL_BNXT_H_
> +
> +#include <linux/types.h>
> +
> +#define MAX_DMA_MEM_SIZE		0x10000 /*64K*/
> +#define DFLT_HWRM_CMD_TIMEOUT		500
> +#define DEVICE_WRITE			0
> +#define DEVICE_READ			1
Those are very generic names to stick in a uapi header. Prefix
them probably or maybe rename the structure field to something like
dma_to_device.

Also, write and read seem a bit oddly aligned with the use in a direction
field. Maybe FWCTL_BNXT_DIR_TO_DEVICE and DIR_FROM_DEVICE.

> +
> +enum fwctl_bnxt_commands {
> +	FWCTL_BNXT_QUERY_COMMANDS = 0,
> +	FWCTL_BNXT_SEND_COMMAND,
Seems like these need to have hardware /firmware matching numbers?
If so I'd make the explicit by assigning them all.  Just assigning
0 kind of gives the wrong impression.

> +};

> +/**
> + * struct fwctl_dma_info_bnxt - describe the buffer that should be DMAed
> + * @data: DMA-intended buffer

As below, I'd call out that's an address. The type obscures that.

> + * @len: length of the @data
> + * @offset: offset at which FW (HWRM) input structure needs DMA address
> + * @dma_direction: DMA direction, DEVICE_READ or DEVICE_WRITE
> + * @unused: pad
> + */
> +struct fwctl_dma_info_bnxt {
> +	__aligned_u64 data;
> +	__u32 len;
> +	__u16 offset;
> +	__u8 dma_direction;
> +	__u8 unused;
> +};
> +
> +/**
> + * struct fwctl_rpc_bnxt - describe the fwctl message for bnxt
> + * @req: FW (HWRM) command input structure
> + * @req_len: length of @req
> + * @timeout: if the user wants to override the driver's default, 0 otherwise
> + * @num_dma: number of DMA buffers to be added to @req
> + * @payload: DMA buffer details in struct fwctl_dma_info_bnxt format

That comment has me confused.  If it's in that format, why not use that structure?
Also it doesn't fit which confused me even more!
Ah. I checked the usage. It's a pointer.  Say that in the comment.
It's a bit of a pitty can't type it such that the pointer version of __counted_by
will work on payload. Never mind...

> + */
> +struct fwctl_rpc_bnxt {
> +	__aligned_u64 req;
> +	__u32 req_len;
> +	__u32 timeout;
> +	__u32 num_dma;
> +	__aligned_u64 payload;
> +};
> +#endif

Powered by blists - more mailing lists