[PATCH net-next] Add bnxt_netlink to facilitate representor pair configurations.

[PATCH net-next] Add bnxt_netlink to facilitate representor pair configurations.

[Matthew Cover]

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 <matthew.cover@stackpath.com>
---
 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

Re: [PATCH net-next] Add bnxt_netlink to facilitate representor pair configurations.

[Andrew Lunn]

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

Re: [PATCH net-next] Add bnxt_netlink to facilitate representor pair configurations.

[Simon Horman]

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;
> +}

...

Re: [PATCH net-next] Add bnxt_netlink to facilitate representor pair configurations.

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

On Fri, Aug 4, 2023 at 2:30 PM Matthew Cover <werekraken@gmail.com> 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.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]