Re: [PATCH net-next 2/7] net/mlx5e: Move driver to use devlink msg API

[Jiri Pirko <jiri@resnulli.us>]

Tue, Jan 22, 2019 at 04:57:19PM CET, eranbe@mellanox.com wrote:
>As part of the devlink health reporter diagnose ops callback, the mlx5e TX
>reporter used devlink health buffers API. Which will soon be depracated.
>Modify the reporter to use the new devlink msg API.
>
>The actual set of the new diagnose function will be done later in the
>downstream patch, only when devlink will actually expose a new diagnose
>operation that uses the new API.
>
>Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>---
> .../mellanox/mlx5/core/en/reporter_tx.c       | 123 ++++--------------
> 1 file changed, 26 insertions(+), 97 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>index d9675afbb924..fc92850c214a 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>@@ -192,110 +192,47 @@ static int mlx5e_tx_reporter_recover(struct devlink_health_reporter *reporter,
> }
> 
> static int
>-mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
>+mlx5e_tx_reporter_build_diagnose_output(struct devlink_msg_ctx *msg_ctx,
> 					u32 sqn, u8 state, u8 stopped)

What is "state" and "stopped"? Is "stopped" a bool? Is "state" an enum.


> {
>-	int err, i;
>-	int nest = 0;
>-	char name[20];
>-
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>-	if (err)
>-		goto buffer_error;
>-	nest++;
>-
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>-	if (err)
>-		goto buffer_error;
>-	nest++;
>-
>-	sprintf(name, "SQ 0x%x", sqn);
>-	err = devlink_health_buffer_put_object_name(buffer, name);
>-	if (err)
>-		goto buffer_error;
>-
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>-	if (err)
>-		goto buffer_error;
>-	nest++;
>-
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>-	if (err)
>-		goto buffer_error;
>-	nest++;
>-
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>-	if (err)
>-		goto buffer_error;
>-	nest++;
>-
>-	err = devlink_health_buffer_put_object_name(buffer, "HW state");
>-	if (err)
>-		goto buffer_error;
>+	int err;
> 
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>+	err = devlink_msg_object_start(msg_ctx, "SQ");
> 	if (err)
>-		goto buffer_error;
>-	nest++;
>+		return err;
> 
>-	err = devlink_health_buffer_put_value_u8(buffer, state);
>+	err = devlink_msg_object_start(msg_ctx, NULL);
> 	if (err)
>-		goto buffer_error;
>-
>-	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE */
>-	nest--;
>-
>-	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR */
>-	nest--;
>+		return err;
> 
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>+	err = devlink_msg_put_name_value_pair(msg_ctx, "sqn", &sqn,
>+					      sizeof(sqn), NLA_U32);
> 	if (err)
>-		goto buffer_error;
>-	nest++;
>+		return err;
> 
>-	err = devlink_health_buffer_put_object_name(buffer, "stopped");
>+	err = devlink_msg_put_name_value_pair(msg_ctx, "HW state", &state,
>+					      sizeof(state), NLA_U8);
> 	if (err)
>-		goto buffer_error;
>+		return err;
> 
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>+	err = devlink_msg_put_name_value_pair(msg_ctx, "stopped", &stopped,
>+					      sizeof(stopped), NLA_U8);
> 	if (err)
>-		goto buffer_error;
>-	nest++;
>+		return err;
> 
>-	err = devlink_health_buffer_put_value_u8(buffer, stopped);
>+	err = devlink_msg_object_end(msg_ctx, NULL);
> 	if (err)
>-		goto buffer_error;
>-
>-	for (i = 0; i < nest; i++)
>-		devlink_health_buffer_nest_end(buffer);
>-
>-	return 0;
>+		return err;
> 
>-buffer_error:
>-	for (i = 0; i < nest; i++)
>-		devlink_health_buffer_nest_cancel(buffer);
>+	err = devlink_msg_object_end(msg_ctx, "SQ");
> 	return err;
> }
> 
> static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>-				      struct devlink_health_buffer **buffers_array,
>-				      unsigned int buffer_size,
>-				      unsigned int num_buffers)
>+				      struct devlink_msg_ctx *msg_ctx)
> {
> 	struct mlx5e_priv *priv = devlink_health_reporter_priv(reporter);
>-	unsigned int buff = 0;
>-	int i = 0, err = 0;
>-
>-	if (buffer_size < MLX5E_TX_REPORTER_PER_SQ_MAX_LEN)
>-		return -ENOMEM;
>+	int i, err = 0;
> 
> 	mutex_lock(&priv->state_lock);
> 
>@@ -304,7 +241,8 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> 		return 0;
> 	}
> 
>-	while (i < priv->channels.num * priv->channels.params.num_tc) {
>+	for (i = 0; i < priv->channels.num * priv->channels.params.num_tc;
>+	     i++) {
> 		struct mlx5e_txqsq *sq = priv->txq2sq[i];
> 		u8 state;
> 
>@@ -312,15 +250,11 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> 		if (err)
> 			break;
> 
>-		err = mlx5e_tx_reporter_build_diagnose_output(buffers_array[buff],
>-							      sq->sqn, state,
>+		err = mlx5e_tx_reporter_build_diagnose_output(msg_ctx, sq->sqn,
>+							      state,
> 							      netif_xmit_stopped(sq->txq));

This should be an array. On SQ entry : one member of array.

So if you want to change it, you need to do this in 2 patches. One API,
one the actual layout. :/



>-		if (err) {
>-			if (++buff == num_buffers)
>-				break;
>-		} else {
>-			i++;
>-		}
>+		if (err)
>+			break;
> 	}
> 
> 	mutex_unlock(&priv->state_lock);
>@@ -330,11 +264,6 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
> 		.name = "TX",
> 		.recover = mlx5e_tx_reporter_recover,
>-		.diagnose_size = MLX5E_MAX_NUM_CHANNELS * MLX5E_MAX_NUM_TC *
>-				 MLX5E_TX_REPORTER_PER_SQ_MAX_LEN,
>-		.diagnose = mlx5e_tx_reporter_diagnose,

So you change the callback, remove it so the dissection is broken.



>-		.dump_size = 0,
>-		.dump = NULL,


This has 0 relation to the patch. Should be separate.


> };
> 
> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>-- 
>2.17.1
>