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

[Leon Romanovsky <leon@kernel.org>]

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@hisilicon.com>
> >>>>
> >>>> 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