Message-ID: <DS0PR11MB7785E7F829BFD1A59AF16048F0FC2@DS0PR11MB7785.namprd11.prod.outlook.com>
Date: Wed, 12 Feb 2025 15:47:42 +0000
From: "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "horms@...nel.org" <horms@...nel.org>, "Polchlopek,
 Mateusz" <mateusz.polchlopek@...el.com>
Subject: RE: [PATCH iwl-next v3 02/14] ixgbe: add initial devlink support

From: Jiri Pirko <jiri@...nulli.us> 
Sent: Wednesday, February 12, 2025 4:09 PM
>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.

Using ixgbe_devlink_priv here is a workaround which i decided to introduce
to mitigate the fact that ixgbe_adapter is used to alloc netdev with
alloc_etherdev_mq().
This would require general ixgbe refactoring.


>
>
>
>>+
>>+	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.

Sure, will be changed.
>
>
>>+
>>+	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