IB/hns: Add driver files for hns RoCE driver

Hello oulijun,

The patch 9a4435375cd1: "IB/hns: Add driver files for hns RoCE driver" from Jul 21, 2016, leads to the following static checker warning:

	drivers/infiniband/hw/hns/hns_roce_mr.c:575 hns_roce_reg_user_mr()
	warn: no lower bound on 'n'

drivers/infiniband/hw/hns/hns_roce_mr.c
542 struct ib_mr *hns_roce_reg_user_mr(struct ib_pd *pd, u64 start, u64 length,
543 u64 virt_addr, int access_flags,
544 struct ib_udata *udata)
545 {

   546          struct hns_roce_dev *hr_dev = to_hr_dev(pd->device);
   547          struct device *dev = &hr_dev->pdev->dev;
   548          struct hns_roce_mr *mr = NULL;
   549          int ret = 0;
   550          int n = 0;
                ^^^^^^^^^

Notice this is signed. Please don't initialize variables to bogus values. The compiler has a feature to warn about uninitialized variables but by assigning bogus valus to "n" then you are disabling the safety checks and potentially hiding bugs.

   551  
   552          mr = kmalloc(sizeof(*mr), GFP_KERNEL);
   553          if (!mr)
   554                  return ERR_PTR(-ENOMEM);
   555  
   556          mr->umem = ib_umem_get(pd->uobject->context, start, length,
   557                                 access_flags, 0);
   558          if (IS_ERR(mr->umem)) {
   559                  ret = PTR_ERR(mr->umem);
   560                  goto err_free;
   561          }
   562  
   563          n = ib_umem_page_count(mr->umem);

Depending on the config then ib_umem_page_count() can return -EINVAL. Probably it's not possible here. Anyway, probably the right thing is to check:

	if (n < 0) {
		ret = -EINVAL;
		goto umem;
	}

It silences the static checker warning.

564 if (mr->umem->page_size != HNS_ROCE_HEM_PAGE_SIZE) {
565 dev_err(dev, "Just support 4K page size but is 0x%x now!\n",
566 mr->umem->page_size);

Should we continue here or is there supposed to be a goto umem;?

567 }
568
569 if (n > HNS_ROCE_MAX_MTPT_PBL_NUM) {
570 dev_err(dev, " MR len %lld err. MR is limited to 4G at most!\n",
571 length);

We should be setting "ret = -EINVAL;" here.

572 goto err_umem;
573 }
574
575 ret = hns_roce_mr_alloc(hr_dev, to_hr_pd(pd)->pdn, virt_addr, length,
576 access_flags, n, mr);
577 if (ret)
578 goto err_umem;
579

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks, we will fix it in next patch!

Thanks for your reviewing. I have checked the ib_umem_page_count(), Maybe it will not return the -EINVAL

when configured CONFIG_INFINIBAND_USER_MEM, the function is

int ib_umem_page_count(struct ib_umem *umem)

{
	int shift;
	int i;
	int n;
	struct scatterlist *sg;

	if (umem->odp_data)
		return ib_umem_num_pages(umem);

	shift = ilog2(umem->page_size);

	n = 0;
	for_each_sg(umem->sg_head.sgl, sg, umem->nmap, i)
		n += sg_dma_len(sg) >> shift;

	return n;
}

EXPORT_SYMBOL(ib_umem_page_count);

when not configured CONFIG_INFINIBAND_USER_MEM, the function as follow:
static inline int ib_umem_page_count(struct ib_umem *umem) { return 0; }

Thanks, Dan Carpenter
We have taken notice of the problem and have sent a patch to Doug to fix last two problem. the patch link as follow:
https://patchwork.kernel.org/patch/9342015/

Thanks, Dan Carpenter
We have taken notice of the problem and have sent a patch to Doug to fix last two problem. the patch link as follow:
https://patchwork.kernel.org/patch/9342015/

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Ah. You're correct, of course. I was looking at the wrong function.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html