| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190124090806.GA2357@nanopsycho>
Date: Thu, 24 Jan 2019 10:08:06 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Eran Ben Elisha <eranbe@...lanox.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Jiri Pirko <jiri@...lanox.com>,
"David S. Miller" <davem@...emloft.net>,
Saeed Mahameed <saeedm@...lanox.com>,
Moshe Shemesh <moshe@...lanox.com>
Subject: Re: [PATCH net-next 2/7] net/mlx5e: Move driver to use devlink msg
API
Thu, Jan 24, 2019 at 08:39:12AM CET, eranbe@...lanox.com wrote:
>
>
>On 1/23/2019 4:39 PM, Jiri Pirko wrote:
>> Tue, Jan 22, 2019 at 04:57:19PM CET, eranbe@...lanox.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@...lanox.com>
>>> Reviewed-by: Moshe Shemesh <moshe@...lanox.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
>>>
Powered by blists - more mailing lists