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

[Eran Ben Elisha <eranbe@mellanox.com>]

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