Re: [net-next 07/15] net/mlx5: SD, Add informative prints in kernel log

[Jiri Pirko <jiri@resnulli.us>]

Thu, Jan 25, 2024 at 08:42:41AM CET, ttoukan.linux@gmail.com wrote:
>
>
>On 05/01/2024 14:12, Jiri Pirko wrote:
>> Thu, Dec 21, 2023 at 01:57:13AM CET, saeed@kernel.org wrote:
>> > From: Tariq Toukan <tariqt@nvidia.com>
>> > 
>> > Print to kernel log when an SD group moves from/to ready state.
>> > 
>> > Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
>> > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
>> > ---
>> > .../net/ethernet/mellanox/mlx5/core/lib/sd.c  | 21 +++++++++++++++++++
>> > 1 file changed, 21 insertions(+)
>> > 
>> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
>> > index 3309f21d892e..f68942277c62 100644
>> > --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
>> > @@ -373,6 +373,21 @@ static void sd_cmd_unset_secondary(struct mlx5_core_dev *secondary)
>> > 	mlx5_fs_cmd_set_l2table_entry_silent(secondary, 0);
>> > }
>> > 
>> > +static void sd_print_group(struct mlx5_core_dev *primary)
>> > +{
>> > +	struct mlx5_sd *sd = mlx5_get_sd(primary);
>> > +	struct mlx5_core_dev *pos;
>> > +	int i;
>> > +
>> > +	sd_info(primary, "group id %#x, primary %s, vhca %u\n",
>> > +		sd->group_id, pci_name(primary->pdev),
>> > +		MLX5_CAP_GEN(primary, vhca_id));
>> > +	mlx5_sd_for_each_secondary(i, primary, pos)
>> > +		sd_info(primary, "group id %#x, secondary#%d %s, vhca %u\n",
>> > +			sd->group_id, i - 1, pci_name(pos->pdev),
>> > +			MLX5_CAP_GEN(pos, vhca_id));
>> > +}
>> > +
>> > int mlx5_sd_init(struct mlx5_core_dev *dev)
>> > {
>> > 	struct mlx5_core_dev *primary, *pos, *to;
>> > @@ -410,6 +425,10 @@ int mlx5_sd_init(struct mlx5_core_dev *dev)
>> > 			goto err_unset_secondaries;
>> > 	}
>> > 
>> > +	sd_info(primary, "group id %#x, size %d, combined\n",
>> > +		sd->group_id, mlx5_devcom_comp_get_size(sd->devcom));
>> 
>> Can't you rather expose this over sysfs or debugfs? I mean, dmesg print
>> does not seem like a good idea.
>> 
>> 
>
>I think that the events of netdev combine/uncombine are important enough to
>be logged in the kernel dmesg.

Why? I believe that the best amount od dmesg log is exactly 0. You
should find proper interfaces. Definitelly for new features. Why do you
keep asking user to look for random messages in dmesg? Does not make any
sense :/



>I can implement a debugfs as an addition, not replacing the print.
>
>> > +	sd_print_group(primary);
>> > +
>> > 	return 0;
>> > 
>> > err_unset_secondaries:
>> > @@ -440,6 +459,8 @@ void mlx5_sd_cleanup(struct mlx5_core_dev *dev)
>> > 	mlx5_sd_for_each_secondary(i, primary, pos)
>> > 		sd_cmd_unset_secondary(pos);
>> > 	sd_cmd_unset_primary(primary);
>> > +
>> > +	sd_info(primary, "group id %#x, uncombined\n", sd->group_id);
>> > out:
>> > 	sd_unregister(dev);
>> > 	sd_cleanup(dev);
>> > -- 
>> > 2.43.0
>> > 
>> >