diff mbox series

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

Message ID fc28d967fbffd53f61a1d42332ee7bc64435df7c.1702314695.git.ecree.xilinx@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series sfc: initial debugfs support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 1115 this patch: 21
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang fail Errors and warnings before: 1142 this patch: 23
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 1142 this patch: 21
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 50 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

edward.cree@amd.com Dec. 11, 2023, 5:18 p.m. UTC 
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(+)

Comments

Simon Horman Dec. 11, 2023, 7:25 p.m. UTC | #1 
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.

...
Nelson, Shannon Dec. 15, 2023, 12:05 a.m. UTC | #2 
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);
>
Edward Cree Feb. 14, 2025, 4:23 p.m. UTC | #3 
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.
Nelson, Shannon Feb. 14, 2025, 5:47 p.m. UTC | #4 
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
diff mbox series

Patch

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);