Re: [PATCH for-next 2/9] RDMA/hns: Refactor the codes of creating qp

[Doug Ledford <dledford@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3535 bytes --]

On Wed, 2019-08-21 at 21:14 +0800, Lijun Ou wrote:
> +static int hns_roce_alloc_recv_inline_buffer(struct hns_roce_qp
> *hr_qp,
> +                                            struct ib_qp_init_attr
> *init_attr)
> +{
> +       int ret;
> +       int i;
> +
> +       /* allocate recv inline buf */
> +       hr_qp->rq_inl_buf.wqe_list = kcalloc(hr_qp->rq.wqe_cnt,
> +                                            sizeof(struct
> hns_roce_rinl_wqe),
> +                                            GFP_KERNEL);
> +       if (!hr_qp->rq_inl_buf.wqe_list) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +
> +       hr_qp->rq_inl_buf.wqe_cnt = hr_qp->rq.wqe_cnt;
> +
> +       /* Firstly, allocate a list of sge space buffer */
> +       hr_qp->rq_inl_buf.wqe_list[0].sg_list =
> +                                       kcalloc(hr_qp-
> >rq_inl_buf.wqe_cnt,
> +                                       init_attr->cap.max_recv_sge *
> +                                       sizeof(struct
> hns_roce_rinl_sge),
> +                                       GFP_KERNEL);
> +       if (!hr_qp->rq_inl_buf.wqe_list[0].sg_list) {
> +               ret = -ENOMEM;
> +               goto err_wqe_list;
> +       }
> +
> +       for (i = 1; i < hr_qp->rq_inl_buf.wqe_cnt; i++)
> +               /* Secondly, reallocate the buffer */
> +               hr_qp->rq_inl_buf.wqe_list[i].sg_list =
> +                                    &hr_qp-
> >rq_inl_buf.wqe_list[0].sg_list[i *
> +                                    init_attr->cap.max_recv_sge];
> +
> +       return 0;
> +
> +err_wqe_list:
> +       kfree(hr_qp->rq_inl_buf.wqe_list);
> +
> +err:
> +       return ret;
> +}

This function is klunky.  You don't need int ret; at all as there are
only two possible return values and you have distinct locations for each
return, so each return can use a constant.  It would be much more
readable like this:

+static int hns_roce_alloc_recv_inline_buffer(struct hns_roce_qp *hr_qp,
+                                            struct ib_qp_init_attr *init_attr)
+{
+	int num_sge = init_attr->cap.max_recv_sge;
+	int wqe_cnt = hr_qp->rq.wqe_cnt;
+       int i;
+
+       /* allocate recv inline WQE bufs */
+       hr_qp->rq_inl_buf.wqe_list = kcalloc(wqe_cnt,
+                                            sizeof(struct hns_roce_rinl_wqe),
+                                            GFP_KERNEL);
+       if (!hr_qp->rq_inl_buf.wqe_list)
+               goto err;
+
+       hr_qp->rq_inl_buf.wqe_cnt = wqe_cnt;
+
+       /* allocate a single sge array for all WQEs */
+       hr_qp->rq_inl_buf.wqe_list[0].sg_list =
+                                       kcalloc(wqe_cnt,
+                                       num_sge *
+                                       sizeof(struct hns_roce_rinl_sge),
+                                       GFP_KERNEL);
+       if (!hr_qp->rq_inl_buf.wqe_list[0].sg_list)
+               goto err_wqe_list;
+
+       for (i = 1; i < wqe_cnt; i++)
+               /* give each WQE a pointer to its array space */
+               hr_qp->rq_inl_buf.wqe_list[i].sg_list =
+                           &hr_qp->rq_inl_buf.wqe_list[0].sg_list[i * num_sge];
+
+       return 0;
+
+err_wqe_list:
+       kfree(hr_qp->rq_inl_buf.wqe_list);
+err:
+       return -ENOMEM;
+}

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]