On Thu, 6 Feb 2025 20:13:31 -0400
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> From: Andy Gospodarek <gospo@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
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
