[PATCH net-next 0/7] sfc: initial debugfs support

[PATCH net-next 0/7] sfc: initial debugfs support

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Expose some basic information about the NIC, its channels and queues
 and their assignments.

Edward Cree (7):
  sfc: initial debugfs implementation
  sfc: debugfs for channels
  sfc: debugfs for (nic) RX queues
  sfc: debugfs for (nic) TX queues
  sfc: add debugfs nodes for loopback mode
  sfc: add debugfs entries for filter table status
  sfc: add debugfs node for filter table contents

 drivers/net/ethernet/sfc/Makefile       |   1 +
 drivers/net/ethernet/sfc/debugfs.c      | 539 ++++++++++++++++++++++++
 drivers/net/ethernet/sfc/debugfs.h      | 149 +++++++
 drivers/net/ethernet/sfc/ef10.c         |  10 +
 drivers/net/ethernet/sfc/ef100_netdev.c |   7 +
 drivers/net/ethernet/sfc/ef100_nic.c    |   8 +
 drivers/net/ethernet/sfc/efx.c          |  15 +-
 drivers/net/ethernet/sfc/efx_channels.c |   8 +
 drivers/net/ethernet/sfc/efx_common.c   |   7 +
 drivers/net/ethernet/sfc/mcdi_filters.c |  57 +++
 drivers/net/ethernet/sfc/mcdi_filters.h |   4 +
 drivers/net/ethernet/sfc/net_driver.h   |  47 +++
 drivers/net/ethernet/sfc/rx_common.c    |   9 +
 drivers/net/ethernet/sfc/tx_common.c    |   8 +
 14 files changed, 868 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/sfc/debugfs.c
 create mode 100644 drivers/net/ethernet/sfc/debugfs.h

[PATCH net-next 1/7] sfc: initial debugfs implementation

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Just a handful of nodes, including one enum with a string table for
 pretty printing the values.

Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/Makefile       |   1 +
 drivers/net/ethernet/sfc/debugfs.c      | 234 ++++++++++++++++++++++++
 drivers/net/ethernet/sfc/debugfs.h      |  56 ++++++
 drivers/net/ethernet/sfc/ef10.c         |  10 +
 drivers/net/ethernet/sfc/ef100_netdev.c |   7 +
 drivers/net/ethernet/sfc/ef100_nic.c    |   8 +
 drivers/net/ethernet/sfc/efx.c          |  15 +-
 drivers/net/ethernet/sfc/efx_common.c   |   7 +
 drivers/net/ethernet/sfc/net_driver.h   |  29 +++
 9 files changed, 366 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/sfc/debugfs.c
 create mode 100644 drivers/net/ethernet/sfc/debugfs.h

diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile
index 8f446b9bd5ee..1fbdd04dc2c5 100644
--- a/drivers/net/ethernet/sfc/Makefile
+++ b/drivers/net/ethernet/sfc/Makefile
@@ -12,6 +12,7 @@ sfc-$(CONFIG_SFC_MTD)	+= mtd.o
 sfc-$(CONFIG_SFC_SRIOV)	+= sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \
                            mae.o tc.o tc_bindings.o tc_counters.o \
                            tc_encap_actions.o tc_conntrack.o
+sfc-$(CONFIG_DEBUG_FS)	+= debugfs.o
 
 obj-$(CONFIG_SFC)	+= sfc.o
 
diff --git a/drivers/net/ethernet/sfc/debugfs.c b/drivers/net/ethernet/sfc/debugfs.c
new file mode 100644
index 000000000000..cf800addb4ff
--- /dev/null
+++ b/drivers/net/ethernet/sfc/debugfs.c
@@ -0,0 +1,234 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/****************************************************************************
+ * Driver for Solarflare network controllers and boards
+ * Copyright 2023, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation, incorporated herein by reference.
+ */
+
+#include "debugfs.h"
+#include <linux/module.h>
+#include <linux/debugfs.h>
+#include <linux/dcache.h>
+#include <linux/seq_file.h>
+
+/* Maximum length for a name component or symlink target */
+#define EFX_DEBUGFS_NAME_LEN 32
+
+/* Top-level debug directory ([/sys/kernel]/debug/sfc) */
+static struct dentry *efx_debug_root;
+
+/* "cards" directory ([/sys/kernel]/debug/sfc/cards) */
+static struct dentry *efx_debug_cards;
+
+/**
+ * efx_init_debugfs_netdev - create debugfs sym-link for net device
+ * @net_dev:		Net device
+ *
+ * Create sym-link named after @net_dev to the debugfs directories for the
+ * corresponding NIC.  The sym-link must be cleaned up using
+ * efx_fini_debugfs_netdev().
+ *
+ * Return: a negative error code or 0 on success.
+ */
+static int efx_init_debugfs_netdev(struct net_device *net_dev)
+{
+	struct efx_nic *efx = efx_netdev_priv(net_dev);
+	char target[EFX_DEBUGFS_NAME_LEN];
+	char name[EFX_DEBUGFS_NAME_LEN];
+
+	if (snprintf(name, sizeof(name), "nic_%s", net_dev->name) >=
+			sizeof(name))
+		return -ENAMETOOLONG;
+	if (snprintf(target, sizeof(target), "cards/%s", pci_name(efx->pci_dev))
+	    >= sizeof(target))
+		return -ENAMETOOLONG;
+	efx->debug_symlink = debugfs_create_symlink(name,
+						    efx_debug_root, target);
+	if (IS_ERR_OR_NULL(efx->debug_symlink))
+		return efx->debug_symlink ? PTR_ERR(efx->debug_symlink) :
+					    -ENOMEM;
+
+	return 0;
+}
+
+/**
+ * efx_fini_debugfs_netdev - remove debugfs sym-link for net device
+ * @net_dev:		Net device
+ *
+ * Remove sym-link created for @net_dev by efx_init_debugfs_netdev().
+ */
+void efx_fini_debugfs_netdev(struct net_device *net_dev)
+{
+	struct efx_nic *efx = efx_netdev_priv(net_dev);
+
+	debugfs_remove(efx->debug_symlink);
+	efx->debug_symlink = NULL;
+}
+
+/* replace debugfs sym-links on net device rename */
+void efx_update_debugfs_netdev(struct efx_nic *efx)
+{
+	mutex_lock(&efx->debugfs_symlink_mutex);
+	if (efx->debug_symlink)
+		efx_fini_debugfs_netdev(efx->net_dev);
+	efx_init_debugfs_netdev(efx->net_dev);
+	mutex_unlock(&efx->debugfs_symlink_mutex);
+}
+
+static int efx_debugfs_enum_read(struct seq_file *s, void *d)
+{
+	struct efx_debugfs_enum_data *data = s->private;
+	u64 value = 0;
+	size_t len;
+
+	len = min(data->vlen, sizeof(value));
+#ifdef BIG_ENDIAN
+	/* If data->value is narrower than u64, we need to copy into the
+	 * far end of value, as that's where the low bits live.
+	 */
+	memcpy(((void *)&value) + sizeof(value) - len, data->value, len);
+#else
+	memcpy(&value, data->value, len);
+#endif
+	seq_printf(s, "%#llx => %s\n", value,
+		   value < data->max ? data->names[value] : "(invalid)");
+	return 0;
+}
+
+static int efx_debugfs_enum_open(struct inode *inode, struct file *f)
+{
+	struct efx_debugfs_enum_data *data = inode->i_private;
+
+	return single_open(f, efx_debugfs_enum_read, data);
+}
+
+static const struct file_operations efx_debugfs_enum_ops = {
+	.owner = THIS_MODULE,
+	.open = efx_debugfs_enum_open,
+	.release = single_release,
+	.read = seq_read,
+	.llseek = seq_lseek,
+};
+
+static void efx_debugfs_create_enum(const char *name, umode_t mode,
+				    struct dentry *parent,
+				    struct efx_debugfs_enum_data *data)
+{
+	debugfs_create_file(name, mode, parent, data, &efx_debugfs_enum_ops);
+}
+
+static const char *const efx_interrupt_mode_names[] = {
+	[EFX_INT_MODE_MSIX]   = "MSI-X",
+	[EFX_INT_MODE_MSI]    = "MSI",
+	[EFX_INT_MODE_LEGACY] = "legacy",
+};
+
+#define EFX_DEBUGFS_EFX(_type, _name)	\
+	debugfs_create_##_type(#_name, 0444, efx->debug_dir, &efx->_name)
+
+/* Create basic debugfs parameter files for an Efx NIC */
+static void efx_init_debugfs_nic_files(struct efx_nic *efx)
+{
+	EFX_DEBUGFS_EFX(x32, rx_dma_len);
+	EFX_DEBUGFS_EFX(x32, rx_buffer_order);
+	EFX_DEBUGFS_EFX(x32, rx_buffer_truesize);
+	efx->debug_interrupt_mode.max = ARRAY_SIZE(efx_interrupt_mode_names);
+	efx->debug_interrupt_mode.names = efx_interrupt_mode_names;
+	efx->debug_interrupt_mode.vlen = sizeof(efx->interrupt_mode);
+	efx->debug_interrupt_mode.value = &efx->interrupt_mode;
+	efx_debugfs_create_enum("interrupt_mode", 0444, efx->debug_dir,
+				&efx->debug_interrupt_mode);
+}
+
+/**
+ * efx_init_debugfs_nic - create debugfs directory for NIC
+ * @efx:		Efx NIC
+ *
+ * Create debugfs directory containing parameter-files for @efx.
+ * The directory must be cleaned up using efx_fini_debugfs_nic().
+ *
+ * Return: a negative error code or 0 on success.
+ */
+int efx_init_debugfs_nic(struct efx_nic *efx)
+{
+	/* Create directory */
+	efx->debug_dir = debugfs_create_dir(pci_name(efx->pci_dev),
+					    efx_debug_cards);
+	if (!efx->debug_dir)
+		return -ENOMEM;
+	efx_init_debugfs_nic_files(efx);
+	return 0;
+}
+
+/**
+ * efx_fini_debugfs_nic - remove debugfs directory for NIC
+ * @efx:		Efx NIC
+ *
+ * Remove debugfs directory created for @efx by efx_init_debugfs_nic().
+ */
+void efx_fini_debugfs_nic(struct efx_nic *efx)
+{
+	debugfs_remove_recursive(efx->debug_dir);
+	efx->debug_dir = NULL;
+}
+
+/**
+ * efx_init_debugfs - create debugfs directories for sfc driver
+ *
+ * Create debugfs directories "sfc" and "sfc/cards".  This must be
+ * called before any of the other functions that create debugfs
+ * directories.  The directories must be cleaned up using
+ * efx_fini_debugfs().
+ *
+ * Return: a negative error code or 0 on success.
+ */
+int efx_init_debugfs(void)
+{
+	int rc;
+
+	/* Create top-level directory */
+	efx_debug_root = debugfs_create_dir(KBUILD_MODNAME, NULL);
+	if (!efx_debug_root) {
+		pr_err("debugfs_create_dir %s failed.\n", KBUILD_MODNAME);
+		rc = -ENOMEM;
+		goto err;
+	} else if (IS_ERR(efx_debug_root)) {
+		rc = PTR_ERR(efx_debug_root);
+		pr_err("debugfs_create_dir %s failed, rc=%d.\n",
+		       KBUILD_MODNAME, rc);
+		goto err;
+	}
+
+	/* Create "cards" directory */
+	efx_debug_cards = debugfs_create_dir("cards", efx_debug_root);
+	if (!efx_debug_cards) {
+		pr_err("debugfs_create_dir cards failed.\n");
+		rc = -ENOMEM;
+		goto err;
+	} else if (IS_ERR(efx_debug_cards)) {
+		rc = PTR_ERR(efx_debug_cards);
+		pr_err("debugfs_create_dir cards failed, rc=%d.\n", rc);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	efx_fini_debugfs();
+	return rc;
+}
+
+/**
+ * efx_fini_debugfs - remove debugfs directories for sfc driver
+ *
+ * Remove directories created by efx_init_debugfs().
+ */
+void efx_fini_debugfs(void)
+{
+	debugfs_remove_recursive(efx_debug_root);
+	efx_debug_cards = NULL;
+	efx_debug_root = NULL;
+}
diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h
new file mode 100644
index 000000000000..1fe40fbffa5e
--- /dev/null
+++ b/drivers/net/ethernet/sfc/debugfs.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/****************************************************************************
+ * Driver for Solarflare network controllers and boards
+ * Copyright 2023, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation, incorporated herein by reference.
+ */
+
+#ifndef EFX_DEBUGFS_H
+#define EFX_DEBUGFS_H
+#include "net_driver.h"
+
+#ifdef CONFIG_DEBUG_FS
+
+/**
+ * DOC: Directory layout for sfc debugfs
+ *
+ * At top level ([/sys/kernel]/debug/sfc) are per-netdev symlinks "nic_$name"
+ * and the "cards" directory.  For each PCI device to which the driver has
+ * bound and created a &struct efx_nic, there is a directory &efx_nic.debug_dir
+ * in "cards" whose name is the PCI address of the device; it is to this
+ * directory that the netdev symlink points.
+ */
+
+void efx_fini_debugfs_netdev(struct net_device *net_dev);
+void efx_update_debugfs_netdev(struct efx_nic *efx);
+
+int efx_init_debugfs_nic(struct efx_nic *efx);
+void efx_fini_debugfs_nic(struct efx_nic *efx);
+
+int efx_init_debugfs(void);
+void efx_fini_debugfs(void);
+
+#else /* CONFIG_DEBUG_FS */
+
+static inline void efx_fini_debugfs_netdev(struct net_device *net_dev) {}
+
+static inline void efx_update_debugfs_netdev(struct efx_nic *efx) {}
+
+static inline int efx_init_debugfs_nic(struct efx_nic *efx)
+{
+	return 0;
+}
+static inline void efx_fini_debugfs_nic(struct efx_nic *efx) {}
+
+static inline int efx_init_debugfs(void)
+{
+	return 0;
+}
+static inline void efx_fini_debugfs(void) {}
+
+#endif /* CONFIG_DEBUG_FS */
+
+#endif /* EFX_DEBUGFS_H */
diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 6dfa062feebc..58e18fc92093 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -19,6 +19,7 @@
 #include "workarounds.h"
 #include "selftest.h"
 #include "ef10_sriov.h"
+#include "debugfs.h"
 #include <linux/in.h>
 #include <linux/jhash.h>
 #include <linux/wait.h>
@@ -580,6 +581,13 @@ static int efx_ef10_probe(struct efx_nic *efx)
 	if (rc)
 		goto fail3;
 
+	/* Populate debugfs */
+#ifdef CONFIG_DEBUG_FS
+	rc = efx_init_debugfs_nic(efx);
+	if (rc)
+		pci_err(efx->pci_dev, "failed to init device debugfs\n");
+#endif
+
 	rc = device_create_file(&efx->pci_dev->dev,
 				&dev_attr_link_control_flag);
 	if (rc)
@@ -693,6 +701,7 @@ static int efx_ef10_probe(struct efx_nic *efx)
 fail4:
 	device_remove_file(&efx->pci_dev->dev, &dev_attr_link_control_flag);
 fail3:
+	efx_fini_debugfs_nic(efx);
 	efx_mcdi_detach(efx);
 
 	mutex_lock(&nic_data->udp_tunnels_lock);
@@ -962,6 +971,7 @@ static void efx_ef10_remove(struct efx_nic *efx)
 	device_remove_file(&efx->pci_dev->dev, &dev_attr_link_control_flag);
 
 	efx_mcdi_detach(efx);
+	efx_fini_debugfs_nic(efx);
 
 	memset(nic_data->udp_tunnels, 0, sizeof(nic_data->udp_tunnels));
 	mutex_lock(&nic_data->udp_tunnels_lock);
diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
index 7f7d560cb2b4..e844d57754b7 100644
--- a/drivers/net/ethernet/sfc/ef100_netdev.c
+++ b/drivers/net/ethernet/sfc/ef100_netdev.c
@@ -26,10 +26,12 @@
 #include "tc_bindings.h"
 #include "tc_encap_actions.h"
 #include "efx_devlink.h"
+#include "debugfs.h"
 
 static void ef100_update_name(struct efx_nic *efx)
 {
 	strcpy(efx->name, efx->net_dev->name);
+	efx_update_debugfs_netdev(efx);
 }
 
 static int ef100_alloc_vis(struct efx_nic *efx, unsigned int *allocated_vis)
@@ -405,6 +407,11 @@ void ef100_remove_netdev(struct efx_probe_data *probe_data)
 	ef100_pf_unset_devlink_port(efx);
 	efx_fini_tc(efx);
 #endif
+#ifdef CONFIG_DEBUG_FS
+	mutex_lock(&efx->debugfs_symlink_mutex);
+	efx_fini_debugfs_netdev(efx->net_dev);
+	mutex_unlock(&efx->debugfs_symlink_mutex);
+#endif
 
 	down_write(&efx->filter_sem);
 	efx_mcdi_filter_table_remove(efx);
diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
index 6da06931187d..ad378aa05dc3 100644
--- a/drivers/net/ethernet/sfc/ef100_nic.c
+++ b/drivers/net/ethernet/sfc/ef100_nic.c
@@ -27,6 +27,7 @@
 #include "tc.h"
 #include "mae.h"
 #include "rx_common.h"
+#include "debugfs.h"
 
 #define EF100_MAX_VIS 4096
 #define EF100_NUM_MCDI_BUFFERS	1
@@ -1077,6 +1078,12 @@ static int ef100_probe_main(struct efx_nic *efx)
 
 	/* Post-IO section. */
 
+	/* Populate debugfs */
+#ifdef CONFIG_DEBUG_FS
+	rc = efx_init_debugfs_nic(efx);
+	if (rc)
+		pci_err(efx->pci_dev, "failed to init device debugfs\n");
+#endif
 	rc = efx_mcdi_init(efx);
 	if (rc)
 		goto fail;
@@ -1213,6 +1220,7 @@ void ef100_remove(struct efx_nic *efx)
 
 	efx_mcdi_detach(efx);
 	efx_mcdi_fini(efx);
+	efx_fini_debugfs_nic(efx);
 	if (nic_data)
 		efx_nic_free_buffer(efx, &nic_data->mcdi_buf);
 	kfree(nic_data);
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 19f4b4d0b851..9266c7b5b4fd 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -33,6 +33,7 @@
 #include "selftest.h"
 #include "sriov.h"
 #include "efx_devlink.h"
+#include "debugfs.h"
 
 #include "mcdi_port_common.h"
 #include "mcdi_pcol.h"
@@ -401,6 +402,11 @@ static void efx_remove_all(struct efx_nic *efx)
 #endif
 	efx_remove_port(efx);
 	efx_remove_nic(efx);
+#ifdef CONFIG_DEBUG_FS
+	mutex_lock(&efx->debugfs_symlink_mutex);
+	efx_fini_debugfs_netdev(efx->net_dev);
+	mutex_unlock(&efx->debugfs_symlink_mutex);
+#endif
 }
 
 /**************************************************************************
@@ -667,6 +673,7 @@ static void efx_update_name(struct efx_nic *efx)
 	strcpy(efx->name, efx->net_dev->name);
 	efx_mtd_rename(efx);
 	efx_set_channel_names(efx);
+	efx_update_debugfs_netdev(efx);
 }
 
 static int efx_netdev_event(struct notifier_block *this,
@@ -1319,6 +1326,10 @@ static int __init efx_init_module(void)
 
 	printk(KERN_INFO "Solarflare NET driver\n");
 
+	rc = efx_init_debugfs();
+	if (rc)
+		goto err_debugfs;
+
 	rc = register_netdevice_notifier(&efx_netdev_notifier);
 	if (rc)
 		goto err_notifier;
@@ -1344,6 +1355,8 @@ static int __init efx_init_module(void)
  err_reset:
 	unregister_netdevice_notifier(&efx_netdev_notifier);
  err_notifier:
+	efx_fini_debugfs();
+ err_debugfs:
 	return rc;
 }
 
@@ -1355,7 +1368,7 @@ static void __exit efx_exit_module(void)
 	pci_unregister_driver(&efx_pci_driver);
 	efx_destroy_reset_workqueue();
 	unregister_netdevice_notifier(&efx_netdev_notifier);
-
+	efx_fini_debugfs();
 }
 
 module_init(efx_init_module);
diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
index 175bd9cdfdac..7a9d6b6b66e5 100644
--- a/drivers/net/ethernet/sfc/efx_common.c
+++ b/drivers/net/ethernet/sfc/efx_common.c
@@ -1022,6 +1022,9 @@ int efx_init_struct(struct efx_nic *efx, struct pci_dev *pci_dev)
 	INIT_WORK(&efx->mac_work, efx_mac_work);
 	init_waitqueue_head(&efx->flush_wq);
 
+#ifdef CONFIG_DEBUG_FS
+	mutex_init(&efx->debugfs_symlink_mutex);
+#endif
 	efx->tx_queues_per_channel = 1;
 	efx->rxq_entries = EFX_DEFAULT_DMAQ_SIZE;
 	efx->txq_entries = EFX_DEFAULT_DMAQ_SIZE;
@@ -1056,6 +1059,10 @@ void efx_fini_struct(struct efx_nic *efx)
 
 	efx_fini_channels(efx);
 
+#ifdef CONFIG_DEBUG_FS
+	mutex_destroy(&efx->debugfs_symlink_mutex);
+#endif
+
 	kfree(efx->vpd_sn);
 
 	if (efx->workqueue) {
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 27d86e90a3bb..961e2db31c6e 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -107,6 +107,24 @@ struct hwtstamp_config;
 
 struct efx_self_tests;
 
+/**
+ * struct efx_debugfs_enum_data - information for pretty-printing enums
+ * @value: pointer to the actual enum
+ * @vlen: sizeof the enum
+ * @names: array of names of enumerated values.  May contain some %NULL entries.
+ * @max: number of entries in @names, typically from ARRAY_SIZE()
+ *
+ * Where a driver struct contains an enum member which we wish to expose in
+ * debugfs, we also embed an instance of this struct, which
+ * efx_debugfs_enum_read() uses to pretty-print the value.
+ */
+struct efx_debugfs_enum_data {
+	void *value;
+	size_t vlen;
+	const char *const *names;
+	unsigned int max;
+};
+
 /**
  * struct efx_buffer - A general-purpose DMA buffer
  * @addr: host base address of the buffer
@@ -1123,6 +1141,17 @@ struct efx_nic {
 	u32 rps_next_id;
 #endif
 
+#ifdef CONFIG_DEBUG_FS
+	/** @debug_dir: NIC debugfs directory */
+	struct dentry *debug_dir;
+	/** @debug_symlink: NIC debugfs symlink (``nic_eth%d``) */
+	struct dentry *debug_symlink;
+	/** @debug_interrupt_mode: debugfs details for printing @interrupt_mode */
+	struct efx_debugfs_enum_data debug_interrupt_mode;
+	/** @debugfs_symlink_mutex: protects debugfs @debug_symlink */
+	struct mutex debugfs_symlink_mutex;
+#endif
+
 	atomic_t active_queues;
 	atomic_t rxq_flush_pending;
 	atomic_t rxq_flush_outstanding;

