Re: [PATCH v5 fwctl 3/3] fwctl/bnxt: add DMA buffer support for HWRM commands
From: Jason Gunthorpe
Date: Thu Aug 06 2026 - 13:57:59 EST
On Tue, Jul 07, 2026 at 03:30:19PM +0530, Pavan Chebbi wrote:
> +struct bnxtctl_dma_field {
> + size_t offset; /* offsetof(hwrm_xxx_input, addr_field) */
> + u8 dir;
> + size_t len_offset; /* offsetof(hwrm_xxx_input, len_field); 0 if the
> + * command carries no transfer-length field
> + */
> + u8 len_width; /* byte width of the length field: 2 or 4 */
> + u8 len_unit; /* bytes represented by one unit of the length field */
> + u32 buf_len; /* for commands with no length in payload */
> +};
> +
> +struct bnxtctl_cmd_dma_desc {
> + u16 req_type;
> + u8 num_fields;
> + u8 scope_min;
This is a good idea, but it is never used, I think you should search
for this in the validate function instead of adding all the case
statements?
Two things worth your eye, both pre-existing rather than introduced
here: pdi_cmd_buf_addr/pdi_resp_buf_addr are declared __le32 [2] in
hsi.h, not __le64, so bnxtctl_zero_dma_fields() and
bnxtctl_map_dma_bufs() are doing a __le64 write through a __le32[2]
— correct on little-endian but type-punned. And
HWRM_DBG_PTRACE uses pdi_req_buf_len to size both the to-device and
from-device buffers, which is what the original literal did but looks
like it may want a separate response length.
> + { HWRM_DBG_PTRACE, 2, FWCTL_RPC_DEBUG_WRITE,
> + sizeof(struct hwrm_dbg_ptrace_input),
> + {{ .offset = offsetof(struct hwrm_dbg_ptrace_input, pdi_cmd_buf_addr),
> + .dir = FWCTL_BNXT_BUF_TO_DEVICE,
> + .len_offset = offsetof(struct hwrm_dbg_ptrace_input, pdi_req_buf_len),
> + .len_width = 4, .len_unit = 1 },
> + { .offset = offsetof(struct hwrm_dbg_ptrace_input, pdi_resp_buf_addr),
> + .dir = FWCTL_BNXT_BUF_FROM_DEVICE,
> + .len_offset = offsetof(struct hwrm_dbg_ptrace_input, pdi_req_buf_len),
> + .len_width = 4, .len_unit = 1 }} },
AI pointed out that:
struct hwrm_dbg_ptrace_input {
__le16 req_type;
__le16 cmpl_ring;
__le16 seq_id;
__le16 target_id;
__le64 resp_addr;
__le32 pdi_cmd_buf_addr[2];
__le32 pdi_resp_buf_addr[2];
^^^
This is really sketchy to cast an array to a __le64. If it really
means a __le64 then the struct should say that, if it doesn't mean a
__le64 then this is broken right?
> +static const struct bnxtctl_cmd_dma_desc *
> +bnxtctl_find_dma_desc(u16 req_type)
> +{
> + int i;
unsigned for things that can't be negative
> +static int bnxtctl_check_dma_lens(void *cmd, const struct bnxtctl_cmd_dma_desc *desc,
> + const struct fwctl_bnxt_driver_data *dd, u32 *lens)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < dd->num_bufs; i++) {
> + const struct bnxtctl_dma_field *f = &desc->fields[i];
> + u64 len;
> +
> + if (dd->bufs[i].dir != f->dir)
> + return -EINVAL;
> +
> + if (f->len_offset)
> + len = (u64)bnxtctl_read_len_field(cmd, f) * f->len_unit;
check_mul_overflow is probably nicer than this cast to u64
> @@ -178,21 +589,59 @@ static void *bnxtctl_fw_rpc(struct fwctl_uctx *uctx,
> struct bnxtctl_dev *bnxtctl =
> container_of(uctx->fwctl, struct bnxtctl_dev, fwctl);
> struct bnxt_en_dev *edev = bnxtctl->aux_priv->edev;
> + dma_addr_t dma_addrs[FWCTL_BNXT_MAX_BUFS];
> + const struct bnxtctl_cmd_dma_desc *desc;
> + void *kbufs[FWCTL_BNXT_MAX_BUFS] = {0};
> + struct fwctl_bnxt_driver_data dd = {0};
> + struct device *dev = &edev->pdev->dev;
> + u32 dma_lens[FWCTL_BNXT_MAX_BUFS];
> struct bnxt_fw_msg rpc_in = {0};
> + unsigned int num_mapped = 0;
> + struct input *req = in;
> int rc;
Just use {}, the {0} is actually subtly a different thing.
> if (desc) {
> if (in_len < desc->req_size)
> return ERR_PTR(-EINVAL);
Sadly I don't think you can do this, or perhaps you have to have a
very clear contract with your FW team.
If they make the struct longer and the longer struct happens to have
another DMA address in it then it is security broken on old
kernels. So I suggest you check for exact size for every permitted
command.
> +/**
> + * struct fwctl_bnxt_buf - one indirect DMA buffer descriptor
> + * @addr: Userspace pointer to the payload data.
> + * @dir: One of enum fwctl_bnxt_buf_dir.
> + * @rsvd: Must be zero.
> + */
> +struct fwctl_bnxt_buf {
> + __aligned_u64 addr;
> + __u32 dir;
> + __u32 rsvd[3];
> +};
This is a little weird now, you are taking the length from the data
buffer itself but passing the u64 through this array?
I feel like it should be consistent and since you have have a nice
description of the struct layout and DMA direction why not just have
the kernel get everything from the data buffer itself and do away with
this driver data?
Also that table is quite hard to read, I suggest some macros:
/* Struct has an addr/len pair, but len is multiplied by _unit */
#define CMD_DATA_UNIT(_struct, _dir, _data, _len, _unit) \
{ .offset = offsetof(_struct, _data), \
.dir = _dir, \
.len_offset = offsetof(_struct, _len), \
.len_width = sizeof(((_struct *)0)->_len), \
.len_unit = _unit }
/* Struct has an addr/len pair with byte length */
#define CMD_DATA_SIMPLE(_struct, _dir, _data, _len) \
CMD_DATA_UNIT(_struct, _dir, _data, _len, 1)
/* Struct has an addr but the length is fixed */
#define CMD_DATA_FIXED(_struct, _dir, _data, _len) \
{ .offset = offsetof(_struct, _data), .dir = _dir, .buf_len = _len }
#define CMD_DMAS(_req_type, _scope_min, _struct, _num_fields, ...) \
{ \
.req_type = _req_type, \
.scope_min = _scope_min, \
.req_size = sizeof(_struct), \
.num_fields = _num_fields, \
.fields = { __VA_ARGS__ }, \
}
#define CMD_DMA_LEN(_req_type, _scope_min, _dir, _struct, _data, _len) \
CMD_DMAS(_req_type, _scope_min, _struct, 1, \
CMD_DATA_SIMPLE(_struct, _dir, _data, _len))
I fed the macros to AI and it converted thusly:
/*
* Per-command DMA buffer descriptor table for HWRM commands that
* carry __le64 DMA address fields in their input
*/
static const struct bnxtctl_cmd_dma_desc bnxtctl_dma_cmds[] = {
CMD_DMA_LEN(HWRM_NVM_SET_VARIABLE, FWCTL_RPC_CONFIGURATION,
FWCTL_BNXT_BUF_TO_DEVICE,
struct hwrm_nvm_set_variable_input, src_data_addr,
data_len),
CMD_DMA_LEN(HWRM_NVM_GET_VARIABLE, FWCTL_RPC_CONFIGURATION,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_nvm_get_variable_input, dest_data_addr,
data_len),
CMD_DMA_LEN(HWRM_NVM_READ, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE, struct hwrm_nvm_read_input,
host_dest_addr, len),
CMD_DMAS(HWRM_NVM_GET_DIR_ENTRIES, FWCTL_RPC_DEBUG_READ_ONLY,
struct hwrm_nvm_get_dir_entries_input, 1,
CMD_DATA_FIXED(struct hwrm_nvm_get_dir_entries_input,
FWCTL_BNXT_BUF_FROM_DEVICE, host_dest_addr,
FWCTL_BNXT_MAX_DMABUF)),
CMD_DMA_LEN(HWRM_NVM_WRITE, FWCTL_RPC_DEBUG_WRITE,
FWCTL_BNXT_BUF_TO_DEVICE, struct hwrm_nvm_write_input,
host_src_addr, dir_data_length),
CMD_DMA_LEN(HWRM_NVM_MODIFY, FWCTL_RPC_DEBUG_WRITE,
FWCTL_BNXT_BUF_TO_DEVICE, struct hwrm_nvm_modify_input,
host_src_addr, len),
CMD_DMA_LEN(HWRM_NVM_RAW_WRITE_BLK, FWCTL_RPC_DEBUG_WRITE_FULL,
FWCTL_BNXT_BUF_TO_DEVICE,
struct hwrm_nvm_raw_write_blk_input, host_src_addr, len),
CMD_DMA_LEN(HWRM_NVM_RAW_DUMP, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE, struct hwrm_nvm_raw_dump_input,
host_dest_addr, len),
CMD_DMA_LEN(HWRM_FW_GET_STRUCTURED_DATA, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_fw_get_structured_data_input, dest_data_addr,
data_len),
CMD_DMA_LEN(HWRM_FW_SET_STRUCTURED_DATA, FWCTL_RPC_DEBUG_WRITE,
FWCTL_BNXT_BUF_TO_DEVICE,
struct hwrm_fw_set_structured_data_input, src_data_addr,
data_len),
CMD_DMA_LEN(HWRM_FW_LIVEPATCH, FWCTL_RPC_DEBUG_WRITE_FULL,
FWCTL_BNXT_BUF_TO_DEVICE, struct hwrm_fw_livepatch_input,
host_addr, patch_len),
CMD_DMA_LEN(HWRM_DBG_COREDUMP_LIST, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_dbg_coredump_list_input, host_dest_addr,
host_buf_len),
CMD_DMA_LEN(HWRM_DBG_COREDUMP_RETRIEVE, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_dbg_coredump_retrieve_input, host_dest_addr,
host_buf_len),
/* read_len32 counts 32-bit words, not bytes (see bnxt_dbg_hwrm_rd_reg()). */
CMD_DMAS(HWRM_DBG_READ_DIRECT, FWCTL_RPC_DEBUG_READ_ONLY,
struct hwrm_dbg_read_direct_input, 1,
CMD_DATA_UNIT(struct hwrm_dbg_read_direct_input,
FWCTL_BNXT_BUF_FROM_DEVICE,
host_dest_addr, read_len32, 4)),
CMD_DMA_LEN(HWRM_DBG_READ_INDIRECT, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_dbg_read_indirect_input, host_dest_addr,
host_dest_addr_len),
CMD_DMA_LEN(HWRM_DBG_SERDES_TEST, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_dbg_serdes_test_input, resp_data_addr,
data_len),
CMD_DMA_LEN(HWRM_DBG_TOKEN_CFG, FWCTL_RPC_DEBUG_WRITE_FULL,
FWCTL_BNXT_BUF_TO_DEVICE, struct hwrm_dbg_token_cfg_input,
host_src_addr, dbg_token_len),
CMD_DMA_LEN(HWRM_QUEUE_DSCP2PRI_QCFG, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_queue_dscp2pri_qcfg_input, dest_data_addr,
dest_data_buffer_size),
CMD_DMAS(HWRM_PORT_QSTATS, FWCTL_RPC_DEBUG_READ_ONLY,
struct hwrm_port_qstats_input, 2,
CMD_DATA_FIXED(struct hwrm_port_qstats_input,
FWCTL_BNXT_BUF_FROM_DEVICE, tx_stat_host_addr,
sizeof(struct tx_port_stats)),
CMD_DATA_FIXED(struct hwrm_port_qstats_input,
FWCTL_BNXT_BUF_FROM_DEVICE, rx_stat_host_addr,
sizeof(struct rx_port_stats))),
CMD_DMAS(HWRM_PORT_QSTATS_EXT, FWCTL_RPC_DEBUG_READ_ONLY,
struct hwrm_port_qstats_ext_input, 2,
CMD_DATA_SIMPLE(struct hwrm_port_qstats_ext_input,
FWCTL_BNXT_BUF_FROM_DEVICE, tx_stat_host_addr,
tx_stat_size),
CMD_DATA_SIMPLE(struct hwrm_port_qstats_ext_input,
FWCTL_BNXT_BUF_FROM_DEVICE, rx_stat_host_addr,
rx_stat_size)),
CMD_DMAS(HWRM_PORT_QSTATS_EXT_PFC_ADV, FWCTL_RPC_DEBUG_READ_ONLY,
struct hwrm_port_qstats_ext_pfc_adv_input, 2,
CMD_DATA_SIMPLE(struct hwrm_port_qstats_ext_pfc_adv_input,
FWCTL_BNXT_BUF_FROM_DEVICE,
tx_pfc_adv_stat_host_addr, pfc_adv_stat_size),
CMD_DATA_SIMPLE(struct hwrm_port_qstats_ext_pfc_adv_input,
FWCTL_BNXT_BUF_FROM_DEVICE,
rx_pfc_adv_stat_host_addr, pfc_adv_stat_size)),
CMD_DMA_LEN(HWRM_PCIE_QSTATS, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE, struct hwrm_pcie_qstats_input,
pcie_stat_host_addr, pcie_stat_size),
CMD_DMA_LEN(HWRM_STAT_GENERIC_QSTATS, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_stat_generic_qstats_input,
generic_stat_host_addr, generic_stat_size),
CMD_DMA_LEN(HWRM_STAT_QUERY_ROCE_STATS, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_stat_query_roce_stats_input,
roce_stat_host_addr, roce_stat_size),
CMD_DMA_LEN(HWRM_STAT_QUERY_ROCE_STATS_EXT, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_stat_query_roce_stats_ext_input,
roce_stat_host_addr, roce_stat_size),
CMD_DMA_LEN(HWRM_PORT_EVENTS_LOG, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_port_events_log_input, host_dest_addr,
host_dest_addr_len),
CMD_DMA_LEN(HWRM_PORT_PRBS_TEST, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_port_prbs_test_input, resp_data_addr, data_len),
CMD_DMA_LEN(HWRM_PORT_DSC_DUMP, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE, struct hwrm_port_dsc_dump_input,
resp_data_addr, data_len),
/* num_fids counts 16-bit FIDs, not bytes. */
CMD_DMAS(HWRM_SCH_GRP_CFG, FWCTL_RPC_DEBUG_WRITE,
struct hwrm_sch_grp_cfg_input, 1,
CMD_DATA_UNIT(struct hwrm_sch_grp_cfg_input,
FWCTL_BNXT_BUF_TO_DEVICE, fid_table_addr,
num_fids, 2)),
CMD_DMA_LEN(HWRM_SCH_GRP_QCFG, FWCTL_RPC_DEBUG_READ_ONLY,
FWCTL_BNXT_BUF_FROM_DEVICE, struct hwrm_sch_grp_qcfg_input,
fid_table_addr, fid_table_len),
CMD_DMA_LEN(HWRM_SELFTEST_RETRIEVE_SERDES_DATA,
FWCTL_RPC_DEBUG_READ_ONLY, FWCTL_BNXT_BUF_FROM_DEVICE,
struct hwrm_selftest_retrieve_serdes_data_input,
resp_data_addr, data_len),
CMD_DMAS(HWRM_DBG_PTRACE, FWCTL_RPC_DEBUG_WRITE,
struct hwrm_dbg_ptrace_input, 2,
CMD_DATA_SIMPLE(struct hwrm_dbg_ptrace_input,
FWCTL_BNXT_BUF_TO_DEVICE, pdi_cmd_buf_addr,
pdi_req_buf_len),
CMD_DATA_SIMPLE(struct hwrm_dbg_ptrace_input,
FWCTL_BNXT_BUF_FROM_DEVICE, pdi_resp_buf_addr,
pdi_req_buf_len)),
};
Can you respin this really fast please to make this cycle?
Jason