Message-ID: <cmywoei5shisdjbt7ipv6rmfxx6jgafu2ccb4xr3phq3ealx3n@kxsdwd6u5bgk>
Date: Wed, 12 Feb 2025 16:09:19 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, anthony.l.nguyen@...el.com, 
	netdev@...r.kernel.org, horms@...nel.org, 
	Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Subject: Re: [PATCH iwl-next v3 02/14] ixgbe: add initial devlink support

Wed, Feb 12, 2025 at 02:14:01PM +0100, jedrzej.jagielski@...el.com wrote:
>Add an initial support for devlink interface to ixgbe driver.
>
>Similarly to i40e driver the implementation doesn't enable
>devlink to manage device-wide configuration. Devlink instance
>is created for each physical function of PCIe device.
>
>Create separate directory for devlink related ixgbe files
>and use naming scheme similar to the one used in the ice driver.
>
>Add a stub for Documentation, to be extended by further patches.
>
>Reviewed-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
>Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>---
>v2: fix error patch in probe; minor tweaks
>---
> Documentation/networking/devlink/index.rst    |  1 +
> Documentation/networking/devlink/ixgbe.rst    |  8 ++
> drivers/net/ethernet/intel/Kconfig            |  1 +
> drivers/net/ethernet/intel/ixgbe/Makefile     |  3 +-
> .../ethernet/intel/ixgbe/devlink/devlink.c    | 80 +++++++++++++++++++
> .../ethernet/intel/ixgbe/devlink/devlink.h    | 10 +++
> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  8 ++
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 22 +++++
> 8 files changed, 132 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/networking/devlink/ixgbe.rst
> create mode 100644 drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
> create mode 100644 drivers/net/ethernet/intel/ixgbe/devlink/devlink.h
>
>diff --git a/Documentation/networking/devlink/index.rst b/Documentation/networking/devlink/index.rst
>index 948c8c44e233..8319f43b5933 100644
>--- a/Documentation/networking/devlink/index.rst
>+++ b/Documentation/networking/devlink/index.rst
>@@ -84,6 +84,7 @@ parameters, info versions, and other features it supports.
>    i40e
>    ionic
>    ice
>+   ixgbe
>    mlx4
>    mlx5
>    mlxsw
>diff --git a/Documentation/networking/devlink/ixgbe.rst b/Documentation/networking/devlink/ixgbe.rst
>new file mode 100644
>index 000000000000..c04ac51c6d85
>--- /dev/null
>+++ b/Documentation/networking/devlink/ixgbe.rst
>@@ -0,0 +1,8 @@
>+.. SPDX-License-Identifier: GPL-2.0
>+
>+=====================
>+ixgbe devlink support
>+=====================
>+
>+This document describes the devlink features implemented by the ``ixgbe``
>+device driver.
>diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
>index 1640d2f27833..56ee58c9df21 100644
>--- a/drivers/net/ethernet/intel/Kconfig
>+++ b/drivers/net/ethernet/intel/Kconfig
>@@ -147,6 +147,7 @@ config IXGBE
> 	depends on PCI
> 	depends on PTP_1588_CLOCK_OPTIONAL
> 	select MDIO
>+	select NET_DEVLINK
> 	select PHYLIB
> 	help
> 	  This driver supports Intel(R) 10GbE PCI Express family of
>diff --git a/drivers/net/ethernet/intel/ixgbe/Makefile b/drivers/net/ethernet/intel/ixgbe/Makefile
>index b456d102655a..11f37140c0a3 100644
>--- a/drivers/net/ethernet/intel/ixgbe/Makefile
>+++ b/drivers/net/ethernet/intel/ixgbe/Makefile
>@@ -4,12 +4,13 @@
> # Makefile for the Intel(R) 10GbE PCI Express ethernet driver
> #
> 
>+subdir-ccflags-y += -I$(src)
> obj-$(CONFIG_IXGBE) += ixgbe.o
> 
> ixgbe-y := ixgbe_main.o ixgbe_common.o ixgbe_ethtool.o \
>            ixgbe_82599.o ixgbe_82598.o ixgbe_phy.o ixgbe_sriov.o \
>            ixgbe_mbx.o ixgbe_x540.o ixgbe_x550.o ixgbe_lib.o ixgbe_ptp.o \
>-           ixgbe_xsk.o ixgbe_e610.o
>+           ixgbe_xsk.o ixgbe_e610.o devlink/devlink.o
> 
> ixgbe-$(CONFIG_IXGBE_DCB) +=  ixgbe_dcb.o ixgbe_dcb_82598.o \
>                               ixgbe_dcb_82599.o ixgbe_dcb_nl.o
>diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>new file mode 100644
>index 000000000000..c052e95c9496
>--- /dev/null
>+++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>@@ -0,0 +1,80 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/* Copyright (c) 2025, Intel Corporation. */
>+
>+#include "ixgbe.h"
>+#include "devlink.h"
>+
>+static const struct devlink_ops ixgbe_devlink_ops = {
>+};
>+
>+/**
>+ * ixgbe_allocate_devlink - Allocate devlink instance
>+ * @adapter: pointer to the device adapter structure
>+ *
>+ * Allocate a devlink instance for this physical function.
>+ *
>+ * Return: 0 on success, -ENOMEM when allocation failed.
>+ */
>+int ixgbe_allocate_devlink(struct ixgbe_adapter *adapter)
>+{
>+	struct ixgbe_devlink_priv *devlink_private;
>+	struct device *dev = &adapter->pdev->dev;
>+	struct devlink *devlink;
>+
>+	devlink = devlink_alloc(&ixgbe_devlink_ops,
>+				sizeof(*devlink_private), dev);
>+	if (!devlink)
>+		return -ENOMEM;
>+
>+	devlink_private = devlink_priv(devlink);
>+	devlink_private->adapter = adapter;

struct ixgbe_adapter * should be returned by devlink_priv(), that is the
idea, to let devlink allocate the driver private for you.



>+
>+	adapter->devlink = devlink;
>+
>+	return 0;
>+}
>+
>+/**
>+ * ixgbe_devlink_set_switch_id - Set unique switch ID based on PCI DSN
>+ * @adapter: pointer to the device adapter structure
>+ * @ppid: struct with switch id information
>+ */
>+static void ixgbe_devlink_set_switch_id(struct ixgbe_adapter *adapter,
>+					struct netdev_phys_item_id *ppid)
>+{
>+	u64 id = pci_get_dsn(adapter->pdev);
>+
>+	ppid->id_len = sizeof(id);
>+	put_unaligned_be64(id, &ppid->id);
>+}
>+
>+/**
>+ * ixgbe_devlink_register_port - Register devlink port
>+ * @adapter: pointer to the device adapter structure
>+ *
>+ * Create and register a devlink_port for this physical function.
>+ *
>+ * Return: 0 on success, error code on failure.
>+ */
>+int ixgbe_devlink_register_port(struct ixgbe_adapter *adapter)
>+{
>+	struct devlink_port *devlink_port = &adapter->devlink_port;
>+	struct devlink *devlink = adapter->devlink;
>+	struct device *dev = &adapter->pdev->dev;
>+	struct devlink_port_attrs attrs = {};
>+	int err;
>+
>+	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
>+	attrs.phys.port_number = adapter->hw.bus.func;
>+	ixgbe_devlink_set_switch_id(adapter, &attrs.switch_id);
>+
>+	devlink_port_attrs_set(devlink_port, &attrs);
>+
>+	err = devl_port_register(devlink, devlink_port, 0);
>+	if (err) {
>+		dev_err(dev,
>+			"devlink port registration failed, err %d\n", err);
>+	}

Don't use "{}" for single statement.


>+
>+	return err;
>+}
>diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.h b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.h
>new file mode 100644
>index 000000000000..d73c57164aef
>--- /dev/null
>+++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.h
>@@ -0,0 +1,10 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+/* Copyright (c) 2025, Intel Corporation. */
>+
>+#ifndef _IXGBE_DEVLINK_H_
>+#define _IXGBE_DEVLINK_H_
>+
>+int ixgbe_allocate_devlink(struct ixgbe_adapter *adapter);
>+int ixgbe_devlink_register_port(struct ixgbe_adapter *adapter);
>+
>+#endif /* _IXGBE_DEVLINK_H_ */
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>index e6a380d4929b..37d761f8c409 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>@@ -17,6 +17,8 @@
> #include <linux/net_tstamp.h>
> #include <linux/ptp_clock_kernel.h>
> 
>+#include <net/devlink.h>
>+
> #include "ixgbe_type.h"
> #include "ixgbe_common.h"
> #include "ixgbe_dcb.h"
>@@ -612,6 +614,8 @@ struct ixgbe_adapter {
> 	struct bpf_prog *xdp_prog;
> 	struct pci_dev *pdev;
> 	struct mii_bus *mii_bus;
>+	struct devlink *devlink;
>+	struct devlink_port devlink_port;
> 
> 	unsigned long state;
> 
>@@ -830,6 +834,10 @@ struct ixgbe_adapter {
> 	spinlock_t vfs_lock;
> };
> 
>+struct ixgbe_devlink_priv {
>+	struct ixgbe_adapter *adapter;
>+};
>+
> static inline int ixgbe_determine_xdp_q_idx(int cpu)
> {
> 	if (static_key_enabled(&ixgbe_xdp_locking_key))
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>index 7236f20c9a30..1617ece95f1f 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>@@ -49,6 +49,7 @@
> #include "ixgbe_sriov.h"
> #include "ixgbe_model.h"
> #include "ixgbe_txrx_common.h"
>+#include "devlink/devlink.h"
> 
> char ixgbe_driver_name[] = "ixgbe";
> static const char ixgbe_driver_string[] =
>@@ -11275,6 +11276,10 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> 	hw->back = adapter;
> 	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
> 
>+	err = ixgbe_allocate_devlink(adapter);
>+	if (err)
>+		goto err_devlink;
>+
> 	hw->hw_addr = ioremap(pci_resource_start(pdev, 0),
> 			      pci_resource_len(pdev, 0));
> 	adapter->io_addr = hw->hw_addr;
>@@ -11613,6 +11618,11 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> 	}
> 	strcpy(netdev->name, "eth%d");
> 	pci_set_drvdata(pdev, adapter);
>+
>+	devl_lock(adapter->devlink);
>+	ixgbe_devlink_register_port(adapter);
>+	SET_NETDEV_DEVLINK_PORT(adapter->netdev, &adapter->devlink_port);
>+
> 	err = register_netdev(netdev);
> 	if (err)
> 		goto err_register;
>@@ -11667,11 +11677,15 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> 	if (err)
> 		goto err_netdev;
> 
>+	devl_register(adapter->devlink);
>+	devl_unlock(adapter->devlink);
> 	return 0;
> 
> err_netdev:
> 	unregister_netdev(netdev);
> err_register:
>+	devl_port_unregister(&adapter->devlink_port);
>+	devl_unlock(adapter->devlink);
> 	ixgbe_release_hw_control(adapter);
> 	ixgbe_clear_interrupt_scheme(adapter);
> 	if (hw->mac.type == ixgbe_mac_e610)
>@@ -11685,6 +11699,8 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> 	kfree(adapter->rss_key);
> 	bitmap_free(adapter->af_xdp_zc_qps);
> err_ioremap:
>+	devlink_free(adapter->devlink);
>+err_devlink:
> 	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter->state);
> 	free_netdev(netdev);
> err_alloc_etherdev:
>@@ -11717,6 +11733,8 @@ static void ixgbe_remove(struct pci_dev *pdev)
> 		return;
> 
> 	netdev  = adapter->netdev;
>+	devl_lock(adapter->devlink);
>+	devl_unregister(adapter->devlink);
> 	ixgbe_dbg_adapter_exit(adapter);
> 
> 	set_bit(__IXGBE_REMOVING, &adapter->state);
>@@ -11752,6 +11770,10 @@ static void ixgbe_remove(struct pci_dev *pdev)
> 	if (netdev->reg_state == NETREG_REGISTERED)
> 		unregister_netdev(netdev);
> 
>+	devl_port_unregister(&adapter->devlink_port);
>+	devl_unlock(adapter->devlink);
>+	devlink_free(adapter->devlink);
>+
> 	ixgbe_stop_ipsec_offload(adapter);
> 	ixgbe_clear_interrupt_scheme(adapter);
> 
>-- 
>2.31.1
>

Powered by blists - more mailing lists