[PATCH net-next 2/7] sfc: debugfs for channels

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Expose each channel's IRQ number and EVQ read pointer.

Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/debugfs.c      | 51 +++++++++++++++++++++++++
 drivers/net/ethernet/sfc/debugfs.h      | 15 ++++++++
 drivers/net/ethernet/sfc/efx_channels.c |  8 ++++
 drivers/net/ethernet/sfc/net_driver.h   |  6 +++
 4 files changed, 80 insertions(+)

diff --git a/drivers/net/ethernet/sfc/debugfs.c b/drivers/net/ethernet/sfc/debugfs.c
index cf800addb4ff..b46339249794 100644
--- a/drivers/net/ethernet/sfc/debugfs.c
+++ b/drivers/net/ethernet/sfc/debugfs.c
@@ -78,6 +78,54 @@ void efx_update_debugfs_netdev(struct efx_nic *efx)
 	mutex_unlock(&efx->debugfs_symlink_mutex);
 }
 
+#define EFX_DEBUGFS_CHANNEL(_type, _name)	\
+	debugfs_create_##_type(#_name, 0444, channel->debug_dir, &channel->_name)
+
+/* Create basic debugfs parameter files for an Efx channel */
+static void efx_init_debugfs_channel_files(struct efx_channel *channel)
+{
+	EFX_DEBUGFS_CHANNEL(bool, enabled);
+	EFX_DEBUGFS_CHANNEL(u32, irq); /* actually an int */
+	EFX_DEBUGFS_CHANNEL(u32, eventq_read_ptr);
+}
+
+/**
+ * efx_init_debugfs_channel - create debugfs directory for channel
+ * @channel:		Efx channel
+ *
+ * Create a debugfs directory containing parameter-files for @channel.
+ * The directory must be cleaned up using efx_fini_debugfs_channel().
+ *
+ * Return: a negative error code or 0 on success.
+ */
+int efx_init_debugfs_channel(struct efx_channel *channel)
+{
+	char name[EFX_DEBUGFS_NAME_LEN];
+
+	if (!channel->efx->debug_channels_dir)
+		return -ENODEV;
+	if (snprintf(name, sizeof(name), "%d", channel->channel)
+	    >= sizeof(name))
+		return -ENAMETOOLONG;
+	channel->debug_dir = debugfs_create_dir(name, channel->efx->debug_channels_dir);
+	if (!channel->debug_dir)
+		return -ENOMEM;
+	efx_init_debugfs_channel_files(channel);
+	return 0;
+}
+
+/**
+ * efx_fini_debugfs_channel - remove debugfs directory for channel
+ * @channel:		Efx channel
+ *
+ * Remove directory created for @channel by efx_init_debugfs_channel().
+ */
+void efx_fini_debugfs_channel(struct efx_channel *channel)
+{
+	debugfs_remove_recursive(channel->debug_dir);
+	channel->debug_dir = NULL;
+}
+
 static int efx_debugfs_enum_read(struct seq_file *s, void *d)
 {
 	struct efx_debugfs_enum_data *data = s->private;
@@ -160,6 +208,9 @@ int efx_init_debugfs_nic(struct efx_nic *efx)
 	if (!efx->debug_dir)
 		return -ENOMEM;
 	efx_init_debugfs_nic_files(efx);
+	/* Create channels subdirectory */
+	efx->debug_channels_dir = debugfs_create_dir("channels",
+						     efx->debug_dir);
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h
index 1fe40fbffa5e..4af4a03d1b97 100644
--- a/drivers/net/ethernet/sfc/debugfs.h
+++ b/drivers/net/ethernet/sfc/debugfs.h
@@ -22,11 +22,20 @@
  * bound and created a &struct efx_nic, there is a directory &efx_nic.debug_dir
  * in "cards" whose name is the PCI address of the device; it is to this
  * directory that the netdev symlink points.
+ *
+ * Under this directory, besides top-level parameter files, are:
+ *
+ * * "channels/" (&efx_nic.debug_channels_dir).  For each channel, this will
+ *   contain a directory (&efx_channel.debug_dir), whose name is the channel
+ *   index (in decimal).
  */
 
 void efx_fini_debugfs_netdev(struct net_device *net_dev);
 void efx_update_debugfs_netdev(struct efx_nic *efx);
 
+int efx_init_debugfs_channel(struct efx_channel *channel);
+void efx_fini_debugfs_channel(struct efx_channel *channel);
+
 int efx_init_debugfs_nic(struct efx_nic *efx);
 void efx_fini_debugfs_nic(struct efx_nic *efx);
 
@@ -39,6 +48,12 @@ static inline void efx_fini_debugfs_netdev(struct net_device *net_dev) {}
 
 static inline void efx_update_debugfs_netdev(struct efx_nic *efx) {}
 
+int efx_init_debugfs_channel(struct efx_channel *channel)
+{
+	return 0;
+}
+void efx_fini_debugfs_channel(struct efx_channel *channel) {}
+
 static inline int efx_init_debugfs_nic(struct efx_nic *efx)
 {
 	return 0;
diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
index c9e17a8208a9..804a25ceea7f 100644
--- a/drivers/net/ethernet/sfc/efx_channels.c
+++ b/drivers/net/ethernet/sfc/efx_channels.c
@@ -19,6 +19,7 @@
 #include "nic.h"
 #include "sriov.h"
 #include "workarounds.h"
+#include "debugfs.h"
 
 /* This is the first interrupt mode to try out of:
  * 0 => MSI-X
@@ -667,6 +668,12 @@ static int efx_probe_channel(struct efx_channel *channel)
 
 	channel->rx_list = NULL;
 
+	rc = efx_init_debugfs_channel(channel);
+	if (rc) /* not fatal */
+		netif_err(channel->efx, drv, channel->efx->net_dev,
+			  "Failed to create debugfs for channel %d, rc=%d\n",
+			  channel->channel, rc);
+
 	return 0;
 
 fail:
@@ -743,6 +750,7 @@ void efx_remove_channel(struct efx_channel *channel)
 
 	netif_dbg(channel->efx, drv, channel->efx->net_dev,
 		  "destroy chan %d\n", channel->channel);
+	efx_fini_debugfs_channel(channel);
 
 	efx_for_each_channel_rx_queue(rx_queue, channel)
 		efx_remove_rx_queue(rx_queue);
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 961e2db31c6e..2b92c5461fe3 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -528,6 +528,10 @@ struct efx_channel {
 #define RPS_FLOW_ID_INVALID 0xFFFFFFFF
 	u32 *rps_flow_id;
 #endif
+#ifdef CONFIG_DEBUG_FS
+	/** @debug_dir: Channel debugfs directory (under @efx->debug_channels_dir) */
+	struct dentry *debug_dir;
+#endif
 
 	unsigned int n_rx_tobe_disc;
 	unsigned int n_rx_ip_hdr_chksum_err;
@@ -1144,6 +1148,8 @@ struct efx_nic {
 #ifdef CONFIG_DEBUG_FS
 	/** @debug_dir: NIC debugfs directory */
 	struct dentry *debug_dir;
+	/** @debug_channels_dir: contains channel debugfs dirs.  Under @debug_dir */
+	struct dentry *debug_channels_dir;
 	/** @debug_symlink: NIC debugfs symlink (``nic_eth%d``) */
 	struct dentry *debug_symlink;
 	/** @debug_interrupt_mode: debugfs details for printing @interrupt_mode */

[PATCH net-next 3/7] sfc: debugfs for (nic) RX queues

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Expose each RX queue's core RXQ association, and the read/write/etc
 pointers for the descriptor ring.
Each RXQ dir also symlinks to its owning channel.

Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/debugfs.c    | 69 ++++++++++++++++++++++++++-
 drivers/net/ethernet/sfc/debugfs.h    | 15 ++++++
 drivers/net/ethernet/sfc/net_driver.h |  6 +++
 drivers/net/ethernet/sfc/rx_common.c  |  9 ++++
 4 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sfc/debugfs.c b/drivers/net/ethernet/sfc/debugfs.c
index b46339249794..43b1d06a985e 100644
--- a/drivers/net/ethernet/sfc/debugfs.c
+++ b/drivers/net/ethernet/sfc/debugfs.c
@@ -78,6 +78,72 @@ void efx_update_debugfs_netdev(struct efx_nic *efx)
 	mutex_unlock(&efx->debugfs_symlink_mutex);
 }
 
+#define EFX_DEBUGFS_RXQ(_type, _name)	\
+	debugfs_create_##_type(#_name, 0444, rx_queue->debug_dir, &rx_queue->_name)
+
+/* Create basic debugfs parameter files for an Efx RXQ */
+static void efx_init_debugfs_rx_queue_files(struct efx_rx_queue *rx_queue)
+{
+	EFX_DEBUGFS_RXQ(u32, core_index); /* actually an int */
+	/* descriptor ring indices */
+	EFX_DEBUGFS_RXQ(u32, added_count);
+	EFX_DEBUGFS_RXQ(u32, notified_count);
+	EFX_DEBUGFS_RXQ(u32, granted_count);
+	EFX_DEBUGFS_RXQ(u32, removed_count);
+}
+
+/**
+ * efx_init_debugfs_rx_queue - create debugfs directory for RX queue
+ * @rx_queue:		Efx RX queue
+ *
+ * Create a debugfs directory containing parameter-files for @rx_queue.
+ * The directory must be cleaned up using efx_fini_debugfs_rx_queue(),
+ * even if this function returns an error.
+ *
+ * Return: a negative error code or 0 on success.
+ */
+int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue)
+{
+	struct efx_channel *channel = efx_rx_queue_channel(rx_queue);
+	char target[EFX_DEBUGFS_NAME_LEN];
+	char name[EFX_DEBUGFS_NAME_LEN];
+
+	if (!rx_queue->efx->debug_queues_dir)
+		return -ENODEV;
+	/* Create directory */
+	if (snprintf(name, sizeof(name), "rx-%d", efx_rx_queue_index(rx_queue))
+	    >= sizeof(name))
+		return -ENAMETOOLONG;
+	rx_queue->debug_dir = debugfs_create_dir(name,
+						 rx_queue->efx->debug_queues_dir);
+	if (!rx_queue->debug_dir)
+		return -ENOMEM;
+
+	/* Create files */
+	efx_init_debugfs_rx_queue_files(rx_queue);
+
+	/* Create symlink to channel */
+	if (snprintf(target, sizeof(target), "../../channels/%d",
+		     channel->channel) >= sizeof(target))
+		return -ENAMETOOLONG;
+	if (!debugfs_create_symlink("channel", rx_queue->debug_dir, target))
+		return -ENOMEM;
+
+	return 0;
+}
+
+/**
+ * efx_fini_debugfs_rx_queue - remove debugfs directory for RX queue
+ * @rx_queue:		Efx RX queue
+ *
+ * Remove directory created for @rx_queue by efx_init_debugfs_rx_queue().
+ */
+void efx_fini_debugfs_rx_queue(struct efx_rx_queue *rx_queue)
+{
+	debugfs_remove_recursive(rx_queue->debug_dir);
+	rx_queue->debug_dir = NULL;
+}
+
 #define EFX_DEBUGFS_CHANNEL(_type, _name)	\
 	debugfs_create_##_type(#_name, 0444, channel->debug_dir, &channel->_name)
 
@@ -208,9 +274,10 @@ int efx_init_debugfs_nic(struct efx_nic *efx)
 	if (!efx->debug_dir)
 		return -ENOMEM;
 	efx_init_debugfs_nic_files(efx);
-	/* Create channels subdirectory */
+	/* Create subdirectories */
 	efx->debug_channels_dir = debugfs_create_dir("channels",
 						     efx->debug_dir);
+	efx->debug_queues_dir = debugfs_create_dir("queues", efx->debug_dir);
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h
index 4af4a03d1b97..53c98a2fb4c9 100644
--- a/drivers/net/ethernet/sfc/debugfs.h
+++ b/drivers/net/ethernet/sfc/debugfs.h
@@ -28,11 +28,20 @@
  * * "channels/" (&efx_nic.debug_channels_dir).  For each channel, this will
  *   contain a directory (&efx_channel.debug_dir), whose name is the channel
  *   index (in decimal).
+ * * "queues/" (&efx_nic.debug_queues_dir).
+ *
+ *   * For each NIC RX queue, this will contain a directory
+ *     (&efx_rx_queue.debug_dir), whose name is "rx-N" where N is the RX queue
+ *     index.  (This may not be the same as the kernel core RX queue index.)
+ *     The directory will contain a symlink to the owning channel.
  */
 
 void efx_fini_debugfs_netdev(struct net_device *net_dev);
 void efx_update_debugfs_netdev(struct efx_nic *efx);
 
+int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue);
+void efx_fini_debugfs_rx_queue(struct efx_rx_queue *rx_queue);
+
 int efx_init_debugfs_channel(struct efx_channel *channel);
 void efx_fini_debugfs_channel(struct efx_channel *channel);
 
@@ -48,6 +57,12 @@ static inline void efx_fini_debugfs_netdev(struct net_device *net_dev) {}
 
 static inline void efx_update_debugfs_netdev(struct efx_nic *efx) {}
 
+int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue)
+{
+	return 0;
+}
+void efx_fini_debugfs_rx_queue(struct efx_rx_queue *rx_queue) {}
+
 int efx_init_debugfs_channel(struct efx_channel *channel)
 {
 	return 0;
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 2b92c5461fe3..63eb32670826 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -424,6 +424,10 @@ struct efx_rx_queue {
 	struct work_struct grant_work;
 	/* Statistics to supplement MAC stats */
 	unsigned long rx_packets;
+#ifdef CONFIG_DEBUG_FS
+	/** @debug_dir: Queue debugfs directory (under @efx->debug_queues_dir) */
+	struct dentry *debug_dir;
+#endif
 	struct xdp_rxq_info xdp_rxq_info;
 	bool xdp_rxq_info_valid;
 };
@@ -1150,6 +1154,8 @@ struct efx_nic {
 	struct dentry *debug_dir;
 	/** @debug_channels_dir: contains channel debugfs dirs.  Under @debug_dir */
 	struct dentry *debug_channels_dir;
+	/** @debug_queues_dir: contains RX/TX queue debugfs dirs.  Under @debug_dir */
+	struct dentry *debug_queues_dir;
 	/** @debug_symlink: NIC debugfs symlink (``nic_eth%d``) */
 	struct dentry *debug_symlink;
 	/** @debug_interrupt_mode: debugfs details for printing @interrupt_mode */
diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
index d2f35ee15eff..7f63f70f082d 100644
--- a/drivers/net/ethernet/sfc/rx_common.c
+++ b/drivers/net/ethernet/sfc/rx_common.c
@@ -14,6 +14,7 @@
 #include "efx.h"
 #include "nic.h"
 #include "rx_common.h"
+#include "debugfs.h"
 
 /* This is the percentage fill level below which new RX descriptors
  * will be added to the RX descriptor ring.
@@ -208,6 +209,12 @@ int efx_probe_rx_queue(struct efx_rx_queue *rx_queue)
 	if (!rx_queue->buffer)
 		return -ENOMEM;
 
+	rc = efx_init_debugfs_rx_queue(rx_queue);
+	if (rc) /* not fatal */
+		netif_err(efx, drv, efx->net_dev,
+			  "Failed to create debugfs for RXQ %d, rc=%d\n",
+			  efx_rx_queue_index(rx_queue), rc);
+
 	rc = efx_nic_probe_rx(rx_queue);
 	if (rc) {
 		kfree(rx_queue->buffer);
@@ -311,6 +318,8 @@ void efx_remove_rx_queue(struct efx_rx_queue *rx_queue)
 
 	efx_nic_remove_rx(rx_queue);
 
+	efx_fini_debugfs_rx_queue(rx_queue);
+
 	kfree(rx_queue->buffer);
 	rx_queue->buffer = NULL;
 }

[PATCH net-next 4/7] sfc: debugfs for (nic) TX queues

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Expose each TX queue's label, type (csum offloads), TSO and timestamping
 capabilities, and the read/write/etc pointers for the descriptor ring.
Each TXQ dir also symlinks to its owning channel.

Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/debugfs.c    | 71 +++++++++++++++++++++++++++
 drivers/net/ethernet/sfc/debugfs.h    | 14 ++++++
 drivers/net/ethernet/sfc/net_driver.h |  4 ++
 drivers/net/ethernet/sfc/tx_common.c  |  8 +++
 4 files changed, 97 insertions(+)

diff --git a/drivers/net/ethernet/sfc/debugfs.c b/drivers/net/ethernet/sfc/debugfs.c
index 43b1d06a985e..8ee6e401ea44 100644
--- a/drivers/net/ethernet/sfc/debugfs.c
+++ b/drivers/net/ethernet/sfc/debugfs.c
@@ -78,6 +78,77 @@ void efx_update_debugfs_netdev(struct efx_nic *efx)
 	mutex_unlock(&efx->debugfs_symlink_mutex);
 }
 
+#define EFX_DEBUGFS_TXQ(_type, _name)	\
+	debugfs_create_##_type(#_name, 0444, tx_queue->debug_dir, &tx_queue->_name)
+
+/* Create basic debugfs parameter files for an Efx TXQ */
+static void efx_init_debugfs_tx_queue_files(struct efx_tx_queue *tx_queue)
+{
+	EFX_DEBUGFS_TXQ(u32, label);
+	EFX_DEBUGFS_TXQ(bool, xdp_tx);
+	/* offload features */
+	EFX_DEBUGFS_TXQ(u32, type);
+	EFX_DEBUGFS_TXQ(u32, tso_version);
+	EFX_DEBUGFS_TXQ(bool, tso_encap);
+	EFX_DEBUGFS_TXQ(bool, timestamping);
+	/* descriptor ring indices */
+	EFX_DEBUGFS_TXQ(u32, read_count);
+	EFX_DEBUGFS_TXQ(u32, insert_count);
+	EFX_DEBUGFS_TXQ(u32, write_count);
+	EFX_DEBUGFS_TXQ(u32, notify_count);
+}
+
+/**
+ * efx_init_debugfs_tx_queue - create debugfs directory for TX queue
+ * @tx_queue:		Efx TX queue
+ *
+ * Create a debugfs directory containing parameter-files for @tx_queue.
+ * The directory must be cleaned up using efx_fini_debugfs_tx_queue(),
+ * even if this function returns an error.
+ *
+ * Return: a negative error code or 0 on success.
+ */
+int efx_init_debugfs_tx_queue(struct efx_tx_queue *tx_queue)
+{
+	char target[EFX_DEBUGFS_NAME_LEN];
+	char name[EFX_DEBUGFS_NAME_LEN];
+
+	if (!tx_queue->efx->debug_queues_dir)
+		return -ENODEV;
+	/* Create directory */
+	if (snprintf(name, sizeof(name), "tx-%d", tx_queue->queue)
+	    >= sizeof(name))
+		return -ENAMETOOLONG;
+	tx_queue->debug_dir = debugfs_create_dir(name,
+						 tx_queue->efx->debug_queues_dir);
+	if (!tx_queue->debug_dir)
+		return -ENOMEM;
+
+	/* Create files */
+	efx_init_debugfs_tx_queue_files(tx_queue);
+
+	/* Create symlink to channel */
+	if (snprintf(target, sizeof(target), "../../channels/%d",
+		     tx_queue->channel->channel) >= sizeof(target))
+		return -ENAMETOOLONG;
+	if (!debugfs_create_symlink("channel", tx_queue->debug_dir, target))
+		return -ENOMEM;
+
+	return 0;
+}
+
+/**
+ * efx_fini_debugfs_tx_queue - remove debugfs directory for TX queue
+ * @tx_queue:		Efx TX queue
+ *
+ * Remove directory created for @tx_queue by efx_init_debugfs_tx_queue().
+ */
+void efx_fini_debugfs_tx_queue(struct efx_tx_queue *tx_queue)
+{
+	debugfs_remove_recursive(tx_queue->debug_dir);
+	tx_queue->debug_dir = NULL;
+}
+
 #define EFX_DEBUGFS_RXQ(_type, _name)	\
 	debugfs_create_##_type(#_name, 0444, rx_queue->debug_dir, &rx_queue->_name)
 
diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h
index 53c98a2fb4c9..3e8d2e2b5bad 100644
--- a/drivers/net/ethernet/sfc/debugfs.h
+++ b/drivers/net/ethernet/sfc/debugfs.h
@@ -34,11 +34,19 @@
  *     (&efx_rx_queue.debug_dir), whose name is "rx-N" where N is the RX queue
  *     index.  (This may not be the same as the kernel core RX queue index.)
  *     The directory will contain a symlink to the owning channel.
+ *   * For each NIC TX queue, this will contain a directory
+ *     (&efx_tx_queue.debug_dir), whose name is "tx-N" where N is the TX queue
+ *     index.  (This may differ from both the kernel core TX queue index and
+ *     the hardware queue label of the TXQ.)
+ *     The directory will contain a symlink to the owning channel.
  */
 
 void efx_fini_debugfs_netdev(struct net_device *net_dev);
 void efx_update_debugfs_netdev(struct efx_nic *efx);
 
+int efx_init_debugfs_tx_queue(struct efx_tx_queue *tx_queue);
+void efx_fini_debugfs_tx_queue(struct efx_tx_queue *tx_queue);
+
 int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue);
 void efx_fini_debugfs_rx_queue(struct efx_rx_queue *rx_queue);
 
