Re: [PATCH for-next v3 2/2] RDMA/rxe: Call rxe_set_mtu after rxe_register_device

[Zhijian Li (Fujitsu)]

On 19/09/2023 09:11, Zhu Yanjun wrote:
> On Tue, Sep 19, 2023 at 8:57 AM Zhijian Li (Fujitsu)
> <lizhijian@xxxxxxxxxxx> wrote:
>>
>>
>>
>> On 18/09/2023 20:37, Leon Romanovsky wrote:
>>> On Mon, Sep 18, 2023 at 10:05:43AM +0800, Li Zhijian wrote:
>>>> rxe_set_mtu() will call rxe_info_dev() to print message, and
>>>> rxe_info_dev() expects dev_name(rxe->ib_dev->dev) has been assigned.
>>>>
>>>> Previously since dev_name() is not set, when a new rxe link is being
>>>> added, 'null' will be used as the dev_name like:
>>>>
>>>> "(null): rxe_set_mtu: Set mtu to 1024"
>>>>
>>>> Move rxe_register_device() earlier to assign the correct dev_name
>>>> so that it can be read by rxe_set_mtu() later.
>>>
>>> I would expect removal of that print line instead of moving
>>> rxe_register_device().
>>
>>
>> I also struggled with this point. The last option is keep it as it is.
>> Once rxe is registered, this print will work fine.
> 
> I delved into the source code. About moving rxe_register_device, I
> could not find any harm to the driver.

The point i'm struggling was that, it's strange/opaque to move rxe_register_device().
There is no doubt that the original order was more clear.

In terms of the message content, is it valuable to print(pr_info) this message, i noticed
that there is a duplicate pr_dbg() in rxe_notify().

rxe's mtu is always same with the NIC, isn't it ?

Thanks
Zhijian



> So I think this is also a solution to this problem.
> 
> Zhu Yanjun
> 
>>
>> Thanks
>> Zhijian
>>
>>
>>>
>>> Thanks
>>>
>>>>
>>>> And it's safe to do such change since mtu will not be used during the
>>>> rxe_register_device()
>>>>
>>>> After this change, the message becomes:
>>>> "rxe_eth0: rxe_set_mtu: Set mtu to 4096"
>>>>
>>>> Fixes: 9ac01f434a1e ("RDMA/rxe: Extend dbg log messages to err and info")
>>>> Reviewed-by: Daisuke Matsuda <matsuda-daisuke@xxxxxxxxxxx>
>>>> Reviewed-by: Zhu Yanjun <yanjun.zhu@xxxxxxxxx>
>>>> Signed-off-by: Li Zhijian <lizhijian@xxxxxxxxxxx>
>>>> ---
>>>>    drivers/infiniband/sw/rxe/rxe.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
>>>> index a086d588e159..8a43c0c4f8d8 100644
>>>> --- a/drivers/infiniband/sw/rxe/rxe.c
>>>> +++ b/drivers/infiniband/sw/rxe/rxe.c
>>>> @@ -169,10 +169,13 @@ void rxe_set_mtu(struct rxe_dev *rxe, unsigned int ndev_mtu)
>>>>     */
>>>>    int rxe_add(struct rxe_dev *rxe, unsigned int mtu, const char *ibdev_name)
>>>>    {
>>>> +    int ret;
>>>> +
>>>>       rxe_init(rxe);
>>>> +    ret = rxe_register_device(rxe, ibdev_name);
>>>>       rxe_set_mtu(rxe, mtu);
>>>>
>>>> -    return rxe_register_device(rxe, ibdev_name);
>>>> +    return ret;
>>>>    }
>>>>
>>>>    static int rxe_newlink(const char *ibdev_name, struct net_device *ndev)
>>>> --
>>>> 2.29.2
>>>>