Re: [PATCH for-next] RDMA/hns: Refactor the hns_roce_buf allocation flow

[liweihang <liweihang@huawei.com>]

On 2020/11/13 0:01, Jason Gunthorpe wrote:
> On Thu, Oct 29, 2020 at 06:31:02PM +0800, Weihang Li wrote:
>>  	/* The minimum shift of the page accessed by hw is HNS_HW_PAGE_SHIFT */
>> -	buf->page_shift = max_t(int, HNS_HW_PAGE_SHIFT, page_shift);
>> +	WARN_ON(page_shift < HNS_HW_PAGE_SHIFT);
> 
> Stuff like this should be written as
> 
>   if (WARN_ON(page_shift < HNS_HW_PAGE_SHIFT))
>     return ERR_PTR(-EINVAL);
> 
> Rather than wrongly continuing on

Thank you, I will correct it.

> 
>> @@ -780,19 +769,16 @@ static int mtr_alloc_bufs(struct hns_roce_dev *hr_dev, struct hns_roce_mtr *mtr,
>>  		ret = 0;
>>  	} else {
>>  		mtr->umem = NULL;
>> -		mtr->kmem = kzalloc(sizeof(*mtr->kmem), GFP_KERNEL);
>> -		if (!mtr->kmem) {
>> -			ibdev_err(ibdev, "Failed to alloc kmem\n");
>> +		mtr->kmem =
>> +			hns_roce_buf_alloc(hr_dev, total_size,
>> +					   buf_attr->page_shift,
>> +					   is_direct ? HNS_ROCE_BUF_DIRECT : 0);
>> +		if (IS_ERR_OR_NULL(mtr->kmem)) {
> 
> Please do not use IS_ERR_OR_NULL
> 
> Routines should not return NULL and an error pointer, one or the other
> - or NULL needs to have special meaning and is not an error.
> 
>> +			ibdev_err(ibdev, "failed to alloc kmem, ret = %ld.\n",
>> +				  PTR_ERR(mtr->kmem));
>>  			return -ENOMEM;
> 
> Here you should return PTR_ERR((mtr->kmem)
> 

OK, I will change it to:

	if (IS_ERR(mtr->kmem)) {
		ibdev_err(ibdev, "failed to alloc kmem, ret = %ld.\n",
			  PTR_ERR(mtr->kmem));
		return PTR_ERR(mtr->kmem);
	}


> In other places in this driver too, please check all the
> IS_ERR_OR_NULL's

Thanks for your reminder, I found some callers of IS_ERR_OR_NULL() in our
driver. I will check all of them and send another patch to fix them if
needed.

Weihang

> 
> Jason
>