@@ -57,6 +65,12 @@ static inline void efx_fini_debugfs_netdev(struct net_device *net_dev) {}
 
 static inline void efx_update_debugfs_netdev(struct efx_nic *efx) {}
 
+int efx_init_debugfs_tx_queue(struct efx_tx_queue *tx_queue)
+{
+	return 0;
+}
+void efx_fini_debugfs_tx_queue(struct efx_tx_queue *tx_queue) {}
+
 int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue)
 {
 	return 0;
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 63eb32670826..feb87979059c 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -273,6 +273,10 @@ struct efx_tx_queue {
 	bool initialised;
 	bool timestamping;
 	bool xdp_tx;
+#ifdef CONFIG_DEBUG_FS
+	/** @debug_dir: Queue debugfs directory (under @efx->debug_queues_dir) */
+	struct dentry *debug_dir;
+#endif
 
 	/* Members used mainly on the completion path */
 	unsigned int read_count ____cacheline_aligned_in_smp;
diff --git a/drivers/net/ethernet/sfc/tx_common.c b/drivers/net/ethernet/sfc/tx_common.c
index 9f2393d34371..3780da849e98 100644
--- a/drivers/net/ethernet/sfc/tx_common.c
+++ b/drivers/net/ethernet/sfc/tx_common.c
@@ -12,6 +12,7 @@
 #include "efx.h"
 #include "nic_common.h"
 #include "tx_common.h"
+#include "debugfs.h"
 #include <net/gso.h>
 
 static unsigned int efx_tx_cb_page_count(struct efx_tx_queue *tx_queue)
@@ -47,6 +48,11 @@ int efx_probe_tx_queue(struct efx_tx_queue *tx_queue)
 		rc = -ENOMEM;
 		goto fail1;
 	}
+	rc = efx_init_debugfs_tx_queue(tx_queue);
+	if (rc) /* not fatal */
+		netif_err(efx, drv, efx->net_dev,
+			  "Failed to create debugfs for TXQ %d, rc=%d\n",
+			  tx_queue->queue, rc);
 
 	/* Allocate hardware ring, determine TXQ type */
 	rc = efx_nic_probe_tx(tx_queue);
@@ -133,6 +139,8 @@ void efx_remove_tx_queue(struct efx_tx_queue *tx_queue)
 		  "destroying TX queue %d\n", tx_queue->queue);
 	efx_nic_remove_tx(tx_queue);
 
+	efx_fini_debugfs_tx_queue(tx_queue);
+
 	if (tx_queue->cb_page) {
 		for (i = 0; i < efx_tx_cb_page_count(tx_queue); i++)
 			efx_nic_free_buffer(tx_queue->efx,

[PATCH net-next 5/7] sfc: add debugfs nodes for loopback mode

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

'loopback_mode' shows the currently selected mode, while 'loopback_modes'
 is a bitmask of modes supported by the NIC+FW.

Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/debugfs.c    | 7 +++++++
 drivers/net/ethernet/sfc/net_driver.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/sfc/debugfs.c b/drivers/net/ethernet/sfc/debugfs.c
index 8ee6e401ea44..549ff1ee273e 100644
--- a/drivers/net/ethernet/sfc/debugfs.c
+++ b/drivers/net/ethernet/sfc/debugfs.c
@@ -326,6 +326,13 @@ static void efx_init_debugfs_nic_files(struct efx_nic *efx)
 	efx->debug_interrupt_mode.value = &efx->interrupt_mode;
 	efx_debugfs_create_enum("interrupt_mode", 0444, efx->debug_dir,
 				&efx->debug_interrupt_mode);
+	EFX_DEBUGFS_EFX(x64, loopback_modes);
+	efx->debug_loopback_mode.max = efx_loopback_mode_max;
+	efx->debug_loopback_mode.names = efx_loopback_mode_names;
+	efx->debug_loopback_mode.vlen = sizeof(efx->loopback_mode);
+	efx->debug_loopback_mode.value = &efx->loopback_mode;
+	efx_debugfs_create_enum("loopback_mode", 0444, efx->debug_dir,
+				&efx->debug_loopback_mode);
 }
 
 /**
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index feb87979059c..e3605c361117 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -1164,6 +1164,8 @@ struct efx_nic {
 	struct dentry *debug_symlink;
 	/** @debug_interrupt_mode: debugfs details for printing @interrupt_mode */
 	struct efx_debugfs_enum_data debug_interrupt_mode;
+	/** @debug_loopback_mode: debugfs details for printing @loopback_mode */
+	struct efx_debugfs_enum_data debug_loopback_mode;
 	/** @debugfs_symlink_mutex: protects debugfs @debug_symlink */
 	struct mutex debugfs_symlink_mutex;
 #endif

[PATCH net-next 6/7] sfc: add debugfs entries for filter table status

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Filter table management is complicated by the possibility of overflow
 kicking us into a promiscuous fallback for either unicast or multicast.
 Expose the internal flags that drive this.
Since the table state (efx->filter_state) has a separate, shorter
 lifetime than struct efx_nic, put its debugfs nodes in a subdirectory
 (efx->filter_state->debug_dir) so that they can be cleaned up easily
 before the filter_state is freed.

Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/debugfs.h      |  4 ++++
 drivers/net/ethernet/sfc/mcdi_filters.c | 18 ++++++++++++++++++
 drivers/net/ethernet/sfc/mcdi_filters.h |  4 ++++
 3 files changed, 26 insertions(+)

diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h
index 3e8d2e2b5bad..7a96f3798cbd 100644
--- a/drivers/net/ethernet/sfc/debugfs.h
+++ b/drivers/net/ethernet/sfc/debugfs.h
@@ -39,6 +39,10 @@
  *     index.  (This may differ from both the kernel core TX queue index and
  *     the hardware queue label of the TXQ.)
  *     The directory will contain a symlink to the owning channel.
+ *
+ * * "filters/" (&efx_mcdi_filter_table.debug_dir).
+ *   This contains parameter files for the NIC receive filter table
+ *   (@efx->filter_state).
  */
 
 void efx_fini_debugfs_netdev(struct net_device *net_dev);
diff --git a/drivers/net/ethernet/sfc/mcdi_filters.c b/drivers/net/ethernet/sfc/mcdi_filters.c
index 4ff6586116ee..a4ab45082c8f 100644
--- a/drivers/net/ethernet/sfc/mcdi_filters.c
+++ b/drivers/net/ethernet/sfc/mcdi_filters.c
@@ -1348,6 +1348,20 @@ int efx_mcdi_filter_table_probe(struct efx_nic *efx, bool multicast_chaining)
 	INIT_LIST_HEAD(&table->vlan_list);
 	init_rwsem(&table->lock);
 
+#ifdef CONFIG_DEBUG_FS
+	table->debug_dir = debugfs_create_dir("filters", efx->debug_dir);
+	debugfs_create_bool("uc_promisc", 0444, table->debug_dir,
+			    &table->uc_promisc);
+	debugfs_create_bool("mc_promisc", 0444, table->debug_dir,
+			    &table->mc_promisc);
+	debugfs_create_bool("mc_promisc_last", 0444, table->debug_dir,
+			    &table->mc_promisc_last);
+	debugfs_create_bool("mc_overflow", 0444, table->debug_dir,
+			    &table->mc_overflow);
+	debugfs_create_bool("mc_chaining", 0444, table->debug_dir,
+			    &table->mc_chaining);
+#endif
+
 	efx->filter_state = table;
 
 	return 0;
@@ -1518,6 +1532,10 @@ void efx_mcdi_filter_table_remove(struct efx_nic *efx)
 		return;
 
 	vfree(table->entry);
+#ifdef CONFIG_DEBUG_FS
+	/* Remove debugfs entries pointing into @table */
+	debugfs_remove_recursive(table->debug_dir);
+#endif
 	kfree(table);
 }
 
diff --git a/drivers/net/ethernet/sfc/mcdi_filters.h b/drivers/net/ethernet/sfc/mcdi_filters.h
index c0d6558b9fd2..897843ade3ec 100644
--- a/drivers/net/ethernet/sfc/mcdi_filters.h
+++ b/drivers/net/ethernet/sfc/mcdi_filters.h
@@ -91,6 +91,10 @@ struct efx_mcdi_filter_table {
 	bool vlan_filter;
 	/* Entries on the vlan_list are added/removed under filter_sem */
 	struct list_head vlan_list;
+#ifdef CONFIG_DEBUG_FS
+	/* filter table debugfs directory */
+	struct dentry *debug_dir;
+#endif
 };
 
 int efx_mcdi_filter_table_probe(struct efx_nic *efx, bool multicast_chaining);

[PATCH net-next 7/7] sfc: add debugfs node for filter table contents

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Expose the filter table entries.

Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/debugfs.c      | 117 +++++++++++++++++++++++-
 drivers/net/ethernet/sfc/debugfs.h      |  45 +++++++++
 drivers/net/ethernet/sfc/mcdi_filters.c |  39 ++++++++
 3 files changed, 197 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/sfc/debugfs.c b/drivers/net/ethernet/sfc/debugfs.c
index 549ff1ee273e..e67b0fc927fe 100644
--- a/drivers/net/ethernet/sfc/debugfs.c
+++ b/drivers/net/ethernet/sfc/debugfs.c
@@ -9,10 +9,6 @@
  */
 
 #include "debugfs.h"
-#include <linux/module.h>
-#include <linux/debugfs.h>
-#include <linux/dcache.h>
-#include <linux/seq_file.h>
 
 /* Maximum length for a name component or symlink target */
 #define EFX_DEBUGFS_NAME_LEN 32
@@ -428,3 +424,116 @@ void efx_fini_debugfs(void)
 	efx_debug_cards = NULL;
 	efx_debug_root = NULL;
 }
