Re: [PATCH net-next RFC v1 1/4] devlink: Wrap trap related lists and ops in trap_mngr

[Jiri Pirko]

Wed, Sep 02, 2020 at 05:32:11PM CEST, ayal@xxxxxxxxxxxx wrote:
>Bundle the trap related lists: trap_list, trap_group_list and
>trap_policer_list and trap ops like: trap_init, trap_fini,
>trap_action_set... together in trap_mngr. This will be handy in the
>coming patches in the set introducing traps in devlink port context.
>With trap_mngr, code reuse is much simpler.
>
>Signed-off-by: Aya Levin <ayal@xxxxxxxxxxxx>
>---
> drivers/net/ethernet/mellanox/mlxsw/core.c |   4 +
> include/net/devlink.h                      |  59 ++++---
> net/core/devlink.c                         | 255 +++++++++++++++++------------

You need to split this. You do at least 2 separate things in one patch.
Please check it.


> 3 files changed, 188 insertions(+), 130 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
>index 08d101138fbe..97460f47e537 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
>@@ -1285,6 +1285,9 @@ static const struct devlink_ops mlxsw_devlink_ops = {
> 	.sb_occ_tc_port_bind_get	= mlxsw_devlink_sb_occ_tc_port_bind_get,
> 	.info_get			= mlxsw_devlink_info_get,
> 	.flash_update			= mlxsw_devlink_flash_update,
>+};
>+
>+static const struct devlink_trap_ops mlxsw_devlink_traps_ops = {
> 	.trap_init			= mlxsw_devlink_trap_init,
> 	.trap_fini			= mlxsw_devlink_trap_fini,
> 	.trap_action_set		= mlxsw_devlink_trap_action_set,
>@@ -1321,6 +1324,7 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
> 			err = -ENOMEM;
> 			goto err_devlink_alloc;
> 		}
>+		devlink_traps_ops(devlink, &mlxsw_devlink_traps_ops);
> 	}
> 
> 	mlxsw_core = devlink_priv(devlink);
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 8f3c8a443238..d387ea5518c3 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -21,6 +21,13 @@
> #include <linux/xarray.h>
> 
> struct devlink_ops;
>+struct devlink_trap_ops;
>+struct devlink_trap_mngr {

"Mngr" is odd. This is not a manager. Just a common place to store
trap-related things. Perhaps just "devlink_traps"?


>+	struct list_head trap_list;
>+	struct list_head trap_group_list;
>+	struct list_head trap_policer_list;
>+	const struct devlink_trap_ops *trap_ops;
>+};

[...]