| Message ID | 20250103060610.2233908-6-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add more feautues for ENETC v4 - round 2 | expand |
On Fri, Jan 03, 2025 at 02:06:01PM +0800, Wei Fang wrote: > ENETC's MAC filter consists of hash MAC filter and exact MAC filter. Hash > MAC filter is a 64-entry hash table consisting of two 32-bit registers. > Exact MAC filter is implemented by configuring MAC address filter table > through command BD ring. The table is stored in ENETC's internal memory > and needs to be read through command BD ring. In order to facilitate > debugging, added a debugfs interface to get the relevant information > about MAC filter. How do other drivers do this? You should only use debugfs if there is no standard way to accomplish something. And if there is no standard way, you should be thinking is this a common feature other drivers will need, and if so, add a standard mechanism. You will get pushback for using debugfs as a bumping ground without adding some justification that debugfs is the only possible solution. Andrew
> Subject: Re: [PATCH net-next 05/13] net: enetc: add debugfs interface to dump > MAC filter > > On Fri, Jan 03, 2025 at 02:06:01PM +0800, Wei Fang wrote: > > ENETC's MAC filter consists of hash MAC filter and exact MAC filter. Hash > > MAC filter is a 64-entry hash table consisting of two 32-bit registers. > > Exact MAC filter is implemented by configuring MAC address filter table > > through command BD ring. The table is stored in ENETC's internal memory > > and needs to be read through command BD ring. In order to facilitate > > debugging, added a debugfs interface to get the relevant information > > about MAC filter. > > How do other drivers do this? I don't know about other vendor's hardware, but IMO, if the configuration is done through registers, we only need to debug through some tools that read and write registers, such as devmem2. I also saw some drivers added debugfs interface, such as Intel, Huawei, Marvell, etc. I think they also added it to facilitate obtaining some debugging information. > > You should only use debugfs if there is no standard way to accomplish > something. And if there is no standard way, you should be thinking is > this a common feature other drivers will need, and if so, add a > standard mechanism. > > You will get pushback for using debugfs as a bumping ground without > adding some justification that debugfs is the only possible solution. > IMO, standard methods are only suitable for extracting common information, but each vendor's implementation is not uniform, and the specific details are also different. It is impossible to obtain every detail information through standard methods. The purpose of adding debugfs is just to facilitate debugging, so we need to obtain all the detailed information, so I think the debugfs interface is good, which allows us to obtain every specific information in all NETC tables.
diff --git a/drivers/net/ethernet/freescale/enetc/Makefile b/drivers/net/ethernet/freescale/enetc/Makefile index 707a68e26971..f1c5ad45fd76 100644 --- a/drivers/net/ethernet/freescale/enetc/Makefile +++ b/drivers/net/ethernet/freescale/enetc/Makefile @@ -16,6 +16,7 @@ fsl-enetc-$(CONFIG_FSL_ENETC_QOS) += enetc_qos.o obj-$(CONFIG_NXP_ENETC4) += nxp-enetc4.o nxp-enetc4-y := enetc4_pf.o +nxp-enetc4-$(CONFIG_DEBUG_FS) += enetc4_debugfs.o obj-$(CONFIG_FSL_ENETC_VF) += fsl-enetc-vf.o fsl-enetc-vf-y := enetc_vf.o diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h index 4dba91408e3d..ca1bc85c0ac9 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.h +++ b/drivers/net/ethernet/freescale/enetc/enetc.h @@ -318,6 +318,7 @@ struct enetc_si { struct enetc_mac_filter mac_filter[MADDR_TYPE]; struct workqueue_struct *workqueue; struct work_struct rx_mode_task; + struct dentry *debugfs_root; }; #define ENETC_SI_ALIGN 32 diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_debugfs.c b/drivers/net/ethernet/freescale/enetc/enetc4_debugfs.c new file mode 100644 index 000000000000..3a660c80344a --- /dev/null +++ b/drivers/net/ethernet/freescale/enetc/enetc4_debugfs.c @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Copyright 2025 NXP */ + +#include <linux/device.h> +#include <linux/debugfs.h> +#include <linux/seq_file.h> + +#include "enetc_pf.h" +#include "enetc4_debugfs.h" + +#define is_en(x) (x) ? "Enabled" : "Disabled" + +static void enetc_show_si_mac_hash_filter(struct seq_file *s, int i) +{ + struct enetc_si *si = s->private; + struct enetc_hw *hw = &si->hw; + u32 hash_h, hash_l; + + hash_l = enetc_port_rd(hw, ENETC4_PSIUMHFR0(i)); + hash_h = enetc_port_rd(hw, ENETC4_PSIUMHFR1(i)); + seq_printf(s, "SI %d unicast MAC hash filter: 0x%08x%08x\n", + i, hash_h, hash_l); + + hash_l = enetc_port_rd(hw, ENETC4_PSIMMHFR0(i)); + hash_h = enetc_port_rd(hw, ENETC4_PSIMMHFR1(i)); + seq_printf(s, "SI %d multicast MAC hash filter: 0x%08x%08x\n", + i, hash_h, hash_l); +} + +static int enetc_mac_filter_show(struct seq_file *s, void *data) +{ + struct maft_entry_data maft_data; + struct enetc_si *si = s->private; + struct enetc_hw *hw = &si->hw; + struct maft_keye_data *keye; + struct enetc_pf *pf; + int i, err, num_si; + u32 val; + + pf = enetc_si_priv(si); + num_si = pf->caps.num_vsi + 1; + + val = enetc_port_rd(hw, ENETC4_PSIPMMR); + for (i = 0; i < num_si; i++) { + seq_printf(s, "SI %d Unicast Promiscuous mode: %s\n", + i, is_en(PSIPMMR_SI_MAC_UP(i) & val)); + seq_printf(s, "SI %d Multicast Promiscuous mode: %s\n", + i, is_en(PSIPMMR_SI_MAC_MP(i) & val)); + } + + /* MAC hash filter table */ + for (i = 0; i < num_si; i++) + enetc_show_si_mac_hash_filter(s, i); + + if (!pf->num_mfe) + return 0; + + /* MAC address filter table */ + seq_puts(s, "Show MAC address filter table\n"); + for (i = 0; i < pf->num_mfe; i++) { + memset(&maft_data, 0, sizeof(maft_data)); + err = ntmp_maft_query_entry(&si->ntmp.cbdrs, i, &maft_data); + if (err) + return err; + + keye = &maft_data.keye; + seq_printf(s, "Entry %d, MAC: %pM, SI bitmap: 0x%04x\n", i, + keye->mac_addr, le16_to_cpu(maft_data.cfge.si_bitmap)); + } + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(enetc_mac_filter); + +void enetc_create_debugfs(struct enetc_si *si) +{ + struct net_device *ndev = si->ndev; + struct dentry *root; + + root = debugfs_create_dir(netdev_name(ndev), NULL); + if (IS_ERR(root)) + return; + + si->debugfs_root = root; + + debugfs_create_file("mac_filter", 0444, root, si, &enetc_mac_filter_fops); +} + +void enetc_remove_debugfs(struct enetc_si *si) +{ + debugfs_remove_recursive(si->debugfs_root); + si->debugfs_root = NULL; +} diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_debugfs.h b/drivers/net/ethernet/freescale/enetc/enetc4_debugfs.h new file mode 100644 index 000000000000..96caca35f79d --- /dev/null +++ b/drivers/net/ethernet/freescale/enetc/enetc4_debugfs.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */ +/* Copyright 2025 NXP */ + +#ifndef __ENETC4_DEBUGFS_H +#define __ENETC4_DEBUGFS_H + +#if IS_ENABLED(CONFIG_DEBUG_FS) +void enetc_create_debugfs(struct enetc_si *si); +void enetc_remove_debugfs(struct enetc_si *si); +#else +static inline void enetc_create_debugfs(struct enetc_si *si) +{ +} + +static inline void enetc_remove_debugfs(struct enetc_si *si) +{ +} +#endif + +#endif diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c index 6ec849949267..6de571b5b425 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c @@ -8,6 +8,7 @@ #include <linux/unaligned.h> #include "enetc_pf_common.h" +#include "enetc4_debugfs.h" #define ENETC_SI_MAX_RING_NUM 8 @@ -1169,6 +1170,8 @@ static int enetc4_pf_probe(struct pci_dev *pdev, if (err) goto err_netdev_create; + enetc_create_debugfs(si); + return 0; err_netdev_create: @@ -1186,6 +1189,7 @@ static void enetc4_pf_remove(struct pci_dev *pdev) struct enetc_si *si = pci_get_drvdata(pdev); struct enetc_pf *pf = enetc_si_priv(si); + enetc_remove_debugfs(si); enetc4_pf_netdev_destroy(si); enetc4_pf_free(pf); destroy_workqueue(si->workqueue);
ENETC's MAC filter consists of hash MAC filter and exact MAC filter. Hash MAC filter is a 64-entry hash table consisting of two 32-bit registers. Exact MAC filter is implemented by configuring MAC address filter table through command BD ring. The table is stored in ENETC's internal memory and needs to be read through command BD ring. In order to facilitate debugging, added a debugfs interface to get the relevant information about MAC filter. Signed-off-by: Wei Fang <wei.fang@nxp.com> --- drivers/net/ethernet/freescale/enetc/Makefile | 1 + drivers/net/ethernet/freescale/enetc/enetc.h | 1 + .../ethernet/freescale/enetc/enetc4_debugfs.c | 93 +++++++++++++++++++ .../ethernet/freescale/enetc/enetc4_debugfs.h | 20 ++++ .../net/ethernet/freescale/enetc/enetc4_pf.c | 4 + 5 files changed, 119 insertions(+) create mode 100644 drivers/net/ethernet/freescale/enetc/enetc4_debugfs.c create mode 100644 drivers/net/ethernet/freescale/enetc/enetc4_debugfs.h