Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Wenjia Zhang <wenjia@linux.ibm.com>]

On 27.10.24 21:18, Leon Romanovsky wrote:
> On Fri, Oct 25, 2024 at 09:23:55AM +0200, Wenjia Zhang wrote:
>> Commit c2261dd76b54 ("RDMA/device: Add ib_device_set_netdev() as an
>> alternative to get_netdev") introduced an API ib_device_get_netdev.
>> The SMC-R variant of the SMC protocol continued to use the old API
>> ib_device_ops.get_netdev() to lookup netdev.
> 
> I would say that calls to ibdev ops from ULPs was never been right
> thing to do. The ib_device_set_netdev() was introduced for the drivers.
> 
> So the whole commit message is not accurate and better to be rewritten.
> 
>> As this commit 8d159eb2117b
>> ("RDMA/mlx5: Use IB set_netdev and get_netdev functions") removed the
>> get_netdev callback from mlx5_ib_dev_common_roce_ops, calling
>> ib_device_ops.get_netdev didn't work any more at least by using a mlx5
>> device driver.
> 
> It is not a correct statement too. All modern drivers (for last 5 years)
> don't have that .get_netdev() ops, so it is not mlx5 specific, but another
> justification to say that SMC-R was doing it wrong.
> 
>> Thus, using ib_device_set_netdev() now became mandatory.
> 
> ib_device_set_netdev() is mandatory for the drivers, it is nothing to do
> with ULPs.
> 
>>
>> Replace ib_device_ops.get_netdev() with ib_device_get_netdev().
> 
> It is too late for me to do proper review for today, but I would say
> that it is worth to pay attention to multiple dev_put() calls in the
> functions around the ib_device_get_netdev().
> 
>>
>> Fixes: 54903572c23c ("net/smc: allow pnetid-less configuration")
>> Fixes: 8d159eb2117b ("RDMA/mlx5: Use IB set_netdev and get_netdev functions")
> 
> It is not related to this change Fixes line.
> 

Hi Leon,

Thank you for the review! I agree that SMC could do better. However, we 
should fix it and give enough information and reference on the changes, 
since the code has already existed and didn't work with the old way. I 
can rewrite the commit message.

What about:
"
The SMC-R variant of the SMC protocol still called 
ib_device_ops.get_netdev() to lookup netdev. As we used mlx5 device 
driver to run SMC-R, it failed to find a device, because in mlx5_ib the 
internal net device management for retrieving net devices was replaced 
by a common interface ib_device_get_netdev() in commit 8d159eb2117b 
("RDMA/mlx5: Use IB set_netdev and get_netdev functions"). Thus, replace 
ib_device_ops.get_netdev() with ib_device_get_netdev() in SMC.
"

Thanks,
Wenjia