Re: [PATCH v4 09/10] fwctl/bnxt: Support communicating with bnxt fw

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu,  6 Feb 2025 20:13:31 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> From: Andy Gospodarek <gospo@broadcom.com>
> 
> This patch adds basic support for the fwctl infrastructure.  With the
> approriate tool, the most basic RPC to the bnxt_en firmware can be
> called.
> 
> Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
As commented on below, this one should perhaps have been marked
RFC given there are a bunch of FIXME inline.


> diff --git a/drivers/fwctl/bnxt/bnxt.c b/drivers/fwctl/bnxt/bnxt.c
> new file mode 100644
> index 00000000000000..d2b9a64a1402bf
> --- /dev/null
> +++ b/drivers/fwctl/bnxt/bnxt.c
> @@ -0,0 +1,167 @@

> +
> +/*
> + * bnxt_fw_msg->msg has the whole command
> + * the start of message is of type struct input
> + * struct input {
> + *         __le16  req_type;
> + *         __le16  cmpl_ring;
> + *         __le16  seq_id;
> + *         __le16  target_id;
> + *         __le64  resp_addr;
> + * };
> + * so the hwrm op should be (struct input *)(hwrm_in->msg)->req_type
> + */
> +static bool bnxtctl_validate_rpc(struct fwctl_uctx *uctx,
> +				 struct bnxt_fw_msg *hwrm_in)
> +{
> +	struct input *req = (struct input *)hwrm_in->msg;

hwrm_in->msg is void * so should be no need to cast here.

> +
> +	switch (req->req_type) {
> +	case HWRM_VER_GET:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +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;
> +	/* FIXME: Check me */

With the various FIXME in here I'm guessing this is an RFC for now?
Maybe better to make that clear in the patch title.

> +	struct bnxt_fw_msg rpc_in = {
> +		// FIXME: does bnxt_send_msg() copy?
> +		.msg = in,
> +		.msg_len = in_len,
> +		.resp = in,
> +		// FIXME: Dynamic memory for out_len
> +		.resp_max_len = in_len,
> +	};
> +	int rc;
> +
> +	if (!bnxtctl_validate_rpc(uctx, &rpc_in))
> +		return ERR_PTR(-EPERM);
> +
> +	rc = bnxt_send_msg(bnxt_aux_priv->edev, &rpc_in);
> +	if (!rc)
> +		return ERR_PTR(-EOPNOTSUPP);
> +	return in;
> +}

> +
> +static int bnxtctl_probe(struct auxiliary_device *adev,
> +			 const struct auxiliary_device_id *id)
> +{
> +	struct bnxt_aux_priv *aux_priv =
> +		container_of(adev, struct bnxt_aux_priv, aux_dev);
> +	struct bnxtctl_dev *bnxtctl __free(bnxtctl) =
> +		fwctl_alloc_device(&aux_priv->edev->pdev->dev, &bnxtctl_ops,

Does this make more sense than setting parent to the
auxiliary device?  (same applies to the mlx5 driver but I didn't notice
it there).  That will result in a deeper nest in sysfs but
at least makes it obvious what the aux dev is doing.

> +				   struct bnxtctl_dev, fwctl);
> +	int rc;
> +
> +	if (!bnxtctl)
> +		return -ENOMEM;
> +
> +	bnxtctl->aux_priv = aux_priv;
> +
> +	rc = fwctl_register(&bnxtctl->fwctl);
> +	if (rc)
> +		return rc;
> +
> +	auxiliary_set_drvdata(adev, no_free_ptr(bnxtctl));
> +	return 0;
> +}

> +static const struct auxiliary_device_id bnxtctl_id_table[] = {
> +	{ .name = "bnxt_en.fwctl", },
> +	{},

Trivial but no need for that trailing comma given this will always
be the last entry.

> +};
> +MODULE_DEVICE_TABLE(auxiliary, bnxtctl_id_table);
> +
> +static struct auxiliary_driver bnxtctl_driver = {
> +	.name = "bnxt_fwctl",
> +	.probe = bnxtctl_probe,
> +	.remove = bnxtctl_remove,
> +	.id_table = bnxtctl_id_table,
> +};
> +
> +module_auxiliary_driver(bnxtctl_driver);

> diff --git a/include/uapi/fwctl/bnxt.h b/include/uapi/fwctl/bnxt.h
> new file mode 100644
> index 00000000000000..ea47fdbbf6ea3e
> --- /dev/null
> +++ b/include/uapi/fwctl/bnxt.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2024, Broadcom Corporation
> + *
> + */
> +#ifndef _UAPI_FWCTL_BNXT_H_
> +#define _UAPI_FWCTL_BNXT_H_
> +
> +#include <linux/types.h>
> +
> +enum fwctl_bnxt_commands {
> +	FWCTL_BNXT_QUERY_COMMANDS = 0,
> +	FWCTL_BNXT_SEND_COMMAND,
> +};
> +
> +/**
> + * struct fwctl_info_bnxt - ioctl(FWCTL_INFO) out_device_data
> + * @uctx_caps: The command capabilities driver accepts.

Silly though it may be, if the kernel-doc script runs on this I'm fairly
sure it will moan about lack of docs for reserved.

> + *
> + * Return basic information about the FW interface available.
> + */
> +struct fwctl_info_bnxt {
> +	__u32 uctx_caps;
> +	__u32 reserved;
> +};
> +
> +#endif