+
+/**
+ * efx_debugfs_print_filter - format a filter spec for display
+ * @s: buffer to write result into
+ * @l: length of buffer @s
+ * @spec: filter specification
+ */
+void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec)
+{
+	u32 ip[4];
+	int p = snprintf(s, l, "match=%#x,pri=%d,flags=%#x,q=%d",
+			 spec->match_flags, spec->priority, spec->flags,
+			 spec->dmaq_id);
+
+	if (spec->vport_id)
+		p += snprintf(s + p, l - p, ",vport=%#x", spec->vport_id);
+
+	if (spec->flags & EFX_FILTER_FLAG_RX_RSS)
+		p += snprintf(s + p, l - p, ",rss=%#x", spec->rss_context);
+
+	if (spec->match_flags & EFX_FILTER_MATCH_OUTER_VID)
+		p += snprintf(s + p, l - p,
+			      ",ovid=%d", ntohs(spec->outer_vid));
+	if (spec->match_flags & EFX_FILTER_MATCH_INNER_VID)
+		p += snprintf(s + p, l - p,
+			      ",ivid=%d", ntohs(spec->inner_vid));
+	if (spec->match_flags & EFX_FILTER_MATCH_ENCAP_TYPE)
+		p += snprintf(s + p, l - p,
+			      ",encap=%d", spec->encap_type);
+	if (spec->match_flags & EFX_FILTER_MATCH_LOC_MAC)
+		p += snprintf(s + p, l - p,
+			      ",lmac=%02x:%02x:%02x:%02x:%02x:%02x",
+			      spec->loc_mac[0], spec->loc_mac[1],
+			      spec->loc_mac[2], spec->loc_mac[3],
+			      spec->loc_mac[4], spec->loc_mac[5]);
+	if (spec->match_flags & EFX_FILTER_MATCH_REM_MAC)
+		p += snprintf(s + p, l - p,
+			      ",rmac=%02x:%02x:%02x:%02x:%02x:%02x",
+			      spec->rem_mac[0], spec->rem_mac[1],
+			      spec->rem_mac[2], spec->rem_mac[3],
+			      spec->rem_mac[4], spec->rem_mac[5]);
+	if (spec->match_flags & EFX_FILTER_MATCH_ETHER_TYPE)
+		p += snprintf(s + p, l - p,
+			      ",ether=%#x", ntohs(spec->ether_type));
+	if (spec->match_flags & EFX_FILTER_MATCH_IP_PROTO)
+		p += snprintf(s + p, l - p,
+			      ",ippr=%#x", spec->ip_proto);
+	if (spec->match_flags & EFX_FILTER_MATCH_LOC_HOST) {
+		if (ntohs(spec->ether_type) == ETH_P_IP) {
+			ip[0] = (__force u32) spec->loc_host[0];
+			p += snprintf(s + p, l - p,
+				      ",lip=%d.%d.%d.%d",
+				      ip[0] & 0xff,
+				      (ip[0] >> 8) & 0xff,
+				      (ip[0] >> 16) & 0xff,
+				      (ip[0] >> 24) & 0xff);
+		} else if (ntohs(spec->ether_type) == ETH_P_IPV6) {
+			ip[0] = (__force u32) spec->loc_host[0];
+			ip[1] = (__force u32) spec->loc_host[1];
+			ip[2] = (__force u32) spec->loc_host[2];
+			ip[3] = (__force u32) spec->loc_host[3];
+			p += snprintf(s + p, l - p,
+				      ",lip=%04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x",
+				      ip[0] & 0xffff,
+				      (ip[0] >> 16) & 0xffff,
+				      ip[1] & 0xffff,
+				      (ip[1] >> 16) & 0xffff,
+				      ip[2] & 0xffff,
+				      (ip[2] >> 16) & 0xffff,
+				      ip[3] & 0xffff,
+				      (ip[3] >> 16) & 0xffff);
+		} else {
+			p += snprintf(s + p, l - p, ",lip=?");
+		}
+	}
+	if (spec->match_flags & EFX_FILTER_MATCH_REM_HOST) {
+		if (ntohs(spec->ether_type) == ETH_P_IP) {
+			ip[0] = (__force u32) spec->rem_host[0];
+			p += snprintf(s + p, l - p,
+				      ",rip=%d.%d.%d.%d",
+				      ip[0] & 0xff,
+				      (ip[0] >> 8) & 0xff,
+				      (ip[0] >> 16) & 0xff,
+				      (ip[0] >> 24) & 0xff);
+		} else if (ntohs(spec->ether_type) == ETH_P_IPV6) {
+			ip[0] = (__force u32) spec->rem_host[0];
+			ip[1] = (__force u32) spec->rem_host[1];
+			ip[2] = (__force u32) spec->rem_host[2];
+			ip[3] = (__force u32) spec->rem_host[3];
+			p += snprintf(s + p, l - p,
+				      ",rip=%04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x",
+				      ip[0] & 0xffff,
+				      (ip[0] >> 16) & 0xffff,
+				      ip[1] & 0xffff,
+				      (ip[1] >> 16) & 0xffff,
+				      ip[2] & 0xffff,
+				      (ip[2] >> 16) & 0xffff,
+				      ip[3] & 0xffff,
+				      (ip[3] >> 16) & 0xffff);
+		} else {
+			p += snprintf(s + p, l - p, ",rip=?");
+		}
+	}
+	if (spec->match_flags & EFX_FILTER_MATCH_LOC_PORT)
+		p += snprintf(s + p, l - p,
+			      ",lport=%d", ntohs(spec->loc_port));
+	if (spec->match_flags & EFX_FILTER_MATCH_REM_PORT)
+		p += snprintf(s + p, l - p,
+			      ",rport=%d", ntohs(spec->rem_port));
+	if (spec->match_flags & EFX_FILTER_MATCH_LOC_MAC_IG)
+		p += snprintf(s + p, l - p, ",%s",
+			      spec->loc_mac[0] ? "mc" : "uc");
+}
diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h
index 7a96f3798cbd..f50b4bf33a6b 100644
--- a/drivers/net/ethernet/sfc/debugfs.h
+++ b/drivers/net/ethernet/sfc/debugfs.h
@@ -10,6 +10,10 @@
 
 #ifndef EFX_DEBUGFS_H
 #define EFX_DEBUGFS_H
+#include <linux/module.h>
+#include <linux/debugfs.h>
+#include <linux/dcache.h>
+#include <linux/seq_file.h>
 #include "net_driver.h"
 
 #ifdef CONFIG_DEBUG_FS
@@ -63,6 +67,45 @@ void efx_fini_debugfs_nic(struct efx_nic *efx);
 int efx_init_debugfs(void);
 void efx_fini_debugfs(void);
 
+void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec);
+
+/* Generate operations for a debugfs node with a custom reader function.
+ * The reader should have signature int (*)(struct seq_file *s, void *data)
+ * where data is the pointer passed to EFX_DEBUGFS_CREATE_RAW.
+ */
+#define EFX_DEBUGFS_RAW_PARAMETER(_reader)				       \
+									       \
+static int efx_debugfs_##_reader##_read(struct seq_file *s, void *d)	       \
+{									       \
+	return _reader(s, s->private);					       \
+}									       \
+									       \
+static int efx_debugfs_##_reader##_open(struct inode *inode, struct file *f)   \
+{									       \
+	return single_open(f, efx_debugfs_##_reader##_read, inode->i_private); \
+}									       \
+									       \
+static const struct file_operations efx_debugfs_##_reader##_ops = {	       \
+	.owner = THIS_MODULE,						       \
+	.open = efx_debugfs_##_reader##_open,				       \
+	.release = single_release,					       \
+	.read = seq_read,						       \
+	.llseek = seq_lseek,						       \
+};									       \
+									       \
+static void efx_debugfs_create_##_reader(const char *name, umode_t mode,       \
+					 struct dentry *parent, void *data)    \
+{									       \
+	debugfs_create_file(name, mode, parent, data,			       \
+			    &efx_debugfs_##_reader##_ops);		       \
+}
+
+/* Instantiate a debugfs node with a custom reader function.  The operations
+ * must have been generated with EFX_DEBUGFS_RAW_PARAMETER(_reader).
+ */
+#define EFX_DEBUGFS_CREATE_RAW(_name, _mode, _parent, _data, _reader)	       \
+		efx_debugfs_create_##_reader(_name, _mode, _parent, _data)
+
 #else /* CONFIG_DEBUG_FS */
 
 static inline void efx_fini_debugfs_netdev(struct net_device *net_dev) {}
@@ -99,6 +142,8 @@ static inline int efx_init_debugfs(void)
 }
 static inline void efx_fini_debugfs(void) {}
 
+void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec) {}
+
 #endif /* CONFIG_DEBUG_FS */
 
 #endif /* EFX_DEBUGFS_H */
diff --git a/drivers/net/ethernet/sfc/mcdi_filters.c b/drivers/net/ethernet/sfc/mcdi_filters.c
index a4ab45082c8f..465226c3e8c7 100644
--- a/drivers/net/ethernet/sfc/mcdi_filters.c
+++ b/drivers/net/ethernet/sfc/mcdi_filters.c
@@ -13,6 +13,7 @@
 #include "mcdi.h"
 #include "nic.h"
 #include "rx_common.h"
+#include "debugfs.h"
 
 /* The maximum size of a shared RSS context */
 /* TODO: this should really be from the mcdi protocol export */
@@ -1173,6 +1174,42 @@ s32 efx_mcdi_filter_get_rx_ids(struct efx_nic *efx,
 	return count;
 }
 
+static int efx_debugfs_read_filter_list(struct seq_file *file, void *data)
+{
+	struct efx_mcdi_filter_table *table;
+	struct efx_nic *efx = data;
+	int i;
+
+	down_read(&efx->filter_sem);
+	table = efx->filter_state;
+	if (!table || !table->entry) {
+		up_read(&efx->filter_sem);
+		return -ENETDOWN;
+	}
+
+	/* deliberately don't lock the table->lock, so that we can
+	 * still dump the table even if we hang mid-operation.
+	 */
+	for (i = 0; i < EFX_MCDI_FILTER_TBL_ROWS; ++i) {
+		struct efx_filter_spec *spec =
+			efx_mcdi_filter_entry_spec(table, i);
+		char filter[256];
+
+		if (spec) {
+			efx_debugfs_print_filter(filter, sizeof(filter), spec);
+
+			seq_printf(file, "%d[%#04llx],%#x = %s\n",
+				   i, table->entry[i].handle & 0xffff,
+				   efx_mcdi_filter_entry_flags(table, i),
+				   filter);
+		}
+	}
+
+	up_read(&efx->filter_sem);
+	return 0;
+}
+EFX_DEBUGFS_RAW_PARAMETER(efx_debugfs_read_filter_list);
+
 static int efx_mcdi_filter_match_flags_from_mcdi(bool encap, u32 mcdi_flags)
 {
 	int match_flags = 0;
@@ -1360,6 +1397,8 @@ int efx_mcdi_filter_table_probe(struct efx_nic *efx, bool multicast_chaining)
 			    &table->mc_overflow);
 	debugfs_create_bool("mc_chaining", 0444, table->debug_dir,
 			    &table->mc_chaining);
+	EFX_DEBUGFS_CREATE_RAW("entries", 0444, table->debug_dir, efx,
+			       efx_debugfs_read_filter_list);
 #endif
 
 	efx->filter_state = table;

Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents

[Simon Horman]

On Mon, Dec 11, 2023 at 05:18:32PM +0000, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Expose the filter table entries.
> 
> Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

...

> diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h

...

> @@ -63,6 +67,45 @@ void efx_fini_debugfs_nic(struct efx_nic *efx);
>  int efx_init_debugfs(void);
>  void efx_fini_debugfs(void);
>  
> +void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec);
> +
> +/* Generate operations for a debugfs node with a custom reader function.
> + * The reader should have signature int (*)(struct seq_file *s, void *data)
> + * where data is the pointer passed to EFX_DEBUGFS_CREATE_RAW.
> + */
> +#define EFX_DEBUGFS_RAW_PARAMETER(_reader)				       \
> +									       \
> +static int efx_debugfs_##_reader##_read(struct seq_file *s, void *d)	       \
> +{									       \
> +	return _reader(s, s->private);					       \
> +}									       \
> +									       \
> +static int efx_debugfs_##_reader##_open(struct inode *inode, struct file *f)   \
> +{									       \
> +	return single_open(f, efx_debugfs_##_reader##_read, inode->i_private); \
> +}									       \

Hi Edward,

I think that probably the above should be static inline.

...

Re: [PATCH net-next 6/7] sfc: add debugfs entries for filter table status

[Simon Horman]

On Mon, Dec 11, 2023 at 05:18:31PM +0000, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Filter table management is complicated by the possibility of overflow
>  kicking us into a promiscuous fallback for either unicast or multicast.
>  Expose the internal flags that drive this.
> Since the table state (efx->filter_state) has a separate, shorter
>  lifetime than struct efx_nic, put its debugfs nodes in a subdirectory
>  (efx->filter_state->debug_dir) so that they can be cleaned up easily
>  before the filter_state is freed.
> 
> Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

...

> index 4ff6586116ee..a4ab45082c8f 100644
> --- a/drivers/net/ethernet/sfc/mcdi_filters.c
> +++ b/drivers/net/ethernet/sfc/mcdi_filters.c
> @@ -1348,6 +1348,20 @@ int efx_mcdi_filter_table_probe(struct efx_nic *efx, bool multicast_chaining)
>  	INIT_LIST_HEAD(&table->vlan_list);
>  	init_rwsem(&table->lock);
>  
> +#ifdef CONFIG_DEBUG_FS
> +	table->debug_dir = debugfs_create_dir("filters", efx->debug_dir);
> +	debugfs_create_bool("uc_promisc", 0444, table->debug_dir,
> +			    &table->uc_promisc);
> +	debugfs_create_bool("mc_promisc", 0444, table->debug_dir,
> +			    &table->mc_promisc);
> +	debugfs_create_bool("mc_promisc_last", 0444, table->debug_dir,
> +			    &table->mc_promisc_last);
> +	debugfs_create_bool("mc_overflow", 0444, table->debug_dir,
> +			    &table->mc_overflow);
> +	debugfs_create_bool("mc_chaining", 0444, table->debug_dir,
> +			    &table->mc_chaining);
> +#endif
> +
>  	efx->filter_state = table;
>  
>  	return 0;
> @@ -1518,6 +1532,10 @@ void efx_mcdi_filter_table_remove(struct efx_nic *efx)
>  		return;
>  
>  	vfree(table->entry);
> +#ifdef CONFIG_DEBUG_FS
> +	/* Remove debugfs entries pointing into @table */
> +	debugfs_remove_recursive(table->debug_dir);
> +#endif
>  	kfree(table);
>  }
>  

Hi Edward,

I think debugfs.h needs to be included so that debugfs_*() are defined.

...

-- 
pw-bot: changes-requested

Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents

[Edward Cree]

