Re: [PATCH v2 for-next] RDMA/core: Get IB width and speed from netdev

[Junxian Huang]

On 2023/7/5 15:12, Leon Romanovsky wrote:
> On Wed, Jul 05, 2023 at 11:05:50AM +0800, Junxian Huang wrote:
>>
>>
>> On 2023/6/28 13:00, Leon Romanovsky wrote:
>>> On Mon, Jun 19, 2023 at 02:20:54PM +0800, Junxian Huang wrote:
>>>>
>>>>
>>>> On 2023/6/12 1:46, Leon Romanovsky wrote:
>>>>> On Sat, Jun 03, 2023 at 02:38:33PM +0800, Junxian Huang wrote:
>>>>>> From: Haoyue Xu <xuhaoyue1@xxxxxxxxxxxxx>
>>>>>>
>>>>>> Previously, there was no way to query the number of lanes for a network
>>>>>> card, so the same netdev_speed would result in a fixed pair of width and
>>>>>> speed. As network card specifications become more diverse, such fixed
>>>>>> mode is no longer suitable, so a method is needed to obtain the correct
>>>>>> width and speed based on the number of lanes.
>>>>>
>>>>> I'm sorry but I didn't understand the problem statement. Can you please
>>>>> provide an example of configuration that will give different results 
>>>>> before this patch and after?
>>>>>
>>>>
>>>> I'll give examples with 20G and 200G netdevs respectively.
>>>>
>>>> 20G:
>>>> Before this patch, regardless of the actual number of lanes, the width and
>>>> speed displayed in ibv_devinfo would be always fixed:
>>>> 	active_width: 4X
>>>> 	active_speed: 5 Gbps
>>>> After this patch, there will be different combinations of width and speed
>>>> according to the number of lanes. For example, for a 20G netdev whose number
>>>> of lanes is 2, the width and speed displayed in ibv_devinfo will be:
>>>> 	active_width: 2X
>>>> 	active_speed: 10 Gbps
>>>>
>>>> 200G:
>>>> Before this patch, netdevs with netdev_speed more than 40G cannot get a right
>>>> width and speed in ibv_devinfo. Only the default result would be displayed:
>>>> 	active_width: 4X
>>>> 	active_speed: 25 Gbps
>>>> After this patch, taking an example with 4 lanes, the displayed results will be:
>>>> 	active_width: 4X
>>>> 	active_speed: 50 Gbps
>>>>
>>>
>>> <...>
>>>
>>>>>> +	cap_link_lanes_supported = netdev->ethtool_ops->cap_link_lanes_supported;
>>>>>>  	rtnl_unlock();
>>>>>>  
>>>>>>  	dev_put(netdev);
>>>>>>  
>>>>>>  	if (!rc && lksettings.base.speed != (u32)SPEED_UNKNOWN) {
>>>>>>  		netdev_speed = lksettings.base.speed;
>>>>>> +		if (cap_link_lanes_supported && lksettings.lanes) {
>>>>>
>>>>> According to the documentation cap_link_lanes_supported defines if
>>>>> number of lanes can be supplied by user and I would expect from
>>>>> __ethtool_get_link_ksettings() to get right numbers after it was
>>>>> changed.
>>>
>>> No, I'm saying that cap_link_lanes_supported is variable which only says
>>> if number of lanes can be changed and __ethtool_get_link_ksettings()
>>> will return right number of lanes every time it is called without need
>>> to call to ib_get_width_and_speed() again.
>>>
>>> Thanks
>>>
>>
>> These two functions have different purposes.
>>
>> The number of lanes is indeed obtained from __ethtool_get_link_ksettings(),
>> and ib_get_width_and_speed() converts the number of lanes into ib_width and
>> ib_speed, which are the output of ib_get_eth_speed().
> 
> Great, so why do you need to rely on cap_link_lanes_supported in ib_get_width_and_speed()?
> 
> Thanks

Ah, I see what you mean. If cap_link_lanes_supported is false, lksettings.lanes will become 0,
and ib_get_width_and_speed() won't be called. Therefore, cap_link_lanes_supported is redundant.

I'll fix it in v3. Thanks.

Junxian