Re: [PATCH v5 fwctl 4/5] fwctl/bnxt_fwctl: Add bnxt fwctl device

[Jason Gunthorpe]

On Thu, Feb 26, 2026 at 12:23:17AM -0800, Pavan Chebbi wrote:
> +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;
> +	size_t off = offsetof(struct fwctl_rpc_bnxt, reserved1);
> +	struct fwctl_rpc_bnxt *msg = in;
> +	struct bnxt_fw_msg rpc_in = {0};
> +	int rc, err = 0;
> +
> +	if (in_len < sizeof(*msg))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (memchr_inv((unsigned char *)msg + off, 0, in_len - off))
> +		return ERR_PTR(-EINVAL);

EOPNOTSUPP

> +/**
> + * struct fwctl_rpc_bnxt - describe the fwctl message for bnxt
> + * @req: FW (HWRM) command input structure
> + * @req_len: length of @req
> + * @timeout: in ms to override the driver's default, 0 otherwise
> + * @reserved: must be 0
> + * @reserved1: must be 0
> + */
> +struct fwctl_rpc_bnxt {
> +	__aligned_u64 req;
> +	__u32 req_len;
> +	__u32 timeout;
> +	__u32 reserved[2];
> +	__aligned_u64 reserved1;

Why? Upstream isn't going to try to be compatible with OOT drivers.

> +};

I know pensando did it this way, but every time I see this I don't
like it.

struct fwctl_rpc {
        __u32 size;
        __u32 scope;
        __u32 in_len;
        __u32 out_len;
        __aligned_u64 in;
        __aligned_u64 out;
         ^^^^^^^^^^^^^^^^^

These two were supposed to be the actual message, not a layer of
driver indirection, that is why the buffer mangement works the way
it does.

"timeout" just doesn't seem like a great reason to put a wrapper
struct..

Would you be happy to add a
   __aligned_u64 driver_data

To fwctl_rpc to use for your timeout and future stuff and leave the
message buffer as it was intended?

Jason