On 11/12/2023 19:17, Simon Horman wrote:
> On Mon, Dec 11, 2023 at 05:18:32PM +0000, edward.cree@amd.com wrote:
>> @@ -63,6 +67,45 @@ void efx_fini_debugfs_nic(struct efx_nic *efx);
>>  int efx_init_debugfs(void);
>>  void efx_fini_debugfs(void);
>>  
>> +void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec);
>> +
>> +/* Generate operations for a debugfs node with a custom reader function.
>> + * The reader should have signature int (*)(struct seq_file *s, void *data)
>> + * where data is the pointer passed to EFX_DEBUGFS_CREATE_RAW.
>> + */
>> +#define EFX_DEBUGFS_RAW_PARAMETER(_reader)				       \
>> +									       \
>> +static int efx_debugfs_##_reader##_read(struct seq_file *s, void *d)	       \
>> +{									       \
>> +	return _reader(s, s->private);					       \
>> +}									       \
>> +									       \
>> +static int efx_debugfs_##_reader##_open(struct inode *inode, struct file *f)   \
>> +{									       \
>> +	return single_open(f, efx_debugfs_##_reader##_read, inode->i_private); \
>> +}									       \
> 
> Hi Edward,
> 
> I think that probably the above should be static inline.

Yep, in fact there are instances of this from patch 2 onwards (most
 of those aren't even static).  Clearly I hadn't had enough sleep
 the day I wrote this :/
Will fix in v2, along with the build break on #6.
Thanks for reviewing.

-ed

Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents

[Edward Cree]

On 12/12/2023 13:58, Edward Cree wrote:
> On 11/12/2023 19:17, Simon Horman wrote:
>> On Mon, Dec 11, 2023 at 05:18:32PM +0000, edward.cree@amd.com wrote:
>>> @@ -63,6 +67,45 @@ void efx_fini_debugfs_nic(struct efx_nic *efx);
>>>  int efx_init_debugfs(void);
>>>  void efx_fini_debugfs(void);
>>>  
>>> +void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec);
>>> +
>>> +/* Generate operations for a debugfs node with a custom reader function.
>>> + * The reader should have signature int (*)(struct seq_file *s, void *data)
>>> + * where data is the pointer passed to EFX_DEBUGFS_CREATE_RAW.
>>> + */
>>> +#define EFX_DEBUGFS_RAW_PARAMETER(_reader)				       \
>>> +									       \
>>> +static int efx_debugfs_##_reader##_read(struct seq_file *s, void *d)	       \
>>> +{									       \
>>> +	return _reader(s, s->private);					       \
>>> +}									       \
>>> +									       \
>>> +static int efx_debugfs_##_reader##_open(struct inode *inode, struct file *f)   \
>>> +{									       \
>>> +	return single_open(f, efx_debugfs_##_reader##_read, inode->i_private); \
>>> +}									       \
>>
>> Hi Edward,
>>
>> I think that probably the above should be static inline.
> 
> Yep, in fact there are instances of this from patch 2 onwards (most
>  of those aren't even static).  Clearly I hadn't had enough sleep
>  the day I wrote this :/
Or maybe it's *today* I haven't had enough sleep...
Unlike the functions in patches 2-4, which are stubs for the
 CONFIG_DEBUG_FS=n build, these functions should *not* be "static
 inline", because they are intended to be referenced from ops
 structs or passed as callbacks.
The check on patchwork is actually a false positive here, because
 this is not a function that's defined in the header file.  It's
 part of the body of a *macro*, EFX_DEBUGFS_RAW_PARAMETER.
Functions are only defined when some C file expands the macro.

I will update the commit message to call out and explain this; I
 believe the code is actually fine.

-ed

Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents

[Jakub Kicinski]

On Tue, 12 Dec 2023 15:14:17 +0000 Edward Cree wrote:
> On 12/12/2023 13:58, Edward Cree wrote:
> > On 11/12/2023 19:17, Simon Horman wrote:  
> >> On Mon, Dec 11, 2023 at 05:18:32PM +0000, edward.cree@amd.com wrote:  
>  [...]  
> >>
> >> Hi Edward,
> >>
> >> I think that probably the above should be static inline.  
> > 
> > Yep, in fact there are instances of this from patch 2 onwards (most
> >  of those aren't even static).  Clearly I hadn't had enough sleep
> >  the day I wrote this :/  
> Or maybe it's *today* I haven't had enough sleep...
> Unlike the functions in patches 2-4, which are stubs for the
>  CONFIG_DEBUG_FS=n build, these functions should *not* be "static
>  inline", because they are intended to be referenced from ops
>  structs or passed as callbacks.
> The check on patchwork is actually a false positive here, because
>  this is not a function that's defined in the header file.  It's
>  part of the body of a *macro*, EFX_DEBUGFS_RAW_PARAMETER.
> Functions are only defined when some C file expands the macro.
> 
> I will update the commit message to call out and explain this; I
>  believe the code is actually fine.

Fair point, second time in a ~month we see this sort of false positive.
I'll throw [^\\]$ at the end of the regex to try to avoid matching stuff
that's most likely a macro.

This one looks legit tho:

+void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec) {}

Re: [PATCH net-next 2/7] sfc: debugfs for channels

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/edward-cree-amd-com/sfc-initial-debugfs-implementation/20231212-013223
base:   net-next/main
patch link:    https://lore.kernel.org/r/df43d737fda6b6aa0cda3f2cb300916ca4b2e8f8.1702314695.git.ecree.xilinx%40gmail.com
patch subject: [PATCH net-next 2/7] sfc: debugfs for channels
config: mips-ip27_defconfig (https://download.01.org/0day-ci/archive/20231213/202312130301.cKD8l8IO-lkp@intel.com/config)
compiler: mips64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231213/202312130301.cKD8l8IO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312130301.cKD8l8IO-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/net/ethernet/sfc/efx.c:36:
>> drivers/net/ethernet/sfc/debugfs.h:51:5: warning: no previous prototype for 'efx_init_debugfs_channel' [-Wmissing-prototypes]
      51 | int efx_init_debugfs_channel(struct efx_channel *channel)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/ethernet/sfc/debugfs.h:55:6: warning: no previous prototype for 'efx_fini_debugfs_channel' [-Wmissing-prototypes]
      55 | void efx_fini_debugfs_channel(struct efx_channel *channel) {}
         |      ^~~~~~~~~~~~~~~~~~~~~~~~


vim +/efx_init_debugfs_channel +51 drivers/net/ethernet/sfc/debugfs.h

    50	
  > 51	int efx_init_debugfs_channel(struct efx_channel *channel)
    52	{
    53		return 0;
    54	}
  > 55	void efx_fini_debugfs_channel(struct efx_channel *channel) {}
    56	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/edward-cree-amd-com/sfc-initial-debugfs-implementation/20231212-013223
base:   net-next/main
patch link:    https://lore.kernel.org/r/a5c5491d3d0b58b8f8dff65cb53f892d7b13c32a.1702314695.git.ecree.xilinx%40gmail.com
patch subject: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues
config: mips-ip27_defconfig (https://download.01.org/0day-ci/archive/20231213/202312130527.xlkFaOuC-lkp@intel.com/config)
compiler: mips64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231213/202312130527.xlkFaOuC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312130527.xlkFaOuC-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/net/ethernet/sfc/efx.c:36:
>> drivers/net/ethernet/sfc/debugfs.h:60:5: warning: no previous prototype for 'efx_init_debugfs_rx_queue' [-Wmissing-prototypes]
      60 | int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/ethernet/sfc/debugfs.h:64:6: warning: no previous prototype for 'efx_fini_debugfs_rx_queue' [-Wmissing-prototypes]
      64 | void efx_fini_debugfs_rx_queue(struct efx_rx_queue *rx_queue) {}
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/sfc/debugfs.h:66:5: warning: no previous prototype for 'efx_init_debugfs_channel' [-Wmissing-prototypes]
      66 | int efx_init_debugfs_channel(struct efx_channel *channel)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/sfc/debugfs.h:70:6: warning: no previous prototype for 'efx_fini_debugfs_channel' [-Wmissing-prototypes]
      70 | void efx_fini_debugfs_channel(struct efx_channel *channel) {}
         |      ^~~~~~~~~~~~~~~~~~~~~~~~


vim +/efx_init_debugfs_rx_queue +60 drivers/net/ethernet/sfc/debugfs.h

    59	
  > 60	int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue)
    61	{
    62		return 0;
    63	}
  > 64	void efx_fini_debugfs_rx_queue(struct efx_rx_queue *rx_queue) {}
    65	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 4/7] sfc: debugfs for (nic) TX queues

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/edward-cree-amd-com/sfc-initial-debugfs-implementation/20231212-013223
base:   net-next/main
patch link:    https://lore.kernel.org/r/91beef38162b8e243c2275b41a6f37c01f19850f.1702314695.git.ecree.xilinx%40gmail.com
patch subject: [PATCH net-next 4/7] sfc: debugfs for (nic) TX queues
config: mips-ip27_defconfig (https://download.01.org/0day-ci/archive/20231213/202312130801.r8TbjUfD-lkp@intel.com/config)
compiler: mips64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231213/202312130801.r8TbjUfD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312130801.r8TbjUfD-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/net/ethernet/sfc/efx.c:36:
>> drivers/net/ethernet/sfc/debugfs.h:68:5: warning: no previous prototype for 'efx_init_debugfs_tx_queue' [-Wmissing-prototypes]
      68 | int efx_init_debugfs_tx_queue(struct efx_tx_queue *tx_queue)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/ethernet/sfc/debugfs.h:72:6: warning: no previous prototype for 'efx_fini_debugfs_tx_queue' [-Wmissing-prototypes]
      72 | void efx_fini_debugfs_tx_queue(struct efx_tx_queue *tx_queue) {}
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/sfc/debugfs.h:74:5: warning: no previous prototype for 'efx_init_debugfs_rx_queue' [-Wmissing-prototypes]
      74 | int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/sfc/debugfs.h:78:6: warning: no previous prototype for 'efx_fini_debugfs_rx_queue' [-Wmissing-prototypes]
      78 | void efx_fini_debugfs_rx_queue(struct efx_rx_queue *rx_queue) {}
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/sfc/debugfs.h:80:5: warning: no previous prototype for 'efx_init_debugfs_channel' [-Wmissing-prototypes]
      80 | int efx_init_debugfs_channel(struct efx_channel *channel)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/sfc/debugfs.h:84:6: warning: no previous prototype for 'efx_fini_debugfs_channel' [-Wmissing-prototypes]
      84 | void efx_fini_debugfs_channel(struct efx_channel *channel) {}
         |      ^~~~~~~~~~~~~~~~~~~~~~~~


vim +/efx_init_debugfs_tx_queue +68 drivers/net/ethernet/sfc/debugfs.h

    67	
  > 68	int efx_init_debugfs_tx_queue(struct efx_tx_queue *tx_queue)
    69	{
    70		return 0;
    71	}
  > 72	void efx_fini_debugfs_tx_queue(struct efx_tx_queue *tx_queue) {}
    73	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents

[Edward Cree]

On 12/12/2023 16:19, Jakub Kicinski wrote:
> On Tue, 12 Dec 2023 15:14:17 +0000 Edward Cree wrote:
>> I will update the commit message to call out and explain this; I
>>  believe the code is actually fine.
> 
> Fair point, second time in a ~month we see this sort of false positive.
> I'll throw [^\\]$ at the end of the regex to try to avoid matching stuff
> that's most likely a macro.

Sounds good, thanks.

> This one looks legit tho:
> 
> +void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec) {}

Yep, that one's real, will be fixed in v2.
And this time I'll actually build-test with CONFIG_DEBUG_FS=n,
 which I forgot to do with v1 (sorry).

-ed

Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents

[Simon Horman]

On Wed, Dec 13, 2023 at 12:15:04PM +0000, Edward Cree wrote:
> On 12/12/2023 16:19, Jakub Kicinski wrote:
> > On Tue, 12 Dec 2023 15:14:17 +0000 Edward Cree wrote:
> >> I will update the commit message to call out and explain this; I
> >>  believe the code is actually fine.
> > 
> > Fair point, second time in a ~month we see this sort of false positive.
> > I'll throw [^\\]$ at the end of the regex to try to avoid matching stuff
> > that's most likely a macro.
> 
> Sounds good, thanks.
> 
> > This one looks legit tho:
> > 
> > +void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec) {}
> 
> Yep, that one's real, will be fixed in v2.
> And this time I'll actually build-test with CONFIG_DEBUG_FS=n,
>  which I forgot to do with v1 (sorry).

Thanks, and sorry for the false positives.
Also, I like coffee :)

> 
> -ed

Re: [PATCH net-next 1/7] sfc: initial debugfs implementation

[Nelson, Shannon]

On 12/11/2023 9:18 AM, edward.cree@amd.com wrote:
> 
> From: Edward Cree <ecree.xilinx@gmail.com>
> 

Hi Ed, a few minor nits I thought I'd call out.
Cheers,
sln

> Just a handful of nodes, including one enum with a string table for
>   pretty printing the values.

It would be nice to have a couple of example paths listed here

> 
> Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>   drivers/net/ethernet/sfc/Makefile       |   1 +
>   drivers/net/ethernet/sfc/debugfs.c      | 234 ++++++++++++++++++++++++
>   drivers/net/ethernet/sfc/debugfs.h      |  56 ++++++
>   drivers/net/ethernet/sfc/ef10.c         |  10 +
>   drivers/net/ethernet/sfc/ef100_netdev.c |   7 +
>   drivers/net/ethernet/sfc/ef100_nic.c    |   8 +
>   drivers/net/ethernet/sfc/efx.c          |  15 +-
>   drivers/net/ethernet/sfc/efx_common.c   |   7 +
>   drivers/net/ethernet/sfc/net_driver.h   |  29 +++
>   9 files changed, 366 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/net/ethernet/sfc/debugfs.c
>   create mode 100644 drivers/net/ethernet/sfc/debugfs.h
> 
> diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile
> index 8f446b9bd5ee..1fbdd04dc2c5 100644
> --- a/drivers/net/ethernet/sfc/Makefile
> +++ b/drivers/net/ethernet/sfc/Makefile
> @@ -12,6 +12,7 @@ sfc-$(CONFIG_SFC_MTD) += mtd.o
>   sfc-$(CONFIG_SFC_SRIOV)        += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \
>                              mae.o tc.o tc_bindings.o tc_counters.o \
>                              tc_encap_actions.o tc_conntrack.o
> +sfc-$(CONFIG_DEBUG_FS) += debugfs.o

Just as you are using #ifdef CONFIG_DEBUG_FS in your debugfs.h, you 
could use it in your debugfs.c and simply add the .o file to your sfc-y 
list here.

> 
>   obj-$(CONFIG_SFC)      += sfc.o
> 
> diff --git a/drivers/net/ethernet/sfc/debugfs.c b/drivers/net/ethernet/sfc/debugfs.c
> new file mode 100644
> index 000000000000..cf800addb4ff
> --- /dev/null
> +++ b/drivers/net/ethernet/sfc/debugfs.c
> @@ -0,0 +1,234 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/****************************************************************************
> + * Driver for Solarflare network controllers and boards
> + * Copyright 2023, Advanced Micro Devices, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation, incorporated herein by reference.
> + */
> +
> +#include "debugfs.h"
> +#include <linux/module.h>
> +#include <linux/debugfs.h>
> +#include <linux/dcache.h>
> +#include <linux/seq_file.h>
> +
> +/* Maximum length for a name component or symlink target */
> +#define EFX_DEBUGFS_NAME_LEN 32
> +
> +/* Top-level debug directory ([/sys/kernel]/debug/sfc) */
> +static struct dentry *efx_debug_root;
> +
> +/* "cards" directory ([/sys/kernel]/debug/sfc/cards) */
> +static struct dentry *efx_debug_cards;
> +
> +/**
> + * efx_init_debugfs_netdev - create debugfs sym-link for net device
> + * @net_dev:           Net device
> + *
> + * Create sym-link named after @net_dev to the debugfs directories for the
> + * corresponding NIC.  The sym-link must be cleaned up using
> + * efx_fini_debugfs_netdev().
> + *
> + * Return: a negative error code or 0 on success.
> + */
> +static int efx_init_debugfs_netdev(struct net_device *net_dev)
> +{
> +       struct efx_nic *efx = efx_netdev_priv(net_dev);
> +       char target[EFX_DEBUGFS_NAME_LEN];
> +       char name[EFX_DEBUGFS_NAME_LEN];
> +
> +       if (snprintf(name, sizeof(name), "nic_%s", net_dev->name) >=
> +                       sizeof(name))
> +               return -ENAMETOOLONG;
> +       if (snprintf(target, sizeof(target), "cards/%s", pci_name(efx->pci_dev))
> +           >= sizeof(target))
> +               return -ENAMETOOLONG;
> +       efx->debug_symlink = debugfs_create_symlink(name,
> +                                                   efx_debug_root, target);
> +       if (IS_ERR_OR_NULL(efx->debug_symlink))
> +               return efx->debug_symlink ? PTR_ERR(efx->debug_symlink) :
> +                                           -ENOMEM;
> +
> +       return 0;
> +}
> +
> +/**
> + * efx_fini_debugfs_netdev - remove debugfs sym-link for net device
> + * @net_dev:           Net device
> + *
> + * Remove sym-link created for @net_dev by efx_init_debugfs_netdev().
> + */
> +void efx_fini_debugfs_netdev(struct net_device *net_dev)
> +{
> +       struct efx_nic *efx = efx_netdev_priv(net_dev);
> +
> +       debugfs_remove(efx->debug_symlink);
> +       efx->debug_symlink = NULL;
> +}
> +
> +/* replace debugfs sym-links on net device rename */
> +void efx_update_debugfs_netdev(struct efx_nic *efx)
> +{
> +       mutex_lock(&efx->debugfs_symlink_mutex);
> +       if (efx->debug_symlink)
> +               efx_fini_debugfs_netdev(efx->net_dev);
> +       efx_init_debugfs_netdev(efx->net_dev);
> +       mutex_unlock(&efx->debugfs_symlink_mutex);
> +}

How necessary is this netdev symlink?  This seems like a bunch of extra 
maintenance.

> +
> +static int efx_debugfs_enum_read(struct seq_file *s, void *d)
> +{
> +       struct efx_debugfs_enum_data *data = s->private;
> +       u64 value = 0;
> +       size_t len;
> +
> +       len = min(data->vlen, sizeof(value));
> +#ifdef BIG_ENDIAN
> +       /* If data->value is narrower than u64, we need to copy into the
> +        * far end of value, as that's where the low bits live.
> +        */
> +       memcpy(((void *)&value) + sizeof(value) - len, data->value, len);
> +#else
> +       memcpy(&value, data->value, len);
> +#endif
> +       seq_printf(s, "%#llx => %s\n", value,
> +                  value < data->max ? data->names[value] : "(invalid)");
> +       return 0;
> +}
> +
> +static int efx_debugfs_enum_open(struct inode *inode, struct file *f)
> +{
> +       struct efx_debugfs_enum_data *data = inode->i_private;
> +
> +       return single_open(f, efx_debugfs_enum_read, data);
> +}
> +
> +static const struct file_operations efx_debugfs_enum_ops = {
> +       .owner = THIS_MODULE,
> +       .open = efx_debugfs_enum_open,
> +       .release = single_release,
> +       .read = seq_read,
> +       .llseek = seq_lseek,
> +};
> +
> +static void efx_debugfs_create_enum(const char *name, umode_t mode,
> +                                   struct dentry *parent,
> +                                   struct efx_debugfs_enum_data *data)
> +{
> +       debugfs_create_file(name, mode, parent, data, &efx_debugfs_enum_ops);
> +}
> +
> +static const char *const efx_interrupt_mode_names[] = {
> +       [EFX_INT_MODE_MSIX]   = "MSI-X",
> +       [EFX_INT_MODE_MSI]    = "MSI",
> +       [EFX_INT_MODE_LEGACY] = "legacy",
> +};
> +
> +#define EFX_DEBUGFS_EFX(_type, _name)  \
> +       debugfs_create_##_type(#_name, 0444, efx->debug_dir, &efx->_name)
> +
> +/* Create basic debugfs parameter files for an Efx NIC */
> +static void efx_init_debugfs_nic_files(struct efx_nic *efx)
> +{
> +       EFX_DEBUGFS_EFX(x32, rx_dma_len);
> +       EFX_DEBUGFS_EFX(x32, rx_buffer_order);
> +       EFX_DEBUGFS_EFX(x32, rx_buffer_truesize);
> +       efx->debug_interrupt_mode.max = ARRAY_SIZE(efx_interrupt_mode_names);
> +       efx->debug_interrupt_mode.names = efx_interrupt_mode_names;
> +       efx->debug_interrupt_mode.vlen = sizeof(efx->interrupt_mode);
> +       efx->debug_interrupt_mode.value = &efx->interrupt_mode;
> +       efx_debugfs_create_enum("interrupt_mode", 0444, efx->debug_dir,
> +                               &efx->debug_interrupt_mode);
> +}
> +
> +/**
> + * efx_init_debugfs_nic - create debugfs directory for NIC
> + * @efx:               Efx NIC
> + *
> + * Create debugfs directory containing parameter-files for @efx.
> + * The directory must be cleaned up using efx_fini_debugfs_nic().
> + *
> + * Return: a negative error code or 0 on success.
> + */
> +int efx_init_debugfs_nic(struct efx_nic *efx)
> +{
> +       /* Create directory */
> +       efx->debug_dir = debugfs_create_dir(pci_name(efx->pci_dev),
> +                                           efx_debug_cards);
> +       if (!efx->debug_dir)
> +               return -ENOMEM;
> +       efx_init_debugfs_nic_files(efx);
> +       return 0;
> +}
> +
> +/**
> + * efx_fini_debugfs_nic - remove debugfs directory for NIC
> + * @efx:               Efx NIC
> + *
> + * Remove debugfs directory created for @efx by efx_init_debugfs_nic().
> + */
> +void efx_fini_debugfs_nic(struct efx_nic *efx)
> +{
> +       debugfs_remove_recursive(efx->debug_dir);
> +       efx->debug_dir = NULL;
> +}
> +
> +/**
> + * efx_init_debugfs - create debugfs directories for sfc driver
> + *
> + * Create debugfs directories "sfc" and "sfc/cards".  This must be
> + * called before any of the other functions that create debugfs
> + * directories.  The directories must be cleaned up using
> + * efx_fini_debugfs().
> + *
> + * Return: a negative error code or 0 on success.
> + */
> +int efx_init_debugfs(void)
> +{
> +       int rc;
> +
> +       /* Create top-level directory */
> +       efx_debug_root = debugfs_create_dir(KBUILD_MODNAME, NULL);
> +       if (!efx_debug_root) {
> +               pr_err("debugfs_create_dir %s failed.\n", KBUILD_MODNAME);
> +               rc = -ENOMEM;
> +               goto err;
> +       } else if (IS_ERR(efx_debug_root)) {
> +               rc = PTR_ERR(efx_debug_root);
> +               pr_err("debugfs_create_dir %s failed, rc=%d.\n",
> +                      KBUILD_MODNAME, rc);
> +               goto err;
> +       }
> +
> +       /* Create "cards" directory */
> +       efx_debug_cards = debugfs_create_dir("cards", efx_debug_root);
> +       if (!efx_debug_cards) {
> +               pr_err("debugfs_create_dir cards failed.\n");
> +               rc = -ENOMEM;
> +               goto err;
> +       } else if (IS_ERR(efx_debug_cards)) {
> +               rc = PTR_ERR(efx_debug_cards);
> +               pr_err("debugfs_create_dir cards failed, rc=%d.\n", rc);
> +               goto err;
> +       }
> +
> +       return 0;
> +
> +err:
> +       efx_fini_debugfs();
> +       return rc;
> +}
> +
> +/**
> + * efx_fini_debugfs - remove debugfs directories for sfc driver
> + *
> + * Remove directories created by efx_init_debugfs().
> + */
> +void efx_fini_debugfs(void)
> +{
> +       debugfs_remove_recursive(efx_debug_root);
> +       efx_debug_cards = NULL;
> +       efx_debug_root = NULL;
> +}
> diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h
> new file mode 100644
> index 000000000000..1fe40fbffa5e
> --- /dev/null
> +++ b/drivers/net/ethernet/sfc/debugfs.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/****************************************************************************
> + * Driver for Solarflare network controllers and boards
> + * Copyright 2023, Advanced Micro Devices, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation, incorporated herein by reference.
> + */
> +
> +#ifndef EFX_DEBUGFS_H
> +#define EFX_DEBUGFS_H
> +#include "net_driver.h"
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +/**
> + * DOC: Directory layout for sfc debugfs
> + *
> + * At top level ([/sys/kernel]/debug/sfc) are per-netdev symlinks "nic_$name"
> + * and the "cards" directory.  For each PCI device to which the driver has
> + * bound and created a &struct efx_nic, there is a directory &efx_nic.debug_dir
> + * in "cards" whose name is the PCI address of the device; it is to this
> + * directory that the netdev symlink points.
> + */
> +
> +void efx_fini_debugfs_netdev(struct net_device *net_dev);
> +void efx_update_debugfs_netdev(struct efx_nic *efx);
> +
> +int efx_init_debugfs_nic(struct efx_nic *efx);
> +void efx_fini_debugfs_nic(struct efx_nic *efx);
> +
> +int efx_init_debugfs(void);
> +void efx_fini_debugfs(void);
> +
> +#else /* CONFIG_DEBUG_FS */
> +
> +static inline void efx_fini_debugfs_netdev(struct net_device *net_dev) {}
> +
> +static inline void efx_update_debugfs_netdev(struct efx_nic *efx) {}
> +
> +static inline int efx_init_debugfs_nic(struct efx_nic *efx)
> +{
> +       return 0;
> +}
> +static inline void efx_fini_debugfs_nic(struct efx_nic *efx) {}
> +
> +static inline int efx_init_debugfs(void)
> +{
> +       return 0;
> +}
> +static inline void efx_fini_debugfs(void) {}
> +
> +#endif /* CONFIG_DEBUG_FS */
> +
> +#endif /* EFX_DEBUGFS_H */
> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> index 6dfa062feebc..58e18fc92093 100644
> --- a/drivers/net/ethernet/sfc/ef10.c
> +++ b/drivers/net/ethernet/sfc/ef10.c
> @@ -19,6 +19,7 @@
>   #include "workarounds.h"
>   #include "selftest.h"
>   #include "ef10_sriov.h"
> +#include "debugfs.h"
>   #include <linux/in.h>
>   #include <linux/jhash.h>
>   #include <linux/wait.h>
> @@ -580,6 +581,13 @@ static int efx_ef10_probe(struct efx_nic *efx)
>          if (rc)
>                  goto fail3;
> 
> +       /* Populate debugfs */
> +#ifdef CONFIG_DEBUG_FS
> +       rc = efx_init_debugfs_nic(efx);
> +       if (rc)
> +               pci_err(efx->pci_dev, "failed to init device debugfs\n");
> +#endif

I don't think you need the ifdef here because you have the static 
version defined in debugfs.h

> +
>          rc = device_create_file(&efx->pci_dev->dev,
>                                  &dev_attr_link_control_flag);
>          if (rc)
> @@ -693,6 +701,7 @@ static int efx_ef10_probe(struct efx_nic *efx)
>   fail4:
>          device_remove_file(&efx->pci_dev->dev, &dev_attr_link_control_flag);
>   fail3:
> +       efx_fini_debugfs_nic(efx);
>          efx_mcdi_detach(efx);
> 
>          mutex_lock(&nic_data->udp_tunnels_lock);
> @@ -962,6 +971,7 @@ static void efx_ef10_remove(struct efx_nic *efx)
>          device_remove_file(&efx->pci_dev->dev, &dev_attr_link_control_flag);
> 
>          efx_mcdi_detach(efx);
> +       efx_fini_debugfs_nic(efx);
> 
>          memset(nic_data->udp_tunnels, 0, sizeof(nic_data->udp_tunnels));
>          mutex_lock(&nic_data->udp_tunnels_lock);
> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
> index 7f7d560cb2b4..e844d57754b7 100644
> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
> @@ -26,10 +26,12 @@
>   #include "tc_bindings.h"
>   #include "tc_encap_actions.h"
>   #include "efx_devlink.h"
> +#include "debugfs.h"
> 
>   static void ef100_update_name(struct efx_nic *efx)
>   {
>          strcpy(efx->name, efx->net_dev->name);
> +       efx_update_debugfs_netdev(efx);
>   }
> 
>   static int ef100_alloc_vis(struct efx_nic *efx, unsigned int *allocated_vis)
> @@ -405,6 +407,11 @@ void ef100_remove_netdev(struct efx_probe_data *probe_data)
>          ef100_pf_unset_devlink_port(efx);
>          efx_fini_tc(efx);
>   #endif
> +#ifdef CONFIG_DEBUG_FS
> +       mutex_lock(&efx->debugfs_symlink_mutex);
> +       efx_fini_debugfs_netdev(efx->net_dev);
> +       mutex_unlock(&efx->debugfs_symlink_mutex);
> +#endif

Can you do the mutex dance inside of efx_fini_debugfs_netdev() and then 
not need the ifdef here?

> 
>          down_write(&efx->filter_sem);
>          efx_mcdi_filter_table_remove(efx);
> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
> index 6da06931187d..ad378aa05dc3 100644
> --- a/drivers/net/ethernet/sfc/ef100_nic.c
> +++ b/drivers/net/ethernet/sfc/ef100_nic.c
> @@ -27,6 +27,7 @@
>   #include "tc.h"
>   #include "mae.h"
>   #include "rx_common.h"
> +#include "debugfs.h"
> 
>   #define EF100_MAX_VIS 4096
>   #define EF100_NUM_MCDI_BUFFERS 1
> @@ -1077,6 +1078,12 @@ static int ef100_probe_main(struct efx_nic *efx)
> 
>          /* Post-IO section. */
> 
> +       /* Populate debugfs */
> +#ifdef CONFIG_DEBUG_FS
> +       rc = efx_init_debugfs_nic(efx);
> +       if (rc)
> +               pci_err(efx->pci_dev, "failed to init device debugfs\n");
> +#endif

Shouldn't need the ifdef

>          rc = efx_mcdi_init(efx);
>          if (rc)
>                  goto fail;
> @@ -1213,6 +1220,7 @@ void ef100_remove(struct efx_nic *efx)
> 
>          efx_mcdi_detach(efx);
>          efx_mcdi_fini(efx);
> +       efx_fini_debugfs_nic(efx);
>          if (nic_data)
>                  efx_nic_free_buffer(efx, &nic_data->mcdi_buf);
>          kfree(nic_data);
> diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
> index 19f4b4d0b851..9266c7b5b4fd 100644
> --- a/drivers/net/ethernet/sfc/efx.c
> +++ b/drivers/net/ethernet/sfc/efx.c
> @@ -33,6 +33,7 @@
>   #include "selftest.h"
>   #include "sriov.h"
>   #include "efx_devlink.h"
> +#include "debugfs.h"
> 
>   #include "mcdi_port_common.h"
>   #include "mcdi_pcol.h"
> @@ -401,6 +402,11 @@ static void efx_remove_all(struct efx_nic *efx)
>   #endif
>          efx_remove_port(efx);
>          efx_remove_nic(efx);
> +#ifdef CONFIG_DEBUG_FS
> +       mutex_lock(&efx->debugfs_symlink_mutex);
> +       efx_fini_debugfs_netdev(efx->net_dev);
> +       mutex_unlock(&efx->debugfs_symlink_mutex);
> +#endif

Same mutex comment

>   }
> 
>   /**************************************************************************
> @@ -667,6 +673,7 @@ static void efx_update_name(struct efx_nic *efx)
>          strcpy(efx->name, efx->net_dev->name);
>          efx_mtd_rename(efx);
>          efx_set_channel_names(efx);
> +       efx_update_debugfs_netdev(efx);
>   }
> 
>   static int efx_netdev_event(struct notifier_block *this,
> @@ -1319,6 +1326,10 @@ static int __init efx_init_module(void)
> 
>          printk(KERN_INFO "Solarflare NET driver\n");
> 
> +       rc = efx_init_debugfs();
> +       if (rc)
> +               goto err_debugfs;
> +
>          rc = register_netdevice_notifier(&efx_netdev_notifier);
>          if (rc)
>                  goto err_notifier;
> @@ -1344,6 +1355,8 @@ static int __init efx_init_module(void)
>    err_reset:
>          unregister_netdevice_notifier(&efx_netdev_notifier);
>    err_notifier:
> +       efx_fini_debugfs();
> + err_debugfs:
>          return rc;
>   }
> 
> @@ -1355,7 +1368,7 @@ static void __exit efx_exit_module(void)
>          pci_unregister_driver(&efx_pci_driver);
>          efx_destroy_reset_workqueue();
>          unregister_netdevice_notifier(&efx_netdev_notifier);
> -
> +       efx_fini_debugfs();
>   }
> 
>   module_init(efx_init_module);
> diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
> index 175bd9cdfdac..7a9d6b6b66e5 100644
> --- a/drivers/net/ethernet/sfc/efx_common.c
> +++ b/drivers/net/ethernet/sfc/efx_common.c
> @@ -1022,6 +1022,9 @@ int efx_init_struct(struct efx_nic *efx, struct pci_dev *pci_dev)
>          INIT_WORK(&efx->mac_work, efx_mac_work);
>          init_waitqueue_head(&efx->flush_wq);
> 
> +#ifdef CONFIG_DEBUG_FS
> +       mutex_init(&efx->debugfs_symlink_mutex);
> +#endif

Can we do this without the ifdefs in the mainline code?
(okay, I'll stop grinding on that one for now)

>          efx->tx_queues_per_channel = 1;
>          efx->rxq_entries = EFX_DEFAULT_DMAQ_SIZE;
>          efx->txq_entries = EFX_DEFAULT_DMAQ_SIZE;
> @@ -1056,6 +1059,10 @@ void efx_fini_struct(struct efx_nic *efx)
> 
>          efx_fini_channels(efx);
> 
> +#ifdef CONFIG_DEBUG_FS
> +       mutex_destroy(&efx->debugfs_symlink_mutex);
> +#endif
> +
>          kfree(efx->vpd_sn);
> 
>          if (efx->workqueue) {
> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
> index 27d86e90a3bb..961e2db31c6e 100644
> --- a/drivers/net/ethernet/sfc/net_driver.h
> +++ b/drivers/net/ethernet/sfc/net_driver.h
> @@ -107,6 +107,24 @@ struct hwtstamp_config;
> 
>   struct efx_self_tests;
> 
> +/**
> + * struct efx_debugfs_enum_data - information for pretty-printing enums
> + * @value: pointer to the actual enum
> + * @vlen: sizeof the enum
> + * @names: array of names of enumerated values.  May contain some %NULL entries.
> + * @max: number of entries in @names, typically from ARRAY_SIZE()
> + *
> + * Where a driver struct contains an enum member which we wish to expose in
> + * debugfs, we also embed an instance of this struct, which
> + * efx_debugfs_enum_read() uses to pretty-print the value.
> + */
> +struct efx_debugfs_enum_data {
> +       void *value;
> +       size_t vlen;
> +       const char *const *names;
> +       unsigned int max;
> +};
> +
>   /**
>    * struct efx_buffer - A general-purpose DMA buffer
>    * @addr: host base address of the buffer
> @@ -1123,6 +1141,17 @@ struct efx_nic {
>          u32 rps_next_id;
>   #endif
> 
> +#ifdef CONFIG_DEBUG_FS
> +       /** @debug_dir: NIC debugfs directory */
> +       struct dentry *debug_dir;
> +       /** @debug_symlink: NIC debugfs symlink (``nic_eth%d``) */
> +       struct dentry *debug_symlink;
> +       /** @debug_interrupt_mode: debugfs details for printing @interrupt_mode */
> +       struct efx_debugfs_enum_data debug_interrupt_mode;
> +       /** @debugfs_symlink_mutex: protects debugfs @debug_symlink */
> +       struct mutex debugfs_symlink_mutex;
> +#endif
> +
>          atomic_t active_queues;
>          atomic_t rxq_flush_pending;
>          atomic_t rxq_flush_outstanding;
> 

Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues

