diff mbox series

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

Message ID 1548172644-30862-3-git-send-email-eranbe@mellanox.com
State Changes Requested
Delegated to: David Miller
Headers show
Series Devlink health updates | expand

Commit Message

Eran Ben Elisha Jan. 22, 2019, 3:57 p.m. UTC 
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(-)

Comments

Jiri Pirko Jan. 23, 2019, 2:39 p.m. UTC | #1 
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
>
Eran Ben Elisha Jan. 24, 2019, 7:39 a.m. UTC | #2 
On 1/23/2019 4:39 PM, Jiri Pirko wrote:
> 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.
> 
stopped is the reply from netif_xmit_stopped, and it is a bool.
state is the HW state of the SQ.
it is defined in the ifc file:
enum {
         MLX5_SQC_STATE_RST  = 0x0,
         MLX5_SQC_STATE_RDY  = 0x1,
         MLX5_SQC_STATE_ERR  = 0x3,
};
I could have translated it into strings, but I though it would be fine 
to leave it as is.

> 
>> {
>> -	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.

This is needed in order to have this patch compiled.

> 
> 
> 
>> -		.dump_size = 0,
>> -		.dump = NULL,
> 
> 
> This has 0 relation to the patch. Should be separate.

ack.

> 
> 
>> };
>>
>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>> -- 
>> 2.17.1
>>
Jiri Pirko Jan. 24, 2019, 9:08 a.m. UTC | #3 
Thu, Jan 24, 2019 at 08:39:12AM CET, eranbe@mellanox.com wrote:
>
>
>On 1/23/2019 4:39 PM, Jiri Pirko wrote:
>> 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.
>> 
>stopped is the reply from netif_xmit_stopped, and it is a bool.

If it is bool, you should add it into message as NLA_FLAG, not NLA_U8


>state is the HW state of the SQ.
>it is defined in the ifc file:
>enum {
>         MLX5_SQC_STATE_RST  = 0x0,
>         MLX5_SQC_STATE_RDY  = 0x1,
>         MLX5_SQC_STATE_ERR  = 0x3,
>};
>I could have translated it into strings, but I though it would be fine 
>to leave it as is.

It's fine as u8

>
>> 
>>> {
>>> -	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.
>
>This is needed in order to have this patch compiled.

You have to figure it out without breakage. This is not the way to
do this.

>
>> 
>> 
>> 
>>> -		.dump_size = 0,
>>> -		.dump = NULL,
>> 
>> 
>> This has 0 relation to the patch. Should be separate.
>
>ack.
>
>> 
>> 
>>> };
>>>
>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>> -- 
>>> 2.17.1
>>>
Eran Ben Elisha Jan. 24, 2019, 9:57 a.m. UTC | #4 
On 1/24/2019 11:08 AM, Jiri Pirko wrote:
> Thu, Jan 24, 2019 at 08:39:12AM CET, eranbe@mellanox.com wrote:
>>
>>
>> On 1/23/2019 4:39 PM, Jiri Pirko wrote:
>>> 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.
>>>
>> stopped is the reply from netif_xmit_stopped, and it is a bool.
> 
> If it is bool, you should add it into message as NLA_FLAG, not NLA_U8
> 
> 
>> state is the HW state of the SQ.
>> it is defined in the ifc file:
>> enum {
>>          MLX5_SQC_STATE_RST  = 0x0,
>>          MLX5_SQC_STATE_RDY  = 0x1,
>>          MLX5_SQC_STATE_ERR  = 0x3,
>> };
>> I could have translated it into strings, but I though it would be fine
>> to leave it as is.
> 
> It's fine as u8
> 
>>
>>>
>>>> {
>>>> -	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.
>>
>> This is needed in order to have this patch compiled.
> 
> You have to figure it out without breakage. This is not the way to
> do this.

This would cause all devlink and mlx5e patches to be altogether when 
moving to the new API, which I don't like at all.

(#2) I think we shall take your approach and do a revert and put it all 
again on top.

> 
>>
>>>
>>>
>>>
>>>> -		.dump_size = 0,
>>>> -		.dump = NULL,
>>>
>>>
>>> This has 0 relation to the patch. Should be separate.
>>
>> ack.
>>
>>>
>>>
>>>> };
>>>>
>>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>>> -- 
>>>> 2.17.1
>>>>
diff mbox series

Patch

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)
 {
-	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));
-		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,
-		.dump_size = 0,
-		.dump = NULL,
 };
 
 #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500