To leverage the SmartNIC capabilities available in Broadcom
NetXtreme-C/E ethernet devices, representor pairs must be configured
via bnxt-ctl.
Without bnxt_netlink as a registered family, bnxt-ctl errors as seen
below, even for the most basic of subcommands.
$ sudo bnxt-ctl show-pair
Error: Failed to resolve family for name: bnxt_netlink, Cannot allocate memory
With this patch, bnxt-ctl functions as expected providing display of
and changes to representor pair configurations.
$ sudo bnxt-ctl show-pair
Representor Pair[4]: interface: enp65s0f0 name: 0000:06:00.0 state: down
member(a): Linux PF index 2 Firmware function id: 0x3
...
This patch is a minimally modified port of the bnxt_netlink code
found in out-of-tree bnxt_en-1.10.2-218.1.182.15.tar.gz. The
out-of-tree driver contains both the GPL-2.0 in the file COPYING
and a GPL comment in each source file.
Signed-off-by: Matthew Cover <[email protected]>
---
drivers/net/ethernet/broadcom/bnxt/Makefile | 2 +-
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 6 +
drivers/net/ethernet/broadcom/bnxt/bnxt_netlink.c | 231 ++++++++++++++++++++++
drivers/net/ethernet/broadcom/bnxt/bnxt_netlink.h | 41 ++++
4 files changed, 279 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/ethernet/broadcom/bnxt/bnxt_netlink.c
create mode 100644 drivers/net/ethernet/broadcom/bnxt/bnxt_netlink.h
diff --git a/drivers/net/ethernet/broadcom/bnxt/Makefile b/drivers/net/ethernet/broadcom/bnxt/Makefile
index 2bc2b70..31e154f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/Makefile
+++ b/drivers/net/ethernet/broadcom/bnxt/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_BNXT) += bnxt_en.o
-bnxt_en-y := bnxt.o bnxt_hwrm.o bnxt_sriov.o bnxt_ethtool.o bnxt_dcb.o bnxt_ulp.o bnxt_xdp.o bnxt_ptp.o bnxt_vfr.o bnxt_devlink.o bnxt_dim.o bnxt_coredump.o
+bnxt_en-y := bnxt.o bnxt_hwrm.o bnxt_sriov.o bnxt_ethtool.o bnxt_dcb.o bnxt_ulp.o bnxt_xdp.o bnxt_ptp.o bnxt_vfr.o bnxt_devlink.o bnxt_netlink.o bnxt_dim.o bnxt_coredump.o
bnxt_en-$(CONFIG_BNXT_FLOWER_OFFLOAD) += bnxt_tc.o
bnxt_en-$(CONFIG_DEBUG_FS) += bnxt_debugfs.o
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6a643aa..a33c7b6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -67,6 +67,7 @@
#include "bnxt_dcb.h"
#include "bnxt_xdp.h"
#include "bnxt_ptp.h"
+#include "bnxt_netlink.h"
#include "bnxt_vfr.h"
#include "bnxt_tc.h"
#include "bnxt_devlink.h"
@@ -14118,6 +14119,10 @@ static int __init bnxt_init(void)
{
int err;
+ err = bnxt_nl_register();
+ if (err)
+ pr_info("%s : failed to load\n", BNXT_NL_NAME);
+
bnxt_debug_init();
err = pci_register_driver(&bnxt_pci_driver);
if (err) {
@@ -14130,6 +14135,7 @@ static int __init bnxt_init(void)
static void __exit bnxt_exit(void)
{
+ bnxt_nl_unregister();
pci_unregister_driver(&bnxt_pci_driver);
if (bnxt_pf_wq)
destroy_workqueue(bnxt_pf_wq);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_netlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_netlink.c
new file mode 100644
index 0000000..48c1357
--- /dev/null
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_netlink.c
@@ -0,0 +1,231 @@
+/* Broadcom NetXtreme-C/E network driver.
+ *
+ * Copyright (c) 2014-2016 Broadcom Corporation
+ * Copyright (c) 2016-2017 Broadcom Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+#include <linux/netdevice.h>
+#include <linux/pci.h>
+#include "bnxt_hsi.h"
+#include "bnxt_netlink.h"
+#include "bnxt.h"
+#include "bnxt_hwrm.h"
+
+/* attribute policy */
+static struct nla_policy bnxt_netlink_policy[BNXT_NUM_ATTRS] = {
+ [BNXT_ATTR_PID] = { .type = NLA_U32 },
+ [BNXT_ATTR_IF_INDEX] = { .type = NLA_U32 },
+ [BNXT_ATTR_REQUEST] = { .type = NLA_BINARY },
+ [BNXT_ATTR_RESPONSE] = { .type = NLA_BINARY },
+};
+
+static struct genl_family bnxt_netlink_family;
+
+static int bnxt_parse_attrs(struct nlattr **a, struct bnxt **bp,
+ struct net_device **dev)
+{
+ pid_t pid;
+ struct net *ns = NULL;
+ const char *drivername;
+
+ if (!a[BNXT_ATTR_PID]) {
+ netdev_err(*dev, "No process ID specified\n");
+ goto err_inval;
+ }
+ pid = nla_get_u32(a[BNXT_ATTR_PID]);
+ ns = get_net_ns_by_pid(pid);
+ if (IS_ERR(ns)) {
+ netdev_err(*dev, "Invalid net namespace for pid %d (err: %ld)\n",
+ pid, PTR_ERR(ns));
+ goto err_inval;
+ }
+
+ if (!a[BNXT_ATTR_IF_INDEX]) {
+ netdev_err(*dev, "No interface index specified\n");
+ goto err_inval;
+ }
+ *dev = dev_get_by_index(ns, nla_get_u32(a[BNXT_ATTR_IF_INDEX]));
+
+ put_net(ns);
+ ns = NULL;
+ if (!*dev) {
+ netdev_err(*dev, "Invalid network interface index %d (err: %ld)\n",
+ nla_get_u32(a[BNXT_ATTR_IF_INDEX]), PTR_ERR(ns));
+ goto err_inval;
+ }
+ if (!(*dev)->dev.parent || !(*dev)->dev.parent->driver ||
+ !(*dev)->dev.parent->driver->name) {
+ netdev_err(*dev, "Unable to get driver name for device %s\n",
+ (*dev)->name);
+ goto err_inval;
+ }
+ drivername = (*dev)->dev.parent->driver->name;
+ if (strcmp(drivername, DRV_MODULE_NAME)) {
+ netdev_err(*dev, "Device %s (%s) is not a %s device!\n",
+ (*dev)->name, drivername, DRV_MODULE_NAME);
+ goto err_inval;
+ }
+ *bp = netdev_priv(*dev);
+ if (!*bp) {
+ netdev_warn((*bp)->dev, "No private data\n");
+ goto err_inval;
+ }
+
+ return 0;
+
+err_inval:
+ if (ns && !IS_ERR(ns))
+ put_net(ns);
+ return -EINVAL;
+}
+
+/* handler */
+static int bnxt_netlink_hwrm(struct sk_buff *skb, struct genl_info *info)
+{
+ struct nlattr **a = info->attrs;
+ struct net_device *dev = NULL;
+ struct sk_buff *reply = NULL;
+ struct input *req, *nl_req;
+ struct bnxt *bp = NULL;
+ struct output *resp;
+ int len, rc;
+ void *hdr;
+
+ rc = bnxt_parse_attrs(a, &bp, &dev);
+ if (rc)
+ goto err_rc;
+
+ if (!bp) {
+ rc = -EINVAL;
+ goto err_rc;
+ }
+
+ if (!bp->hwrm_dma_pool) {
+ netdev_warn(bp->dev, "HWRM interface not currently available on %s\n",
+ dev->name);
+ rc = -EINVAL;
+ goto err_rc;
+ }
+
+ if (!a[BNXT_ATTR_REQUEST]) {
+ netdev_warn(bp->dev, "No request specified\n");
+ rc = -EINVAL;
+ goto err_rc;
+ }
+ len = nla_len(a[BNXT_ATTR_REQUEST]);
+ nl_req = nla_data(a[BNXT_ATTR_REQUEST]);
+
+ reply = genlmsg_new(PAGE_SIZE, GFP_KERNEL);
+ if (!reply) {
+ netdev_warn(bp->dev, "Error: genlmsg_new failed\n");
+ rc = -ENOMEM;
+ goto err_rc;
+ }
+
+ rc = hwrm_req_init(bp, req, nl_req->req_type);
+ if (rc)
+ goto err_rc;
+
+ rc = hwrm_req_replace(bp, req, nl_req, len);
+ if (rc)
+ goto err_rc;
+
+ resp = hwrm_req_hold(bp, req);
+ rc = hwrm_req_send_silent(bp, req);
+ if (rc) {
+ /*
+ * Indicate success for the hwrm transport, while letting
+ * the hwrm error be passed back to the netlink caller in
+ * the response message. Caller is responsible for handling
+ * any errors.
+ *
+ * no kernel warnings are logged in this case.
+ */
+ rc = 0;
+ }
+ hdr = genlmsg_put_reply(reply, info, &bnxt_netlink_family, 0,
+ BNXT_CMD_HWRM);
+ if (nla_put(reply, BNXT_ATTR_RESPONSE, resp->resp_len, resp)) {
+ netdev_warn(bp->dev, "No space for response attribte\n");
+ hwrm_req_drop(bp, req);
+ rc = -ENOMEM;
+ goto err_rc;
+ }
+ genlmsg_end(reply, hdr);
+ hwrm_req_drop(bp, req);
+
+ dev_put(dev);
+ dev = NULL;
+
+ return genlmsg_reply(reply, info);
+
+err_rc:
+ if (reply && !IS_ERR(reply))
+ kfree_skb(reply);
+ if (dev && !IS_ERR(dev))
+ dev_put(dev);
+
+ if (bp)
+ netdev_warn(bp->dev, "returning with error code %d\n", rc);
+
+ return rc;
+}
+
+/* handlers */
+#if defined(HAVE_GENL_REG_OPS_GRPS) || !defined(HAVE_GENL_REG_FAMILY_WITH_OPS)
+static const struct genl_ops bnxt_netlink_ops[] = {
+#else
+static struct genl_ops bnxt_netlink_ops[] = {
+#endif
+ {
+ .cmd = BNXT_CMD_HWRM,
+ .flags = GENL_ADMIN_PERM, /* Req's CAP_NET_ADMIN privilege */
+#ifndef HAVE_GENL_POLICY
+ .policy = bnxt_netlink_policy,
+#endif
+ .doit = bnxt_netlink_hwrm,
+ .dumpit = NULL,
+ },
+};
+
+/* family definition */
+static struct genl_family bnxt_netlink_family = {
+#ifdef HAVE_GENL_ID_GENERATE
+ .id = GENL_ID_GENERATE,
+#endif
+ .hdrsize = 0,
+ .name = BNXT_NL_NAME,
+ .version = BNXT_NL_VER,
+ .maxattr = BNXT_NUM_ATTRS,
+#ifdef HAVE_GENL_POLICY
+ .policy = bnxt_netlink_policy,
+#endif
+#ifndef HAVE_GENL_REG_FAMILY_WITH_OPS
+ .ops = bnxt_netlink_ops,
+ .n_ops = ARRAY_SIZE(bnxt_netlink_ops)
+#endif
+};
+
+int bnxt_nl_register(void)
+{
+#ifndef HAVE_GENL_REG_FAMILY_WITH_OPS
+ return genl_register_family(&bnxt_netlink_family);
+#elif defined(HAVE_GENL_REG_OPS_GRPS)
+ return genl_register_family_with_ops(&bnxt_netlink_family,
+ bnxt_netlink_ops);
+#else
+ return genl_register_family_with_ops(&bnxt_netlink_family,
+ bnxt_netlink_ops,
+ ARRAY_SIZE(bnxt_netlink_ops));
+#endif
+
+ return 0;
+}
+
+int bnxt_nl_unregister(void)
+{
+ return genl_unregister_family(&bnxt_netlink_family);
+}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_netlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_netlink.h
new file mode 100644
index 0000000..6939cd4
--- /dev/null
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_netlink.h
@@ -0,0 +1,41 @@
+/* Broadcom NetXtreme-C/E network driver.
+ *
+ * Copyright (c) 2014-2016 Broadcom Corporation
+ * Copyright (c) 2016-2017 Broadcom Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+#ifndef __BNXT_NETLINK_H__
+#define __BNXT_NETLINK_H__
+
+#include <net/genetlink.h>
+#include <net/netlink.h>
+
+/* commands */
+enum {
+ BNXT_CMD_UNSPEC,
+ BNXT_CMD_HWRM,
+ BNXT_NUM_CMDS
+};
+#define BNXT_CMD_MAX (BNXT_NUM_CMDS - 1)
+
+/* attributes */
+enum {
+ BNXT_ATTR_UNSPEC,
+ BNXT_ATTR_PID,
+ BNXT_ATTR_IF_INDEX,
+ BNXT_ATTR_REQUEST,
+ BNXT_ATTR_RESPONSE,
+ BNXT_NUM_ATTRS
+};
+#define BNXT_ATTR_MAX (BNXT_NUM_ATTRS - 1)
+
+#define BNXT_NL_NAME "bnxt_netlink"
+#define BNXT_NL_VER 1
+
+int bnxt_nl_register(void);
+int bnxt_nl_unregister(void);
+
+#endif
--
1.8.3.1
On Fri, Aug 04, 2023 at 02:29:54PM -0700, Matthew Cover wrote:
> To leverage the SmartNIC capabilities available in Broadcom
> NetXtreme-C/E ethernet devices, representor pairs must be configured
> via bnxt-ctl
Could you give a link to the bnxt-ctl sources. Also give a brief
description of what they do.
> @@ -0,0 +1,231 @@
> +/* Broadcom NetXtreme-C/E network driver.
> + *
> + * Copyright (c) 2014-2016 Broadcom Corporation
> + * Copyright (c) 2016-2017 Broadcom Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation.
Please remove the license boilerplate and use a SPDX-License-Identifier.
> + */
> +#include <linux/netdevice.h>
> +#include <linux/pci.h>
> +#include "bnxt_hsi.h"
> +#include "bnxt_netlink.h"
> +#include "bnxt.h"
> +#include "bnxt_hwrm.h"
> +
> +/* attribute policy */
> +static struct nla_policy bnxt_netlink_policy[BNXT_NUM_ATTRS] = {
> + [BNXT_ATTR_PID] = { .type = NLA_U32 },
> + [BNXT_ATTR_IF_INDEX] = { .type = NLA_U32 },
> + [BNXT_ATTR_REQUEST] = { .type = NLA_BINARY },
> + [BNXT_ATTR_RESPONSE] = { .type = NLA_BINARY },
Passing binary blobs from user space to firmware will not be
accepted. You need well defined and documented individual commands.
Andrew
---
pw-bot: cr
On Fri, Aug 04, 2023 at 02:29:54PM -0700, Matthew Cover wrote:
...
Hi Matthew,
> +static int bnxt_parse_attrs(struct nlattr **a, struct bnxt **bp,
> + struct net_device **dev)
> +{
> + pid_t pid;
> + struct net *ns = NULL;
> + const char *drivername;
> +
> + if (!a[BNXT_ATTR_PID]) {
> + netdev_err(*dev, "No process ID specified\n");
> + goto err_inval;
> + }
> + pid = nla_get_u32(a[BNXT_ATTR_PID]);
> + ns = get_net_ns_by_pid(pid);
> + if (IS_ERR(ns)) {
> + netdev_err(*dev, "Invalid net namespace for pid %d (err: %ld)\n",
> + pid, PTR_ERR(ns));
> + goto err_inval;
> + }
> +
> + if (!a[BNXT_ATTR_IF_INDEX]) {
> + netdev_err(*dev, "No interface index specified\n");
> + goto err_inval;
> + }
> + *dev = dev_get_by_index(ns, nla_get_u32(a[BNXT_ATTR_IF_INDEX]));
> +
> + put_net(ns);
> + ns = NULL;
> + if (!*dev) {
> + netdev_err(*dev, "Invalid network interface index %d (err: %ld)\n",
> + nla_get_u32(a[BNXT_ATTR_IF_INDEX]), PTR_ERR(ns));
> + goto err_inval;
> + }
> + if (!(*dev)->dev.parent || !(*dev)->dev.parent->driver ||
> + !(*dev)->dev.parent->driver->name) {
> + netdev_err(*dev, "Unable to get driver name for device %s\n",
> + (*dev)->name);
> + goto err_inval;
> + }
> + drivername = (*dev)->dev.parent->driver->name;
> + if (strcmp(drivername, DRV_MODULE_NAME)) {
> + netdev_err(*dev, "Device %s (%s) is not a %s device!\n",
> + (*dev)->name, drivername, DRV_MODULE_NAME);
> + goto err_inval;
> + }
> + *bp = netdev_priv(*dev);
> + if (!*bp) {
We only get here if *bp is NULL.
But on the following line *bp is dereferenced.
Perhaps this should be netdev_warn(*dev, ...)
Flagged by Smatch.
> + netdev_warn((*bp)->dev, "No private data\n");
> + goto err_inval;
> + }
> +
> + return 0;
> +
> +err_inval:
> + if (ns && !IS_ERR(ns))
> + put_net(ns);
> + return -EINVAL;
> +}
> +
> +/* handler */
> +static int bnxt_netlink_hwrm(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct nlattr **a = info->attrs;
> + struct net_device *dev = NULL;
> + struct sk_buff *reply = NULL;
> + struct input *req, *nl_req;
> + struct bnxt *bp = NULL;
> + struct output *resp;
> + int len, rc;
> + void *hdr;
> +
> + rc = bnxt_parse_attrs(a, &bp, &dev);
> + if (rc)
> + goto err_rc;
> +
> + if (!bp) {
> + rc = -EINVAL;
> + goto err_rc;
> + }
> +
> + if (!bp->hwrm_dma_pool) {
> + netdev_warn(bp->dev, "HWRM interface not currently available on %s\n",
> + dev->name);
> + rc = -EINVAL;
> + goto err_rc;
> + }
> +
> + if (!a[BNXT_ATTR_REQUEST]) {
> + netdev_warn(bp->dev, "No request specified\n");
> + rc = -EINVAL;
> + goto err_rc;
> + }
> + len = nla_len(a[BNXT_ATTR_REQUEST]);
> + nl_req = nla_data(a[BNXT_ATTR_REQUEST]);
> +
> + reply = genlmsg_new(PAGE_SIZE, GFP_KERNEL);
> + if (!reply) {
> + netdev_warn(bp->dev, "Error: genlmsg_new failed\n");
> + rc = -ENOMEM;
> + goto err_rc;
> + }
> +
> + rc = hwrm_req_init(bp, req, nl_req->req_type);
hwrm_req_init() expects a variable of type u16 as it's type parameter.
But the tupe of nl_req->req_type is __le32.
As flagged by Sparse.
> + if (rc)
> + goto err_rc;
> +
> + rc = hwrm_req_replace(bp, req, nl_req, len);
> + if (rc)
> + goto err_rc;
> +
> + resp = hwrm_req_hold(bp, req);
> + rc = hwrm_req_send_silent(bp, req);
> + if (rc) {
> + /*
> + * Indicate success for the hwrm transport, while letting
> + * the hwrm error be passed back to the netlink caller in
> + * the response message. Caller is responsible for handling
> + * any errors.
> + *
> + * no kernel warnings are logged in this case.
> + */
> + rc = 0;
> + }
> + hdr = genlmsg_put_reply(reply, info, &bnxt_netlink_family, 0,
> + BNXT_CMD_HWRM);
> + if (nla_put(reply, BNXT_ATTR_RESPONSE, resp->resp_len, resp)) {
Likewise, the type of resp->resp_len is __le16, but an int is expected.
> + netdev_warn(bp->dev, "No space for response attribte\n");
> + hwrm_req_drop(bp, req);
> + rc = -ENOMEM;
> + goto err_rc;
> + }
> + genlmsg_end(reply, hdr);
> + hwrm_req_drop(bp, req);
> +
> + dev_put(dev);
> + dev = NULL;
> +
> + return genlmsg_reply(reply, info);
> +
> +err_rc:
> + if (reply && !IS_ERR(reply))
> + kfree_skb(reply);
> + if (dev && !IS_ERR(dev))
> + dev_put(dev);
> +
> + if (bp)
> + netdev_warn(bp->dev, "returning with error code %d\n", rc);
> +
> + return rc;
> +}
...
On Fri, Aug 4, 2023 at 2:30 PM Matthew Cover <[email protected]> wrote:
> ...
>
> This patch is a minimally modified port of the bnxt_netlink code
> found in out-of-tree bnxt_en-1.10.2-218.1.182.15.tar.gz. The
> out-of-tree driver contains both the GPL-2.0 in the file COPYING
> and a GPL comment in each source file.
This out-of-tree code to send FW commands from userspace using netlink
is never meant for upstream. Please do not apply this patch. Thanks.