| Message ID | 9-v4-0cf4ec3b8143+4995-fwctl_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Introduce fwctl subystem | expand |
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
On Fri, Feb 07, 2025 at 02:59:23PM +0000, Jonathan Cameron wrote: > 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. Yeah, please ignore, it was a mistake to include it Jason
diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig index f802cf5d4951e8..0a542a247303d7 100644 --- a/drivers/fwctl/Kconfig +++ b/drivers/fwctl/Kconfig @@ -9,6 +9,15 @@ menuconfig FWCTL fit neatly into an existing subsystem. if FWCTL +config FWCTL_BNXT + tristate "bnxt control fwctl driver" + depends on BNXT + help + BNXT provides interface for the user process to access the debug and + configuration registers of the Broadcom NIC hardware family + + If you don't know what to do here, say N. + config FWCTL_MLX5 tristate "mlx5 ConnectX control fwctl driver" depends on MLX5_CORE diff --git a/drivers/fwctl/Makefile b/drivers/fwctl/Makefile index 1c535f694d7fe4..5fb289243286ae 100644 --- a/drivers/fwctl/Makefile +++ b/drivers/fwctl/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_FWCTL) += fwctl.o +obj-$(CONFIG_FWCTL_BNXT) += bnxt/ obj-$(CONFIG_FWCTL_MLX5) += mlx5/ fwctl-y += main.o diff --git a/drivers/fwctl/bnxt/Makefile b/drivers/fwctl/bnxt/Makefile new file mode 100644 index 00000000000000..57c76e0e0c9ca7 --- /dev/null +++ b/drivers/fwctl/bnxt/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_FWCTL_BNXT) += bnxt_fwctl.o + +bnxt_fwctl-y += bnxt.o 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 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2024, Broadcom Corporation + */ +#include <linux/fwctl.h> +#include <linux/auxiliary_bus.h> +#include <linux/slab.h> +#include <linux/pci.h> +#include <uapi/fwctl/bnxt.h> + +/* FIXME need a include/linux header for the aux related definitions */ +#include <../../../drivers/net/ethernet/broadcom/bnxt/bnxt.h> +#include <../../../drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h> + +struct bnxtctl_uctx { + struct fwctl_uctx uctx; + u32 uctx_caps; +}; + +struct bnxtctl_dev { + struct fwctl_device fwctl; + struct bnxt_aux_priv *aux_priv; +}; + +DEFINE_FREE(bnxtctl, struct bnxtctl_dev *, if (_T) fwctl_put(&_T->fwctl)) + +static int bnxtctl_open_uctx(struct fwctl_uctx *uctx) +{ + struct bnxtctl_uctx *bnxtctl_uctx = + container_of(uctx, struct bnxtctl_uctx, uctx); + + bnxtctl_uctx->uctx_caps = BIT(FWCTL_BNXT_QUERY_COMMANDS) | + BIT(FWCTL_BNXT_SEND_COMMAND); + return 0; +} + +static void bnxtctl_close_uctx(struct fwctl_uctx *uctx) +{ +} + +static void *bnxtctl_info(struct fwctl_uctx *uctx, size_t *length) +{ + struct bnxtctl_uctx *bnxtctl_uctx = + container_of(uctx, struct bnxtctl_uctx, uctx); + struct fwctl_info_bnxt *info; + + info = kzalloc(sizeof(*info), GFP_KERNEL); + if (!info) + return ERR_PTR(-ENOMEM); + + info->uctx_caps = bnxtctl_uctx->uctx_caps; + + *length = sizeof(*info); + return info; +} + +/* + * 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; + + 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 */ + 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 const struct fwctl_ops bnxtctl_ops = { + .device_type = FWCTL_DEVICE_TYPE_BNXT, + .uctx_size = sizeof(struct bnxtctl_uctx), + .open_uctx = bnxtctl_open_uctx, + .close_uctx = bnxtctl_close_uctx, + .info = bnxtctl_info, + .fw_rpc = bnxtctl_fw_rpc, +}; + +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, + 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 void bnxtctl_remove(struct auxiliary_device *adev) +{ + struct bnxtctl_dev *ctldev = auxiliary_get_drvdata(adev); + + fwctl_unregister(&ctldev->fwctl); + fwctl_put(&ctldev->fwctl); +} + +static const struct auxiliary_device_id bnxtctl_id_table[] = { + { .name = "bnxt_en.fwctl", }, + {}, +}; +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); + +MODULE_IMPORT_NS(BNXT); +MODULE_IMPORT_NS(FWCTL); +MODULE_DESCRIPTION("BNXT fwctl driver"); +MODULE_AUTHOR("Broadcom Corporation"); +MODULE_LICENSE("GPL"); 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. + * + * Return basic information about the FW interface available. + */ +struct fwctl_info_bnxt { + __u32 uctx_caps; + __u32 reserved; +}; + +#endif diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h index 0790b8291ee1bd..518f054f02d2d8 100644 --- a/include/uapi/fwctl/fwctl.h +++ b/include/uapi/fwctl/fwctl.h @@ -43,6 +43,7 @@ enum { enum fwctl_device_type { FWCTL_DEVICE_TYPE_ERROR = 0, FWCTL_DEVICE_TYPE_MLX5 = 1, + FWCTL_DEVICE_TYPE_BNXT = 3, }; /**