[net-next,V3,02/11] net/mlx5_core: Add EQ renaming mechanism

From: Saeed Mahameed <saeedm@mellanox.com>

Introduce mlx5_rename_eq() to rename an EQ. This will be used by a
following commit for the Ethernet functionality of the driver.

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/main.c     | 25 ++++++++++++++++++++--
 .../net/ethernet/mellanox/mlx5/core/mlx5_core.h    |  2 ++
 2 files changed, 25 insertions(+), 2 deletions(-)

From: Amir Vadai <amirv@mellanox.com>
Date: Sun, 10 May 2015 12:38:57 +0300

> +int mlx5_rename_eq(struct mlx5_core_dev *dev, int eq_ix, char *name)
> +{
> +	struct mlx5_priv *priv = &dev->priv;
> +	struct mlx5_eq_table *table = &priv->eq_table;
> +	struct mlx5_eq *eq;
> +	int err = -ENOENT;
> +
> +	spin_lock(&table->lock);
> +	list_for_each_entry(eq, &table->comp_eqs_list, list) {
> +		if (eq->index == eq_ix) {
> +			snprintf(priv->irq_info[eq_ix].name, MLX5_MAX_IRQ_NAME,
> +				 "%s-%d", name, eq_ix);
> +			err = 0;
> +			break;
> +		}
> +	}
> +	spin_unlock(&table->lock);
> +
> +	return err;
> +}

You have to be very careful with this.

If you change these names after the request_irq() call(s), the new name string
will not propagate to /proc/interrupts output etc.

Looking at your later patches, this seems to be invoked very late in
mlx5e_open_locked(), so I am concerned.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, May 11, 2015 at 8:40 PM, David Miller <davem@davemloft.net> wrote:
> From: Amir Vadai <amirv@mellanox.com>
> Date: Sun, 10 May 2015 12:38:57 +0300
>
>> +int mlx5_rename_eq(struct mlx5_core_dev *dev, int eq_ix, char *name)
>> +{
>> +     struct mlx5_priv *priv = &dev->priv;
>> +     struct mlx5_eq_table *table = &priv->eq_table;
>> +     struct mlx5_eq *eq;
>> +     int err = -ENOENT;
>> +
>> +     spin_lock(&table->lock);
>> +     list_for_each_entry(eq, &table->comp_eqs_list, list) {
>> +             if (eq->index == eq_ix) {
>> +                     snprintf(priv->irq_info[eq_ix].name, MLX5_MAX_IRQ_NAME,
>> +                              "%s-%d", name, eq_ix);
>> +                     err = 0;
>> +                     break;
>> +             }
>> +     }
>> +     spin_unlock(&table->lock);
>> +
>> +     return err;
>> +}
>
> You have to be very careful with this.
>
> If you change these names after the request_irq() call(s), the new name string
> will not propagate to /proc/interrupts output etc.
>
> Looking at your later patches, this seems to be invoked very late in
> mlx5e_open_locked(), so I am concerned.
It is true. We call request_irq() when driver is loaded (and we don't
know yet if the ports types are Infiniband or Ethernet). Only later
on, we rename the name when interface is up and we know its name.

But, empirically /proc/interrupts is propagated with the new name
(according to latest net-next). Also, in the code, I don't see why
shouldn't it be updated. When calling request_irq(), name is not
copied, but irq_desc->action->name is pointing to it. This same
pointer is being used by show_interrupts() when /proc/interrupts is
queried.
So don't see why when modifying the string pointed by it, it wouldn't
shown on /proc/interrupts.

What did I miss?

Thanks,
Amir
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, May 12, 2015 at 2:55 PM, Amir Vadai <amirv@mellanox.com> wrote:
> On Mon, May 11, 2015 at 8:40 PM, David Miller <davem@davemloft.net> wrote:
>> From: Amir Vadai <amirv@mellanox.com>
>> Date: Sun, 10 May 2015 12:38:57 +0300
>>
>>> +int mlx5_rename_eq(struct mlx5_core_dev *dev, int eq_ix, char *name)
>>> +{
>>> +     struct mlx5_priv *priv = &dev->priv;
>>> +     struct mlx5_eq_table *table = &priv->eq_table;
>>> +     struct mlx5_eq *eq;
>>> +     int err = -ENOENT;
>>> +
>>> +     spin_lock(&table->lock);
>>> +     list_for_each_entry(eq, &table->comp_eqs_list, list) {
>>> +             if (eq->index == eq_ix) {
>>> +                     snprintf(priv->irq_info[eq_ix].name, MLX5_MAX_IRQ_NAME,
>>> +                              "%s-%d", name, eq_ix);
>>> +                     err = 0;
>>> +                     break;
>>> +             }
>>> +     }
>>> +     spin_unlock(&table->lock);
>>> +
>>> +     return err;
>>> +}
>>
>> You have to be very careful with this.
>>
>> If you change these names after the request_irq() call(s), the new name string
>> will not propagate to /proc/interrupts output etc.
>>
>> Looking at your later patches, this seems to be invoked very late in
>> mlx5e_open_locked(), so I am concerned.
> It is true. We call request_irq() when driver is loaded (and we don't
> know yet if the ports types are Infiniband or Ethernet). Only later
> on, we rename the name when interface is up and we know its name.
>
> But, empirically /proc/interrupts is propagated with the new name
> (according to latest net-next). Also, in the code, I don't see why
> shouldn't it be updated. When calling request_irq(), name is not
> copied, but irq_desc->action->name is pointing to it. This same
> pointer is being used by show_interrupts() when /proc/interrupts is
> queried.
> So don't see why when modifying the string pointed by it, it wouldn't
> shown on /proc/interrupts.

Dave, does this makes sense to you?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html