| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251001160332.00000bbf@huawei.com>
Date: Wed, 1 Oct 2025 16:03:32 +0100
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 v4 4/5] bnxt_fwctl: Add bnxt fwctl device
On Sat, 27 Sep 2025 02:39:29 -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.
>
> Also move 'bnxt_aux_priv' definition required by bnxt_fwctl
> from bnxt.h to ulp.h.
>
> Reviewed-by: Andy Gospodarek <gospo@...adcom.com>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@...adcom.com>
A few minor things inline.
J
> diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
> index b5583b12a011..203b6ebb06fc 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
Full stop missing.
> + 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.
> endif
> diff --git a/drivers/fwctl/bnxt/main.c b/drivers/fwctl/bnxt/main.c
> new file mode 100644
> index 000000000000..397b85671bab
> --- /dev/null
> +++ b/drivers/fwctl/bnxt/main.c
> +
> +static int bnxt_fw_setup_input_dma(struct bnxtctl_dev *bnxt_dev,
> + struct device *dev,
> + int num_dma,
> + struct fwctl_dma_info_bnxt *msg,
> + struct bnxt_fw_msg *fw_msg)
> +{
> + u8 i, num_allocated = 0;
> + void *dma_ptr;
> + int rc = 0;
The compiler should be able to figure out that rc is always set in paths to where
it is used. If not fair enough leaving this.
> +
> + for (i = 0; i < num_dma; i++) {
> + if (msg->len == 0 || msg->len > MAX_DMA_MEM_SIZE) {
> + rc = -EINVAL;
> + goto err;
> + }
> + bnxt_dev->dma_virt_addr[i] = dma_alloc_coherent(dev->parent,
> + msg->len,
> + &bnxt_dev->dma_addr[i],
> + GFP_KERNEL);
> + if (!bnxt_dev->dma_virt_addr[i]) {
> + rc = -ENOMEM;
> + goto err;
> + }
> + num_allocated++;
> + if (msg->dma_direction == DEVICE_WRITE) {
> + if (copy_from_user(bnxt_dev->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(bnxt_dev->dma_addr[i]);
> + } else {
> + rc = -EINVAL;
> + goto err;
> + }
> + msg += 1;
> + }
> +
> + return 0;
> +err:
> + for (i = 0; i < num_allocated; i++)
> + dma_free_coherent(dev->parent,
> + msg->len,
> + bnxt_dev->dma_virt_addr[i],
> + bnxt_dev->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;
> + 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 = kzalloc(msg->req_len, GFP_KERNEL);
> + if (!rpc_in.msg)
> + return ERR_PTR(-ENOMEM);
> +
> + if (copy_from_user(rpc_in.msg, u64_to_user_ptr(msg->req),
> + msg->req_len)) {
> + dev_dbg(dev, "Failed to copy in_payload from user\n");
> + err = -EFAULT;
> + goto free_msg_out;
> + }
> +
> + 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))) {
> + dev_dbg(dev, "Failed to copy payload from user\n");
> + err = -EFAULT;
> + goto free_dmabuf_out;
> + }
> +
> + rc = bnxt_fw_setup_input_dma(bnxtctl, dev, msg->num_dma,
> + dma_buf, &rpc_in);
> + if (rc) {
If there is a strong reason to override the value of rc rather than returning
that I'd add a comment.
> + err = -EIO;
> + goto free_dmabuf_out;
> + }
> + }
> +
> + rc = bnxt_send_msg(bnxt_aux_priv->edev, &rpc_in);
> + if (rc) {
> + err = -EIO;
Likewise here.
Overall I'd just use a single rc variable rather than having separate one for err.
> + goto free_dma_out;
> + }
> +
> + for (i = 0; i < msg->num_dma; i++) {
> + if (dma_buf[i].dma_direction == DEVICE_READ) {
> + if (copy_to_user(u64_to_user_ptr(dma_buf[i].data),
> + bnxtctl->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,
> + bnxtctl->dma_virt_addr[i],
> + bnxtctl->dma_addr[i]);
> +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;
> +}
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index ea1d10c50da6..a7bca802a3e7 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -2075,12 +2075,6 @@ struct bnxt_fw_health {
> #define BNXT_FW_IF_RETRY 10
> #define BNXT_FW_SLOT_RESET_RETRY 4
>
Can you drop the linux/auxiliary_bus.h include given I think this
is the only use in here?
> -struct bnxt_aux_priv {
> - struct auxiliary_device aux_dev;
> - struct bnxt_en_dev *edev;
> - int id;
> -};
> -
> enum board_idx {
> BCM57301,
> BCM57302,
> diff --git a/include/uapi/fwctl/bnxt.h b/include/uapi/fwctl/bnxt.h
> new file mode 100644
> index 000000000000..a4686a45eb35
> --- /dev/null
> +++ b/include/uapi/fwctl/bnxt.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2025, Broadcom Corporation
> + */
> +
> +#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
> +
> +enum fwctl_bnxt_commands {
> + FWCTL_BNXT_QUERY_COMMANDS = 0,
> + FWCTL_BNXT_SEND_COMMAND
Maybe there will be more commands in future. So add a trailing comma.
General convention is always do this unless it is a terminating entry
that is just there to count the elements above.
> +};
Powered by blists - more mailing lists