[Nelson, Shannon]

On 12/11/2023 9:18 AM, edward.cree@amd.com wrote:
> 
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Expose each RX queue's core RXQ association, and the read/write/etc
>   pointers for the descriptor ring.
> Each RXQ dir also symlinks to its owning channel.
> 
> Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>   drivers/net/ethernet/sfc/debugfs.c    | 69 ++++++++++++++++++++++++++-
>   drivers/net/ethernet/sfc/debugfs.h    | 15 ++++++
>   drivers/net/ethernet/sfc/net_driver.h |  6 +++
>   drivers/net/ethernet/sfc/rx_common.c  |  9 ++++
>   4 files changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/sfc/debugfs.c b/drivers/net/ethernet/sfc/debugfs.c
> index b46339249794..43b1d06a985e 100644
> --- a/drivers/net/ethernet/sfc/debugfs.c
> +++ b/drivers/net/ethernet/sfc/debugfs.c
> @@ -78,6 +78,72 @@ void efx_update_debugfs_netdev(struct efx_nic *efx)
>          mutex_unlock(&efx->debugfs_symlink_mutex);
>   }
> 
> +#define EFX_DEBUGFS_RXQ(_type, _name)  \
> +       debugfs_create_##_type(#_name, 0444, rx_queue->debug_dir, &rx_queue->_name)
> +
> +/* Create basic debugfs parameter files for an Efx RXQ */
> +static void efx_init_debugfs_rx_queue_files(struct efx_rx_queue *rx_queue)
> +{
> +       EFX_DEBUGFS_RXQ(u32, core_index); /* actually an int */
> +       /* descriptor ring indices */
> +       EFX_DEBUGFS_RXQ(u32, added_count);
> +       EFX_DEBUGFS_RXQ(u32, notified_count);
> +       EFX_DEBUGFS_RXQ(u32, granted_count);
> +       EFX_DEBUGFS_RXQ(u32, removed_count);
> +}
> +
> +/**
> + * efx_init_debugfs_rx_queue - create debugfs directory for RX queue
> + * @rx_queue:          Efx RX queue
> + *
> + * Create a debugfs directory containing parameter-files for @rx_queue.
> + * The directory must be cleaned up using efx_fini_debugfs_rx_queue(),
> + * even if this function returns an error.
> + *
> + * Return: a negative error code or 0 on success.
> + */
> +int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue)
> +{
> +       struct efx_channel *channel = efx_rx_queue_channel(rx_queue);
> +       char target[EFX_DEBUGFS_NAME_LEN];
> +       char name[EFX_DEBUGFS_NAME_LEN];
> +
> +       if (!rx_queue->efx->debug_queues_dir)
> +               return -ENODEV;
> +       /* Create directory */
> +       if (snprintf(name, sizeof(name), "rx-%d", efx_rx_queue_index(rx_queue))

Adding leading 0's here can be helpful for directory entry sorting

> +           >= sizeof(name))
> +               return -ENAMETOOLONG;
> +       rx_queue->debug_dir = debugfs_create_dir(name,
> +                                                rx_queue->efx->debug_queues_dir);
> +       if (!rx_queue->debug_dir)
> +               return -ENOMEM;
> +
> +       /* Create files */
> +       efx_init_debugfs_rx_queue_files(rx_queue);
> +
> +       /* Create symlink to channel */
> +       if (snprintf(target, sizeof(target), "../../channels/%d",
> +                    channel->channel) >= sizeof(target))
> +               return -ENAMETOOLONG;
> +       if (!debugfs_create_symlink("channel", rx_queue->debug_dir, target))
> +               return -ENOMEM;

If these fail, should you clean up the earlier create_dir()?


> +
> +       return 0;
> +}
> +
> +/**
> + * efx_fini_debugfs_rx_queue - remove debugfs directory for RX queue
> + * @rx_queue:          Efx RX queue
> + *
> + * Remove directory created for @rx_queue by efx_init_debugfs_rx_queue().
> + */
> +void efx_fini_debugfs_rx_queue(struct efx_rx_queue *rx_queue)
> +{
> +       debugfs_remove_recursive(rx_queue->debug_dir);
> +       rx_queue->debug_dir = NULL;
> +}
> +
>   #define EFX_DEBUGFS_CHANNEL(_type, _name)      \
>          debugfs_create_##_type(#_name, 0444, channel->debug_dir, &channel->_name)
> 
> @@ -208,9 +274,10 @@ int efx_init_debugfs_nic(struct efx_nic *efx)
>          if (!efx->debug_dir)
>                  return -ENOMEM;
>          efx_init_debugfs_nic_files(efx);
> -       /* Create channels subdirectory */
> +       /* Create subdirectories */
>          efx->debug_channels_dir = debugfs_create_dir("channels",
>                                                       efx->debug_dir);
> +       efx->debug_queues_dir = debugfs_create_dir("queues", efx->debug_dir);
>          return 0;
>   }
> 
> diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h
> index 4af4a03d1b97..53c98a2fb4c9 100644
> --- a/drivers/net/ethernet/sfc/debugfs.h
> +++ b/drivers/net/ethernet/sfc/debugfs.h
> @@ -28,11 +28,20 @@
>    * * "channels/" (&efx_nic.debug_channels_dir).  For each channel, this will
>    *   contain a directory (&efx_channel.debug_dir), whose name is the channel
>    *   index (in decimal).
> + * * "queues/" (&efx_nic.debug_queues_dir).
> + *
> + *   * For each NIC RX queue, this will contain a directory
> + *     (&efx_rx_queue.debug_dir), whose name is "rx-N" where N is the RX queue
> + *     index.  (This may not be the same as the kernel core RX queue index.)
> + *     The directory will contain a symlink to the owning channel.
>    */
> 
>   void efx_fini_debugfs_netdev(struct net_device *net_dev);
>   void efx_update_debugfs_netdev(struct efx_nic *efx);
> 
> +int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue);
> +void efx_fini_debugfs_rx_queue(struct efx_rx_queue *rx_queue);
> +
>   int efx_init_debugfs_channel(struct efx_channel *channel);
>   void efx_fini_debugfs_channel(struct efx_channel *channel);
> 
> @@ -48,6 +57,12 @@ static inline void efx_fini_debugfs_netdev(struct net_device *net_dev) {}
> 
>   static inline void efx_update_debugfs_netdev(struct efx_nic *efx) {}
> 
> +int efx_init_debugfs_rx_queue(struct efx_rx_queue *rx_queue)
> +{
> +       return 0;
> +}
> +void efx_fini_debugfs_rx_queue(struct efx_rx_queue *rx_queue) {}
> +
>   int efx_init_debugfs_channel(struct efx_channel *channel)
>   {
>          return 0;
> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
> index 2b92c5461fe3..63eb32670826 100644
> --- a/drivers/net/ethernet/sfc/net_driver.h
> +++ b/drivers/net/ethernet/sfc/net_driver.h
> @@ -424,6 +424,10 @@ struct efx_rx_queue {
>          struct work_struct grant_work;
>          /* Statistics to supplement MAC stats */
>          unsigned long rx_packets;
> +#ifdef CONFIG_DEBUG_FS
> +       /** @debug_dir: Queue debugfs directory (under @efx->debug_queues_dir) */
> +       struct dentry *debug_dir;
> +#endif
>          struct xdp_rxq_info xdp_rxq_info;
>          bool xdp_rxq_info_valid;
>   };
> @@ -1150,6 +1154,8 @@ struct efx_nic {
>          struct dentry *debug_dir;
>          /** @debug_channels_dir: contains channel debugfs dirs.  Under @debug_dir */
>          struct dentry *debug_channels_dir;
> +       /** @debug_queues_dir: contains RX/TX queue debugfs dirs.  Under @debug_dir */
> +       struct dentry *debug_queues_dir;
>          /** @debug_symlink: NIC debugfs symlink (``nic_eth%d``) */
>          struct dentry *debug_symlink;
>          /** @debug_interrupt_mode: debugfs details for printing @interrupt_mode */
> diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
> index d2f35ee15eff..7f63f70f082d 100644
> --- a/drivers/net/ethernet/sfc/rx_common.c
> +++ b/drivers/net/ethernet/sfc/rx_common.c
> @@ -14,6 +14,7 @@
>   #include "efx.h"
>   #include "nic.h"
>   #include "rx_common.h"
> +#include "debugfs.h"
> 
>   /* This is the percentage fill level below which new RX descriptors
>    * will be added to the RX descriptor ring.
> @@ -208,6 +209,12 @@ int efx_probe_rx_queue(struct efx_rx_queue *rx_queue)
>          if (!rx_queue->buffer)
>                  return -ENOMEM;
> 
> +       rc = efx_init_debugfs_rx_queue(rx_queue);
> +       if (rc) /* not fatal */
> +               netif_err(efx, drv, efx->net_dev,
> +                         "Failed to create debugfs for RXQ %d, rc=%d\n",
> +                         efx_rx_queue_index(rx_queue), rc);
> +
>          rc = efx_nic_probe_rx(rx_queue);
>          if (rc) {
>                  kfree(rx_queue->buffer);
> @@ -311,6 +318,8 @@ void efx_remove_rx_queue(struct efx_rx_queue *rx_queue)
> 
>          efx_nic_remove_rx(rx_queue);
> 
> +       efx_fini_debugfs_rx_queue(rx_queue);
> +
>          kfree(rx_queue->buffer);
>          rx_queue->buffer = NULL;
>   }
> 

Re: [PATCH net-next 6/7] sfc: add debugfs entries for filter table status

[Nelson, Shannon]

On 12/11/2023 9:18 AM, edward.cree@amd.com wrote:
> 
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Filter table management is complicated by the possibility of overflow
>   kicking us into a promiscuous fallback for either unicast or multicast.
>   Expose the internal flags that drive this.
> Since the table state (efx->filter_state) has a separate, shorter
>   lifetime than struct efx_nic, put its debugfs nodes in a subdirectory
>   (efx->filter_state->debug_dir) so that they can be cleaned up easily
>   before the filter_state is freed.
> 
> Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>   drivers/net/ethernet/sfc/debugfs.h      |  4 ++++
>   drivers/net/ethernet/sfc/mcdi_filters.c | 18 ++++++++++++++++++
>   drivers/net/ethernet/sfc/mcdi_filters.h |  4 ++++
>   3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h
> index 3e8d2e2b5bad..7a96f3798cbd 100644
> --- a/drivers/net/ethernet/sfc/debugfs.h
> +++ b/drivers/net/ethernet/sfc/debugfs.h
> @@ -39,6 +39,10 @@
>    *     index.  (This may differ from both the kernel core TX queue index and
>    *     the hardware queue label of the TXQ.)
>    *     The directory will contain a symlink to the owning channel.
> + *
> + * * "filters/" (&efx_mcdi_filter_table.debug_dir).
> + *   This contains parameter files for the NIC receive filter table
> + *   (@efx->filter_state).
>    */
> 
>   void efx_fini_debugfs_netdev(struct net_device *net_dev);
> diff --git a/drivers/net/ethernet/sfc/mcdi_filters.c b/drivers/net/ethernet/sfc/mcdi_filters.c
> index 4ff6586116ee..a4ab45082c8f 100644
> --- a/drivers/net/ethernet/sfc/mcdi_filters.c
> +++ b/drivers/net/ethernet/sfc/mcdi_filters.c
> @@ -1348,6 +1348,20 @@ int efx_mcdi_filter_table_probe(struct efx_nic *efx, bool multicast_chaining)
>          INIT_LIST_HEAD(&table->vlan_list);
>          init_rwsem(&table->lock);
> 
> +#ifdef CONFIG_DEBUG_FS
> +       table->debug_dir = debugfs_create_dir("filters", efx->debug_dir);
> +       debugfs_create_bool("uc_promisc", 0444, table->debug_dir,
> +                           &table->uc_promisc);
> +       debugfs_create_bool("mc_promisc", 0444, table->debug_dir,
> +                           &table->mc_promisc);
> +       debugfs_create_bool("mc_promisc_last", 0444, table->debug_dir,
> +                           &table->mc_promisc_last);
> +       debugfs_create_bool("mc_overflow", 0444, table->debug_dir,
> +                           &table->mc_overflow);
> +       debugfs_create_bool("mc_chaining", 0444, table->debug_dir,
> +                           &table->mc_chaining);
> +#endif

It would be good to continue the practice of using the debugfs_* 
primitives in your debugfs.c and just make a single call here that 
doesn't need the ifdef

> +
>          efx->filter_state = table;
> 
>          return 0;
> @@ -1518,6 +1532,10 @@ void efx_mcdi_filter_table_remove(struct efx_nic *efx)
>                  return;
> 
>          vfree(table->entry);
> +#ifdef CONFIG_DEBUG_FS
> +       /* Remove debugfs entries pointing into @table */
> +       debugfs_remove_recursive(table->debug_dir);
> +#endif
>          kfree(table);
>   }
> 
> diff --git a/drivers/net/ethernet/sfc/mcdi_filters.h b/drivers/net/ethernet/sfc/mcdi_filters.h
> index c0d6558b9fd2..897843ade3ec 100644
> --- a/drivers/net/ethernet/sfc/mcdi_filters.h
> +++ b/drivers/net/ethernet/sfc/mcdi_filters.h
> @@ -91,6 +91,10 @@ struct efx_mcdi_filter_table {
>          bool vlan_filter;
>          /* Entries on the vlan_list are added/removed under filter_sem */
>          struct list_head vlan_list;
> +#ifdef CONFIG_DEBUG_FS
> +       /* filter table debugfs directory */
> +       struct dentry *debug_dir;
> +#endif
>   };
> 
>   int efx_mcdi_filter_table_probe(struct efx_nic *efx, bool multicast_chaining);
> 

Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents

[Nelson, Shannon]

On 12/11/2023 9:18 AM, edward.cree@amd.com wrote:
> 
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Expose the filter table entries.
> 
> Reviewed-by: Jonathan Cooper <jonathan.s.cooper@amd.com>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
>   drivers/net/ethernet/sfc/debugfs.c      | 117 +++++++++++++++++++++++-
>   drivers/net/ethernet/sfc/debugfs.h      |  45 +++++++++
>   drivers/net/ethernet/sfc/mcdi_filters.c |  39 ++++++++
>   3 files changed, 197 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/debugfs.c b/drivers/net/ethernet/sfc/debugfs.c
> index 549ff1ee273e..e67b0fc927fe 100644
> --- a/drivers/net/ethernet/sfc/debugfs.c
> +++ b/drivers/net/ethernet/sfc/debugfs.c
> @@ -9,10 +9,6 @@
>    */
> 
>   #include "debugfs.h"
> -#include <linux/module.h>
> -#include <linux/debugfs.h>
> -#include <linux/dcache.h>
> -#include <linux/seq_file.h>

Can you leave these out of the original patch and not have to remove 
them here?

> 
>   /* Maximum length for a name component or symlink target */
>   #define EFX_DEBUGFS_NAME_LEN 32
> @@ -428,3 +424,116 @@ void efx_fini_debugfs(void)
>          efx_debug_cards = NULL;
>          efx_debug_root = NULL;
>   }
> +
> +/**
> + * efx_debugfs_print_filter - format a filter spec for display
> + * @s: buffer to write result into
> + * @l: length of buffer @s
> + * @spec: filter specification
> + */
> +void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec)
> +{
> +       u32 ip[4];
> +       int p = snprintf(s, l, "match=%#x,pri=%d,flags=%#x,q=%d",
> +                        spec->match_flags, spec->priority, spec->flags,
> +                        spec->dmaq_id);
> +
> +       if (spec->vport_id)
> +               p += snprintf(s + p, l - p, ",vport=%#x", spec->vport_id);
> +
> +       if (spec->flags & EFX_FILTER_FLAG_RX_RSS)
> +               p += snprintf(s + p, l - p, ",rss=%#x", spec->rss_context);
> +
> +       if (spec->match_flags & EFX_FILTER_MATCH_OUTER_VID)
> +               p += snprintf(s + p, l - p,
> +                             ",ovid=%d", ntohs(spec->outer_vid));
> +       if (spec->match_flags & EFX_FILTER_MATCH_INNER_VID)
> +               p += snprintf(s + p, l - p,
> +                             ",ivid=%d", ntohs(spec->inner_vid));
> +       if (spec->match_flags & EFX_FILTER_MATCH_ENCAP_TYPE)
> +               p += snprintf(s + p, l - p,
> +                             ",encap=%d", spec->encap_type);
> +       if (spec->match_flags & EFX_FILTER_MATCH_LOC_MAC)
> +               p += snprintf(s + p, l - p,
> +                             ",lmac=%02x:%02x:%02x:%02x:%02x:%02x",
> +                             spec->loc_mac[0], spec->loc_mac[1],
> +                             spec->loc_mac[2], spec->loc_mac[3],
> +                             spec->loc_mac[4], spec->loc_mac[5]);
> +       if (spec->match_flags & EFX_FILTER_MATCH_REM_MAC)
> +               p += snprintf(s + p, l - p,
> +                             ",rmac=%02x:%02x:%02x:%02x:%02x:%02x",
> +                             spec->rem_mac[0], spec->rem_mac[1],
> +                             spec->rem_mac[2], spec->rem_mac[3],
> +                             spec->rem_mac[4], spec->rem_mac[5]);
> +       if (spec->match_flags & EFX_FILTER_MATCH_ETHER_TYPE)
> +               p += snprintf(s + p, l - p,
> +                             ",ether=%#x", ntohs(spec->ether_type));
> +       if (spec->match_flags & EFX_FILTER_MATCH_IP_PROTO)
> +               p += snprintf(s + p, l - p,
> +                             ",ippr=%#x", spec->ip_proto);
> +       if (spec->match_flags & EFX_FILTER_MATCH_LOC_HOST) {
> +               if (ntohs(spec->ether_type) == ETH_P_IP) {
> +                       ip[0] = (__force u32) spec->loc_host[0];
> +                       p += snprintf(s + p, l - p,
> +                                     ",lip=%d.%d.%d.%d",
> +                                     ip[0] & 0xff,
> +                                     (ip[0] >> 8) & 0xff,
> +                                     (ip[0] >> 16) & 0xff,
> +                                     (ip[0] >> 24) & 0xff);
> +               } else if (ntohs(spec->ether_type) == ETH_P_IPV6) {
> +                       ip[0] = (__force u32) spec->loc_host[0];
> +                       ip[1] = (__force u32) spec->loc_host[1];
> +                       ip[2] = (__force u32) spec->loc_host[2];
> +                       ip[3] = (__force u32) spec->loc_host[3];
> +                       p += snprintf(s + p, l - p,
> +                                     ",lip=%04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x",
> +                                     ip[0] & 0xffff,
> +                                     (ip[0] >> 16) & 0xffff,
> +                                     ip[1] & 0xffff,
> +                                     (ip[1] >> 16) & 0xffff,
> +                                     ip[2] & 0xffff,
> +                                     (ip[2] >> 16) & 0xffff,
> +                                     ip[3] & 0xffff,
> +                                     (ip[3] >> 16) & 0xffff);
> +               } else {
> +                       p += snprintf(s + p, l - p, ",lip=?");
> +               }
> +       }
> +       if (spec->match_flags & EFX_FILTER_MATCH_REM_HOST) {
> +               if (ntohs(spec->ether_type) == ETH_P_IP) {
> +                       ip[0] = (__force u32) spec->rem_host[0];
> +                       p += snprintf(s + p, l - p,
> +                                     ",rip=%d.%d.%d.%d",
> +                                     ip[0] & 0xff,
> +                                     (ip[0] >> 8) & 0xff,
> +                                     (ip[0] >> 16) & 0xff,
> +                                     (ip[0] >> 24) & 0xff);
> +               } else if (ntohs(spec->ether_type) == ETH_P_IPV6) {
> +                       ip[0] = (__force u32) spec->rem_host[0];
> +                       ip[1] = (__force u32) spec->rem_host[1];
> +                       ip[2] = (__force u32) spec->rem_host[2];
> +                       ip[3] = (__force u32) spec->rem_host[3];
> +                       p += snprintf(s + p, l - p,
> +                                     ",rip=%04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x",
> +                                     ip[0] & 0xffff,
> +                                     (ip[0] >> 16) & 0xffff,
> +                                     ip[1] & 0xffff,
> +                                     (ip[1] >> 16) & 0xffff,
> +                                     ip[2] & 0xffff,
> +                                     (ip[2] >> 16) & 0xffff,
> +                                     ip[3] & 0xffff,
> +                                     (ip[3] >> 16) & 0xffff);
> +               } else {
> +                       p += snprintf(s + p, l - p, ",rip=?");
> +               }

Since you have this code more than once, it might be a candidate for a 
utility function, if one doesn't already exist somewhere in the kernel 
already.

> +       }
> +       if (spec->match_flags & EFX_FILTER_MATCH_LOC_PORT)
> +               p += snprintf(s + p, l - p,
> +                             ",lport=%d", ntohs(spec->loc_port));
> +       if (spec->match_flags & EFX_FILTER_MATCH_REM_PORT)
> +               p += snprintf(s + p, l - p,
> +                             ",rport=%d", ntohs(spec->rem_port));
> +       if (spec->match_flags & EFX_FILTER_MATCH_LOC_MAC_IG)
> +               p += snprintf(s + p, l - p, ",%s",
> +                             spec->loc_mac[0] ? "mc" : "uc");
> +}
> diff --git a/drivers/net/ethernet/sfc/debugfs.h b/drivers/net/ethernet/sfc/debugfs.h
> index 7a96f3798cbd..f50b4bf33a6b 100644
> --- a/drivers/net/ethernet/sfc/debugfs.h
> +++ b/drivers/net/ethernet/sfc/debugfs.h
> @@ -10,6 +10,10 @@
> 
>   #ifndef EFX_DEBUGFS_H
>   #define EFX_DEBUGFS_H
> +#include <linux/module.h>
> +#include <linux/debugfs.h>
> +#include <linux/dcache.h>
> +#include <linux/seq_file.h>
>   #include "net_driver.h"
> 
>   #ifdef CONFIG_DEBUG_FS
> @@ -63,6 +67,45 @@ void efx_fini_debugfs_nic(struct efx_nic *efx);
>   int efx_init_debugfs(void);
>   void efx_fini_debugfs(void);
> 
> +void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec);
> +
> +/* Generate operations for a debugfs node with a custom reader function.
> + * The reader should have signature int (*)(struct seq_file *s, void *data)
> + * where data is the pointer passed to EFX_DEBUGFS_CREATE_RAW.
> + */
> +#define EFX_DEBUGFS_RAW_PARAMETER(_reader)                                    \
> +                                                                              \
> +static int efx_debugfs_##_reader##_read(struct seq_file *s, void *d)          \
> +{                                                                             \
> +       return _reader(s, s->private);                                         \
> +}                                                                             \
> +                                                                              \
> +static int efx_debugfs_##_reader##_open(struct inode *inode, struct file *f)   \
> +{                                                                             \
> +       return single_open(f, efx_debugfs_##_reader##_read, inode->i_private); \
> +}                                                                             \
> +                                                                              \
> +static const struct file_operations efx_debugfs_##_reader##_ops = {           \
> +       .owner = THIS_MODULE,                                                  \
> +       .open = efx_debugfs_##_reader##_open,                                  \
> +       .release = single_release,                                             \
> +       .read = seq_read,                                                      \
> +       .llseek = seq_lseek,                                                   \
> +};                                                                            \
> +                                                                              \
> +static void efx_debugfs_create_##_reader(const char *name, umode_t mode,       \
> +                                        struct dentry *parent, void *data)    \
> +{                                                                             \
> +       debugfs_create_file(name, mode, parent, data,                          \
> +                           &efx_debugfs_##_reader##_ops);                     \
> +}
> +
> +/* Instantiate a debugfs node with a custom reader function.  The operations
> + * must have been generated with EFX_DEBUGFS_RAW_PARAMETER(_reader).
> + */
> +#define EFX_DEBUGFS_CREATE_RAW(_name, _mode, _parent, _data, _reader)         \
> +               efx_debugfs_create_##_reader(_name, _mode, _parent, _data)
> +
>   #else /* CONFIG_DEBUG_FS */
> 
>   static inline void efx_fini_debugfs_netdev(struct net_device *net_dev) {}
> @@ -99,6 +142,8 @@ static inline int efx_init_debugfs(void)
>   }
>   static inline void efx_fini_debugfs(void) {}
> 
> +void efx_debugfs_print_filter(char *s, size_t l, struct efx_filter_spec *spec) {}
> +
>   #endif /* CONFIG_DEBUG_FS */
> 
>   #endif /* EFX_DEBUGFS_H */
> diff --git a/drivers/net/ethernet/sfc/mcdi_filters.c b/drivers/net/ethernet/sfc/mcdi_filters.c
> index a4ab45082c8f..465226c3e8c7 100644
> --- a/drivers/net/ethernet/sfc/mcdi_filters.c
> +++ b/drivers/net/ethernet/sfc/mcdi_filters.c
> @@ -13,6 +13,7 @@
>   #include "mcdi.h"
>   #include "nic.h"
>   #include "rx_common.h"
> +#include "debugfs.h"
> 
>   /* The maximum size of a shared RSS context */
>   /* TODO: this should really be from the mcdi protocol export */
> @@ -1173,6 +1174,42 @@ s32 efx_mcdi_filter_get_rx_ids(struct efx_nic *efx,
>          return count;
>   }
> 
> +static int efx_debugfs_read_filter_list(struct seq_file *file, void *data)
> +{
> +       struct efx_mcdi_filter_table *table;
> +       struct efx_nic *efx = data;
> +       int i;
> +
> +       down_read(&efx->filter_sem);
> +       table = efx->filter_state;
> +       if (!table || !table->entry) {
> +               up_read(&efx->filter_sem);
> +               return -ENETDOWN;
> +       }
> +
> +       /* deliberately don't lock the table->lock, so that we can
> +        * still dump the table even if we hang mid-operation.
> +        */
> +       for (i = 0; i < EFX_MCDI_FILTER_TBL_ROWS; ++i) {
> +               struct efx_filter_spec *spec =
> +                       efx_mcdi_filter_entry_spec(table, i);
> +               char filter[256];
> +
> +               if (spec) {
> +                       efx_debugfs_print_filter(filter, sizeof(filter), spec);
> +
> +                       seq_printf(file, "%d[%#04llx],%#x = %s\n",
> +                                  i, table->entry[i].handle & 0xffff,
> +                                  efx_mcdi_filter_entry_flags(table, i),
> +                                  filter);
> +               }
> +       }
> +
> +       up_read(&efx->filter_sem);
> +       return 0;
> +}
> +EFX_DEBUGFS_RAW_PARAMETER(efx_debugfs_read_filter_list);
> +
>   static int efx_mcdi_filter_match_flags_from_mcdi(bool encap, u32 mcdi_flags)
>   {
>          int match_flags = 0;
> @@ -1360,6 +1397,8 @@ int efx_mcdi_filter_table_probe(struct efx_nic *efx, bool multicast_chaining)
>                              &table->mc_overflow);
>          debugfs_create_bool("mc_chaining", 0444, table->debug_dir,
>                              &table->mc_chaining);
> +       EFX_DEBUGFS_CREATE_RAW("entries", 0444, table->debug_dir, efx,
> +                              efx_debugfs_read_filter_list);
>   #endif
> 
>          efx->filter_state = table;
> 

