Re: [PATCH v5 fwctl 3/3] fwctl/bnxt: add DMA buffer support for HWRM commands

On Thu, Aug 6, 2026 at 11:27 PM Jason Gunthorpe <[email protected]> wrote:
>
> 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?
>

Yes, agree. Will do.

> 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.
>

Yes will fix the casting to __le64 *. But the assumption of
pdi_req_buf_len for both req and resp is deliberate.
I need to work with FW team to change to change it to have specific
lengths. For now I am confirming the assumption is deliberate.

> > +     { 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?
>

Since hsi.h is generated from the HWRM spec rather than hand authored,
it'd need to be fixed at that source.
I will take this as a follow up change that we need to do internally..

>
> > +static const struct bnxtctl_cmd_dma_desc *
> > +bnxtctl_find_dma_desc(u16 req_type)
> > +{
> > +     int i;
>
> unsigned for things that can't be negative
>

Sure,

> > +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
>

Ok

> > @@ -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.
>

Ok

> >       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.
>

Ok, yes, entire purpose we are doing this patch.. Thanks for catching this

> > +/**
> > + * 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?
>

Actually true, once the length got obtained from the main buffer I can
actually do away with driver_data

> Also that table is quite hard to read, I suggest some macros:
>

Ok, yea it does make it look better.

>
> /* 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?
>

Yes, I will do my best to send in next 12-24 hours. Thanks

> Jason