Tue, Jan 22, 2019 at 04:57:19PM CET, era...@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 <era...@mellanox.com>
>Reviewed-by: Moshe Shemesh <mo...@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
>