Re: [PATCH net-next 1/7] sfc: initial debugfs implementation

[Edward Cree]

On 15/12/2023 00:05, Nelson, Shannon wrote:
> It would be nice to have a couple of example paths listed here

Sure; added this to v2:
See included DOC: comment for directory structure; leaf nodes are
 rx_dma_len, rx_buffer_order, rx_buffer_truesize and interrupt_mode.
Is that what you had in mind?  Or more like
 "grep -H . /sys/kernel/debug/sfc" output on a running system?

>> +/* replace debugfs sym-links on net device rename */
>> +void efx_update_debugfs_netdev(struct efx_nic *efx)
>> +{
>> +       mutex_lock(&efx->debugfs_symlink_mutex);
>> +       if (efx->debug_symlink)
>> +               efx_fini_debugfs_netdev(efx->net_dev);
>> +       efx_init_debugfs_netdev(efx->net_dev);
>> +       mutex_unlock(&efx->debugfs_symlink_mutex);
>> +}
> 
> How necessary is this netdev symlink?  This seems like a bunch of extra maintenance.

AFAIK we've had it out-of-tree for a very long time and not found it
 to need any real maintenance effort.  And while it's not strictly
 necessary, it is fairly convenient.

>> +       /* Populate debugfs */
>> +#ifdef CONFIG_DEBUG_FS
>> +       rc = efx_init_debugfs_nic(efx);
>> +       if (rc)
>> +               pci_err(efx->pci_dev, "failed to init device debugfs\n");
>> +#endif
> 
> I don't think you need the ifdef here because you have the static version defined in debugfs.h

