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

[Leon Romanovsky <leon@kernel.org>]

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

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