Re: [PATCH net-next 02/12] net/mlx5: LAG, Refactor lag logic

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Mark Bloch <mbloch@nvidia.com>
Date: Tue, 17 Dec 2024 14:52:55 +0200

> 
> 
> On 17/12/2024 13:32, Alexander Lobakin wrote:
>> From: Rongwei Liu <rongweil@nvidia.com>
>> Date: Tue, 17 Dec 2024 13:44:07 +0800
>>
>>>
>>>
>>> On 2024/12/17 01:55, Alexander Lobakin wrote:
>>>> From: Tariq Toukan <tariqt@nvidia.com>
>>>> Date: Wed, 11 Dec 2024 15:42:13 +0200
>>>>
>>>>> From: Rongwei Liu <rongweil@nvidia.com>
>>>>>
>>>>> Wrap the lag pf access into two new macros:
>>>>> 1. ldev_for_each()
>>>>> 2. ldev_for_each_reverse()
>>>>> The maximum number of lag ports and the index to `natvie_port_num`
>>>>> mapping will be handled by the two new macros.
>>>>> Users shouldn't use the for loop anymore.
>>>>
>>>> [...]
>>>>
>>>>> @@ -1417,6 +1398,26 @@ void mlx5_lag_add_netdev(struct mlx5_core_dev *dev,
>>>>>  	mlx5_queue_bond_work(ldev, 0);
>>>>>  }
>>>>>  
>>>>> +int get_pre_ldev_func(struct mlx5_lag *ldev, int start_idx, int end_idx)
>>>>> +{
>>>>> +	int i;
>>>>> +
>>>>> +	for (i = start_idx; i >= end_idx; i--)
>>>>> +		if (ldev->pf[i].dev)
>>>>> +			return i;
>>>>> +	return -1;
>>>>> +}
>>>>> +
>>>>> +int get_next_ldev_func(struct mlx5_lag *ldev, int start_idx)
>>>>> +{
>>>>> +	int i;
>>>>> +
>>>>> +	for (i = start_idx; i < MLX5_MAX_PORTS; i++)
>>>>> +		if (ldev->pf[i].dev)
>>>>> +			return i;
>>>>> +	return MLX5_MAX_PORTS;
>>>>> +}
>>>>
>>>> Why aren't these two prefixed with mlx5?
>>>> We can have. No mlx5 prefix aligns with "ldev_for_each/ldev_for_each_reverse()", simple, short and meaningful.
>>
>> All drivers must have its symbols prefixed, otherwise there might be
>> name conflicts at anytime and also it's not clear where a definition
>> comes from if it's not prefixed.
>>
> 
> However, those aren't exported symbols, they are used exclusively by the mlx5 lag code.
> I don't see any added value in prefixing internal functions with mlx5 unless it adds
> context to the logic.
> Here it's very clear we are going over the members that are stored inside the ldev struct.

Tomorrow someone will add a function with the same name to the core
kernel code and get a compilation failure. OTOH nobody would add a
generic symbol prefixed with mlx5_ to the kernel. That's the value.

(+ what Jakub wrote)

> 
> Mark

Thanks,
Olek