You're right; I'll fix these.

>> +#ifdef CONFIG_DEBUG_FS
>> +       mutex_lock(&efx->debugfs_symlink_mutex);
>> +       efx_fini_debugfs_netdev(efx->net_dev);
>> +       mutex_unlock(&efx->debugfs_symlink_mutex);
>> +#endif
> 
> Can you do the mutex dance inside of efx_fini_debugfs_netdev() and then not need the ifdef here?

Yes, although I needed to refactor slightly because it's also
 called by efx_update_debugfs_netdev() which is already holding
 the mutex.

>> diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
>> index 175bd9cdfdac..7a9d6b6b66e5 100644
>> --- a/drivers/net/ethernet/sfc/efx_common.c
>> +++ b/drivers/net/ethernet/sfc/efx_common.c
>> @@ -1022,6 +1022,9 @@ int efx_init_struct(struct efx_nic *efx, struct pci_dev *pci_dev)
>>          INIT_WORK(&efx->mac_work, efx_mac_work);
>>          init_waitqueue_head(&efx->flush_wq);
>>
>> +#ifdef CONFIG_DEBUG_FS
>> +       mutex_init(&efx->debugfs_symlink_mutex);
>> +#endif
> 
> Can we do this without the ifdefs in the mainline code?
> (okay, I'll stop grinding on that one for now)

Ifdefs for struct members that may not exist seems to be the
 existing pattern in efx_init_struct and efx_fini_struct, so
 I'd rather leave this here than wrap this single call in an
 efx_init_struct_debugfs function.

Thanks for the review!

Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues

[Edward Cree]

On 15/12/2023—
wait, has it really been more than a year?  Yikes.

On 15/12/2023 00:05, Nelson, Shannon wrote:
> On 12/11/2023 9:18 AM, edward.cree@amd.com wrote:
>> +       if (snprintf(name, sizeof(name), "rx-%d", efx_rx_queue_index(rx_queue))
> 
> Adding leading 0's here can be helpful for directory entry sorting

True, but it's not clear how many to use — the hardware supports over
 1000 in principle, and in practice it's normal to have one per core
 (and more than that on TX) which can get over 100 on powerful systems.
Yet on something like an 8-core box having queues 000 to 007 just looks
 silly imho.  I don't plan to change this line in v2.

>> +           >= sizeof(name))
>> +               return -ENAMETOOLONG;
>> +       rx_queue->debug_dir = debugfs_create_dir(name,
>> +                                                rx_queue->efx->debug_queues_dir);
>> +       if (!rx_queue->debug_dir)
>> +               return -ENOMEM;
>> +
>> +       /* Create files */
>> +       efx_init_debugfs_rx_queue_files(rx_queue);
>> +
>> +       /* Create symlink to channel */
>> +       if (snprintf(target, sizeof(target), "../../channels/%d",
>> +                    channel->channel) >= sizeof(target))
>> +               return -ENAMETOOLONG;
>> +       if (!debugfs_create_symlink("channel", rx_queue->debug_dir, target))
>> +               return -ENOMEM;
> 
> If these fail, should you clean up the earlier create_dir()?

No; these errors mean "we didn't do everything we wanted to", not
 "it's all broken", and the files/dir previously created are still
 useful.  The caller treats errors as non-fatal.
See also the kdoc comment on efx_init_debugfs_rx_queue:
>> + * The directory must be cleaned up using efx_fini_debugfs_rx_queue(),
>> + * even if this function returns an error.

I can't think of a suitable comment on these return statements to
 clarify this, but suggestions are welcome.

Re: [PATCH net-next 6/7] sfc: add debugfs entries for filter table status

[Edward Cree]

On 15/12/2023 00:05, Nelson, Shannon wrote:
> On 12/11/2023 9:18 AM, edward.cree@amd.com wrote:
>> +#ifdef CONFIG_DEBUG_FS
>> +       table->debug_dir = debugfs_create_dir("filters", efx->debug_dir);
>> +       debugfs_create_bool("uc_promisc", 0444, table->debug_dir,
>> +                           &table->uc_promisc);
>> +       debugfs_create_bool("mc_promisc", 0444, table->debug_dir,
>> +                           &table->mc_promisc);
>> +       debugfs_create_bool("mc_promisc_last", 0444, table->debug_dir,
>> +                           &table->mc_promisc_last);
>> +       debugfs_create_bool("mc_overflow", 0444, table->debug_dir,
>> +                           &table->mc_overflow);
>> +       debugfs_create_bool("mc_chaining", 0444, table->debug_dir,
>> +                           &table->mc_chaining);
>> +#endif
> 
> It would be good to continue the practice of using the debugfs_* primitives in your debugfs.c and just make a single call here that doesn't need the ifdef

I'm still in two minds about this.  While it makes sense in isolation
 to do it that way here, in the following patch we add a more complex
 dumper, and I think it makes more sense to put that in mcdi_filters.c
 and have filters code know a bit about debugfs, than put it in
 debugfs.c and have debug code know *everything* about filters — and
 every other part of the driver that subsequently gets its own debug
 nodes.
I already have some follow-up patches that add EF100 MAE debugfs nodes
 which also have custom dumpers, but those are in a separate file
 (tc_debugfs.c) because there are a lot of them and tc/mae code is
 already split into several pieces, whereas I'm not sure whether
 adding a separate file for filter-table debugfs code really makes
 sense, or whether it's sufficient just to refactor this code into
 a(n unconditionally-called) function that continues to live in
 mcdi_filters.c and has the ifdef within it.

Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues

[Nelson, Shannon]

On 2/14/2025 7:51 AM, Edward Cree wrote:
> 
> On 15/12/2023—
> wait, has it really been more than a year?  Yikes.

Time flies when we're having all this fun?

> 
> On 15/12/2023 00:05, Nelson, Shannon wrote:
>> On 12/11/2023 9:18 AM, edward.cree@amd.com wrote:
>>> +       if (snprintf(name, sizeof(name), "rx-%d", efx_rx_queue_index(rx_queue))
>>
>> Adding leading 0's here can be helpful for directory entry sorting
> 
> True, but it's not clear how many to use — the hardware supports over
>   1000 in principle, and in practice it's normal to have one per core
>   (and more than that on TX) which can get over 100 on powerful systems.
> Yet on something like an 8-core box having queues 000 to 007 just looks
>   silly imho.  I don't plan to change this line in v2.

I suppose you could do something tricky with %0*d and log10(num_queues), 
but that seems to be a bit over the top.  No change, no problem.


> 
>>> +           >= sizeof(name))
>>> +               return -ENAMETOOLONG;
>>> +       rx_queue->debug_dir = debugfs_create_dir(name,
>>> +                                                rx_queue->efx->debug_queues_dir);
>>> +       if (!rx_queue->debug_dir)
>>> +               return -ENOMEM;
>>> +
>>> +       /* Create files */
>>> +       efx_init_debugfs_rx_queue_files(rx_queue);
>>> +
>>> +       /* Create symlink to channel */
>>> +       if (snprintf(target, sizeof(target), "../../channels/%d",
>>> +                    channel->channel) >= sizeof(target))
>>> +               return -ENAMETOOLONG;
>>> +       if (!debugfs_create_symlink("channel", rx_queue->debug_dir, target))
>>> +               return -ENOMEM;
>>
>> If these fail, should you clean up the earlier create_dir()?
> 
> No; these errors mean "we didn't do everything we wanted to", not
>   "it's all broken", and the files/dir previously created are still
>   useful.  The caller treats errors as non-fatal.
> See also the kdoc comment on efx_init_debugfs_rx_queue:
>>> + * The directory must be cleaned up using efx_fini_debugfs_rx_queue(),
>>> + * even if this function returns an error.
> 
> I can't think of a suitable comment on these return statements to
>   clarify this, but suggestions are welcome.

Maybe something simple like
  "We don't clean up the files on errors here as they are still useful"

Cheers,
sln

Re: [PATCH net-next 6/7] sfc: add debugfs entries for filter table status

[Nelson, Shannon]

On 2/14/2025 8:23 AM, Edward Cree wrote:
> 
> On 15/12/2023 00:05, Nelson, Shannon wrote:
>> On 12/11/2023 9:18 AM, edward.cree@amd.com wrote:
>>> +#ifdef CONFIG_DEBUG_FS
>>> +       table->debug_dir = debugfs_create_dir("filters", efx->debug_dir);
>>> +       debugfs_create_bool("uc_promisc", 0444, table->debug_dir,
>>> +                           &table->uc_promisc);
>>> +       debugfs_create_bool("mc_promisc", 0444, table->debug_dir,
>>> +                           &table->mc_promisc);
>>> +       debugfs_create_bool("mc_promisc_last", 0444, table->debug_dir,
>>> +                           &table->mc_promisc_last);
>>> +       debugfs_create_bool("mc_overflow", 0444, table->debug_dir,
>>> +                           &table->mc_overflow);
>>> +       debugfs_create_bool("mc_chaining", 0444, table->debug_dir,
>>> +                           &table->mc_chaining);
>>> +#endif
>>
>> It would be good to continue the practice of using the debugfs_* primitives in your debugfs.c and just make a single call here that doesn't need the ifdef
> 
> I'm still in two minds about this.  While it makes sense in isolation
>   to do it that way here, in the following patch we add a more complex
>   dumper, and I think it makes more sense to put that in mcdi_filters.c
>   and have filters code know a bit about debugfs, than put it in
>   debugfs.c and have debug code know *everything* about filters — and
>   every other part of the driver that subsequently gets its own debug
>   nodes.
> I already have some follow-up patches that add EF100 MAE debugfs nodes
>   which also have custom dumpers, but those are in a separate file
>   (tc_debugfs.c) because there are a lot of them and tc/mae code is
>   already split into several pieces, whereas I'm not sure whether
>   adding a separate file for filter-table debugfs code really makes
>   sense, or whether it's sufficient just to refactor this code into
>   a(n unconditionally-called) function that continues to live in
>   mcdi_filters.c and has the ifdef within it.

Thanks, I can live with this.
sln