Re: [PATCH for-next] RDMA/hns: Optimize eqe buffer allocation flow

[Weihang Li <liweihang@huawei.com>]

On 2020/2/12 16:18, Leon Romanovsky wrote:
> On Mon, Feb 10, 2020 at 07:26:59PM +0800, Weihang Li wrote:
>>
>>
>> On 2020/2/10 18:21, Leon Romanovsky wrote:
>>> On Mon, Feb 10, 2020 at 05:48:05PM +0800, Weihang Li wrote:
>>>>
>>>>
>>>> On 2020/2/10 17:25, Leon Romanovsky wrote:
>>>>>>>> -		if (!eq->bt_l0)
>>>>>>>> -			return -ENOMEM;
>>>>>>>> -
>>>>>>>> -		eq->cur_eqe_ba = eq->l0_dma;
>>>>>>>> -		eq->nxt_eqe_ba = 0;
>>>>>>>> +	/* alloc a tmp list for storing eq buf address */
>>>>>>>> +	ret = hns_roce_alloc_buf_list(&region, &buf_list, 1);
>>>>>>>> +	if (ret) {
>>>>>>>> +		dev_err(hr_dev->dev, "alloc eq buf_list error\n");
>>>>>>> The same comment like we gave for bnxt driver, no dev_* prints inside
>>>>>>> driver, use ibdev_*.
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>> Hi Leon,
>>>>>>
>>>>>> map_eq_buf() is called before ib_register_device(), so we can't use
>>>>>> ibdev_* here.
>>>>> As long as map_eq_buf() is called after ib_alloc_device(), you will be fine.
>>>>>
>>>>> Thanks
>>>>
>>>> Hi Leon,
>>>>
>>>> eq is used to queue hardware event, it should be ready before hardware is initialized.
>>>> So we can't call map_eq_buf() after ib_alloc_device().
>>>
>>> How can it be that your newly added function has hns_roce_dev in the
>>> signature and you didn't call to ib_alloc_device()?
>>>
>>>  +static int map_eq_buf(struct hns_roce_dev *hr_dev, struct hns_roce_eq *eq,
>>>  +                u32 page_shift)
>>>
>>> Thanks
>>>
>>
>> Sorry, I confused ib_alloc_device() and ib_register_device(). What I was about to say is
>> ib_register_device().
>>
>> Order of these functions in hns driver is:
>>
>> 1. ib_alloc_device()
>> 2. map_eq_buf()
>> 3. ib_register_device()
>>
>> Refer to code in __ibdev_printk():
>>
>> 	else if (ibdev)
>> 		printk("%s%s: %pV",
>> 		       level, dev_name(&ibdev->dev), vaf);
>>
>>
>> If we called ibdev_*() before ib_register_device(), it will print "null" for the device
>> name. And I make a simple test, it will print like this:
>>
>> [   41.400347] (null): -------------- This is a test!----------
>>
>> Because map_eq_buf() should be finished before ib_register_device(), so I think we have
>> to use dev_*() in it.
> 
> Interesting, I wonder why "ibdev->dev" is set so late. I afraid that it
> is a bug in hns.
> 
> Thanks
> 

I will check the code in hns and ib core to find out the reasons. Thank you.

Weihang

>>
>>>>
>>>> Thanks
>>>> Weihang
>>>>
>>>>>
>>>>>> Thanks for your reminder, another patch that replace other dev_* in
>>>>>> hns driver with ibdev_* is on preparing.
>>>>>>
>>>>>> Weihang
>>>>>>
>>>>>>> .
>>>>>>>
>>>>> .
>>>>>
>>>>
>>>
>>> .
>>>
>>
> 
> .
>