On 1/24/2019 11:08 AM, Jiri Pirko wrote:
> Thu, Jan 24, 2019 at 08:39:12AM CET, era...@mellanox.com wrote:
>>
>>
>> On 1/23/2019 4:39 PM, Jiri Pirko wrote:
>>> 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.
>>>
>> 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
>>>>
