* [PATCH net-next 1/7] sfc: initial debugfs implementation
2023-12-11 17:18 [PATCH net-next 0/7] sfc: initial debugfs support edward.cree
@ 2023-12-11 17:18 ` edward.cree
2023-12-15 0:05 ` Nelson, Shannon
2023-12-11 17:18 ` [PATCH net-next 2/7] sfc: debugfs for channels edward.cree
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: edward.cree @ 2023-12-11 17:18 UTC (permalink / raw)
To: linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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;
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 1/7] sfc: initial debugfs implementation
2023-12-11 17:18 ` [PATCH net-next 1/7] sfc: initial debugfs implementation edward.cree
@ 2023-12-15 0:05 ` Nelson, Shannon
2024-02-08 21:25 ` Edward Cree
0 siblings, 1 reply; 27+ messages in thread
From: Nelson, Shannon @ 2023-12-15 0:05 UTC (permalink / raw)
To: edward.cree, linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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;
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 1/7] sfc: initial debugfs implementation
2023-12-15 0:05 ` Nelson, Shannon
@ 2024-02-08 21:25 ` Edward Cree
0 siblings, 0 replies; 27+ messages in thread
From: Edward Cree @ 2024-02-08 21:25 UTC (permalink / raw)
To: Nelson, Shannon, edward.cree, linux-net-drivers, davem, kuba,
pabeni, edumazet
Cc: netdev, habetsm.xilinx, Jonathan Cooper
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!
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 2/7] sfc: debugfs for channels
2023-12-11 17:18 [PATCH net-next 0/7] sfc: initial debugfs support edward.cree
2023-12-11 17:18 ` [PATCH net-next 1/7] sfc: initial debugfs implementation edward.cree
@ 2023-12-11 17:18 ` edward.cree
2023-12-12 19:54 ` kernel test robot
2023-12-11 17:18 ` [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues edward.cree
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: edward.cree @ 2023-12-11 17:18 UTC (permalink / raw)
To: linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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 */
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 2/7] sfc: debugfs for channels
2023-12-11 17:18 ` [PATCH net-next 2/7] sfc: debugfs for channels edward.cree
@ 2023-12-12 19:54 ` kernel test robot
0 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2023-12-12 19:54 UTC (permalink / raw)
To: edward.cree, linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: oe-kbuild-all, Edward Cree, netdev, habetsm.xilinx,
Jonathan Cooper
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues
2023-12-11 17:18 [PATCH net-next 0/7] sfc: initial debugfs support edward.cree
2023-12-11 17:18 ` [PATCH net-next 1/7] sfc: initial debugfs implementation edward.cree
2023-12-11 17:18 ` [PATCH net-next 2/7] sfc: debugfs for channels edward.cree
@ 2023-12-11 17:18 ` edward.cree
2023-12-12 22:06 ` kernel test robot
2023-12-15 0:05 ` Nelson, Shannon
2023-12-11 17:18 ` [PATCH net-next 4/7] sfc: debugfs for (nic) TX queues edward.cree
` (3 subsequent siblings)
6 siblings, 2 replies; 27+ messages in thread
From: edward.cree @ 2023-12-11 17:18 UTC (permalink / raw)
To: linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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;
}
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues
2023-12-11 17:18 ` [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues edward.cree
@ 2023-12-12 22:06 ` kernel test robot
2023-12-15 0:05 ` Nelson, Shannon
1 sibling, 0 replies; 27+ messages in thread
From: kernel test robot @ 2023-12-12 22:06 UTC (permalink / raw)
To: edward.cree, linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: oe-kbuild-all, Edward Cree, netdev, habetsm.xilinx,
Jonathan Cooper
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues
2023-12-11 17:18 ` [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues edward.cree
2023-12-12 22:06 ` kernel test robot
@ 2023-12-15 0:05 ` Nelson, Shannon
2025-02-14 15:51 ` Edward Cree
1 sibling, 1 reply; 27+ messages in thread
From: Nelson, Shannon @ 2023-12-15 0:05 UTC (permalink / raw)
To: edward.cree, linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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;
> }
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues
2023-12-15 0:05 ` Nelson, Shannon
@ 2025-02-14 15:51 ` Edward Cree
2025-02-14 17:39 ` Nelson, Shannon
0 siblings, 1 reply; 27+ messages in thread
From: Edward Cree @ 2025-02-14 15:51 UTC (permalink / raw)
To: Nelson, Shannon, edward.cree, linux-net-drivers, davem, kuba,
pabeni, edumazet
Cc: netdev, habetsm.xilinx, Jonathan Cooper
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.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues
2025-02-14 15:51 ` Edward Cree
@ 2025-02-14 17:39 ` Nelson, Shannon
0 siblings, 0 replies; 27+ messages in thread
From: Nelson, Shannon @ 2025-02-14 17:39 UTC (permalink / raw)
To: Edward Cree, edward.cree, linux-net-drivers, davem, kuba, pabeni,
edumazet
Cc: netdev, habetsm.xilinx, Jonathan Cooper
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 4/7] sfc: debugfs for (nic) TX queues
2023-12-11 17:18 [PATCH net-next 0/7] sfc: initial debugfs support edward.cree
` (2 preceding siblings ...)
2023-12-11 17:18 ` [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues edward.cree
@ 2023-12-11 17:18 ` edward.cree
2023-12-13 0:15 ` kernel test robot
2023-12-11 17:18 ` [PATCH net-next 5/7] sfc: add debugfs nodes for loopback mode edward.cree
` (2 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: edward.cree @ 2023-12-11 17:18 UTC (permalink / raw)
To: linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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,
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 4/7] sfc: debugfs for (nic) TX queues
2023-12-11 17:18 ` [PATCH net-next 4/7] sfc: debugfs for (nic) TX queues edward.cree
@ 2023-12-13 0:15 ` kernel test robot
0 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2023-12-13 0:15 UTC (permalink / raw)
To: edward.cree, linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: oe-kbuild-all, Edward Cree, netdev, habetsm.xilinx,
Jonathan Cooper
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 5/7] sfc: add debugfs nodes for loopback mode
2023-12-11 17:18 [PATCH net-next 0/7] sfc: initial debugfs support edward.cree
` (3 preceding siblings ...)
2023-12-11 17:18 ` [PATCH net-next 4/7] sfc: debugfs for (nic) TX queues edward.cree
@ 2023-12-11 17:18 ` edward.cree
2023-12-11 17:18 ` [PATCH net-next 6/7] sfc: add debugfs entries for filter table status edward.cree
2023-12-11 17:18 ` [PATCH net-next 7/7] sfc: add debugfs node for filter table contents edward.cree
6 siblings, 0 replies; 27+ messages in thread
From: edward.cree @ 2023-12-11 17:18 UTC (permalink / raw)
To: linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH net-next 6/7] sfc: add debugfs entries for filter table status
2023-12-11 17:18 [PATCH net-next 0/7] sfc: initial debugfs support edward.cree
` (4 preceding siblings ...)
2023-12-11 17:18 ` [PATCH net-next 5/7] sfc: add debugfs nodes for loopback mode edward.cree
@ 2023-12-11 17:18 ` edward.cree
2023-12-11 19:25 ` Simon Horman
2023-12-15 0:05 ` Nelson, Shannon
2023-12-11 17:18 ` [PATCH net-next 7/7] sfc: add debugfs node for filter table contents edward.cree
6 siblings, 2 replies; 27+ messages in thread
From: edward.cree @ 2023-12-11 17:18 UTC (permalink / raw)
To: linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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);
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 6/7] sfc: add debugfs entries for filter table status
2023-12-11 17:18 ` [PATCH net-next 6/7] sfc: add debugfs entries for filter table status edward.cree
@ 2023-12-11 19:25 ` Simon Horman
2023-12-15 0:05 ` Nelson, Shannon
1 sibling, 0 replies; 27+ messages in thread
From: Simon Horman @ 2023-12-11 19:25 UTC (permalink / raw)
To: edward.cree
Cc: linux-net-drivers, davem, kuba, pabeni, edumazet, Edward Cree,
netdev, habetsm.xilinx, Jonathan Cooper
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 6/7] sfc: add debugfs entries for filter table status
2023-12-11 17:18 ` [PATCH net-next 6/7] sfc: add debugfs entries for filter table status edward.cree
2023-12-11 19:25 ` Simon Horman
@ 2023-12-15 0:05 ` Nelson, Shannon
2025-02-14 16:23 ` Edward Cree
1 sibling, 1 reply; 27+ messages in thread
From: Nelson, Shannon @ 2023-12-15 0:05 UTC (permalink / raw)
To: edward.cree, linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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);
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 6/7] sfc: add debugfs entries for filter table status
2023-12-15 0:05 ` Nelson, Shannon
@ 2025-02-14 16:23 ` Edward Cree
2025-02-14 17:47 ` Nelson, Shannon
0 siblings, 1 reply; 27+ messages in thread
From: Edward Cree @ 2025-02-14 16:23 UTC (permalink / raw)
To: Nelson, Shannon, edward.cree, linux-net-drivers, davem, kuba,
pabeni, edumazet
Cc: netdev, habetsm.xilinx, Jonathan Cooper
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.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 6/7] sfc: add debugfs entries for filter table status
2025-02-14 16:23 ` Edward Cree
@ 2025-02-14 17:47 ` Nelson, Shannon
0 siblings, 0 replies; 27+ messages in thread
From: Nelson, Shannon @ 2025-02-14 17:47 UTC (permalink / raw)
To: Edward Cree, edward.cree, linux-net-drivers, davem, kuba, pabeni,
edumazet
Cc: netdev, habetsm.xilinx, Jonathan Cooper
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH net-next 7/7] sfc: add debugfs node for filter table contents
2023-12-11 17:18 [PATCH net-next 0/7] sfc: initial debugfs support edward.cree
` (5 preceding siblings ...)
2023-12-11 17:18 ` [PATCH net-next 6/7] sfc: add debugfs entries for filter table status edward.cree
@ 2023-12-11 17:18 ` edward.cree
2023-12-11 19:17 ` Simon Horman
2023-12-15 0:05 ` Nelson, Shannon
6 siblings, 2 replies; 27+ messages in thread
From: edward.cree @ 2023-12-11 17:18 UTC (permalink / raw)
To: linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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;
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents
2023-12-11 17:18 ` [PATCH net-next 7/7] sfc: add debugfs node for filter table contents edward.cree
@ 2023-12-11 19:17 ` Simon Horman
2023-12-12 13:58 ` Edward Cree
2023-12-15 0:05 ` Nelson, Shannon
1 sibling, 1 reply; 27+ messages in thread
From: Simon Horman @ 2023-12-11 19:17 UTC (permalink / raw)
To: edward.cree
Cc: linux-net-drivers, davem, kuba, pabeni, edumazet, Edward Cree,
netdev, habetsm.xilinx, Jonathan Cooper
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.
...
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents
2023-12-11 19:17 ` Simon Horman
@ 2023-12-12 13:58 ` Edward Cree
2023-12-12 15:14 ` Edward Cree
0 siblings, 1 reply; 27+ messages in thread
From: Edward Cree @ 2023-12-12 13:58 UTC (permalink / raw)
To: Simon Horman, edward.cree
Cc: linux-net-drivers, davem, kuba, pabeni, edumazet, netdev,
habetsm.xilinx, Jonathan Cooper
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents
2023-12-12 13:58 ` Edward Cree
@ 2023-12-12 15:14 ` Edward Cree
2023-12-12 16:19 ` Jakub Kicinski
0 siblings, 1 reply; 27+ messages in thread
From: Edward Cree @ 2023-12-12 15:14 UTC (permalink / raw)
To: Simon Horman, edward.cree
Cc: linux-net-drivers, davem, kuba, pabeni, edumazet, netdev,
habetsm.xilinx, Jonathan Cooper
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents
2023-12-12 15:14 ` Edward Cree
@ 2023-12-12 16:19 ` Jakub Kicinski
2023-12-13 12:15 ` Edward Cree
0 siblings, 1 reply; 27+ messages in thread
From: Jakub Kicinski @ 2023-12-12 16:19 UTC (permalink / raw)
To: Edward Cree
Cc: Simon Horman, edward.cree, linux-net-drivers, davem, pabeni,
edumazet, netdev, habetsm.xilinx, Jonathan Cooper
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) {}
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents
2023-12-12 16:19 ` Jakub Kicinski
@ 2023-12-13 12:15 ` Edward Cree
2023-12-14 16:56 ` Simon Horman
0 siblings, 1 reply; 27+ messages in thread
From: Edward Cree @ 2023-12-13 12:15 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Simon Horman, edward.cree, linux-net-drivers, davem, pabeni,
edumazet, netdev, habetsm.xilinx, Jonathan Cooper
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents
2023-12-13 12:15 ` Edward Cree
@ 2023-12-14 16:56 ` Simon Horman
0 siblings, 0 replies; 27+ messages in thread
From: Simon Horman @ 2023-12-14 16:56 UTC (permalink / raw)
To: Edward Cree
Cc: Jakub Kicinski, edward.cree, linux-net-drivers, davem, pabeni,
edumazet, netdev, habetsm.xilinx, Jonathan Cooper
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH net-next 7/7] sfc: add debugfs node for filter table contents
2023-12-11 17:18 ` [PATCH net-next 7/7] sfc: add debugfs node for filter table contents edward.cree
2023-12-11 19:17 ` Simon Horman
@ 2023-12-15 0:05 ` Nelson, Shannon
1 sibling, 0 replies; 27+ messages in thread
From: Nelson, Shannon @ 2023-12-15 0:05 UTC (permalink / raw)
To: edward.cree, linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx, Jonathan Cooper
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;
>
^ permalink raw reply [flat|nested] 27+ messages in thread