On 28-Feb-19 00:13, Saleem, Shiraz wrote:
>> Subject: [PATCH rdma-next v2 09/11] RDMA/efa: Add EFA verbs implementation
>>
>> Add a file that implements the EFA verbs.
>>
>> Signed-off-by: Gal Pressman <galpress@xxxxxxxxxx>
>> ---
>> drivers/infiniband/hw/efa/efa_verbs.c | 1891
>> +++++++++++++++++++++++++++++++++
>> 1 file changed, 1891 insertions(+)
>> create mode 100644 drivers/infiniband/hw/efa/efa_verbs.c
>>
> [...]
>> +int efa_query_port(struct ib_device *ibdev, u8 port,
>> + struct ib_port_attr *props)
>> +{
>> + struct efa_dev *dev = to_edev(ibdev);
>> +
>> + memset(props, 0, sizeof(*props));
> I don't think you need to explicitly zero initialize this struct. It's already done by core in ib_query_port().
ACK.
>
> [....]
>
>> +struct ib_qp *efa_create_qp(struct ib_pd *ibpd,
>> + struct ib_qp_init_attr *init_attr,
>> + struct ib_udata *udata)
>> +{
>> + struct efa_com_create_qp_params create_qp_params = {};
>> + struct efa_com_create_qp_result create_qp_resp;
>> + struct efa_dev *dev = to_edev(ibpd->device);
>> + struct efa_ibv_create_qp_resp resp = {};
>> + struct efa_ibv_create_qp cmd = {};
>> + struct efa_ucontext *ucontext;
>> + struct efa_qp *qp;
>> + int err;
>> +
>> + ucontext = ibpd->uobject ? to_eucontext(ibpd->uobject->context) :
>> + NULL;
>> +
>> + if (!udata) {
>> + efa_err_rl(&dev->ibdev.dev, "udata is NULL\n");
>> + err = -EOPNOTSUPP;
>> + goto err_out;
>> + }
>> +
>> + err = efa_qp_validate_cap(dev, init_attr);
>> + if (err)
>> + goto err_out;
>> +
>> + err = efa_qp_validate_attr(dev, init_attr);
>> + if (err)
>> + goto err_out;
>> +
>> + if (!field_avail(cmd, driver_qp_type, udata->inlen)) {
>> + efa_err_rl(&dev->ibdev.dev,
>> + "Incompatible ABI params, no input udata\n");
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + if (udata->inlen > sizeof(cmd) &&
>> + !ib_is_udata_cleared(udata, sizeof(cmd),
>> + udata->inlen - sizeof(cmd))) {
>> + efa_err_rl(&dev->ibdev.dev,
>> + "Incompatible ABI params, unknown fields in udata\n");
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + err = ib_copy_from_udata(&cmd, udata,
>> + min(sizeof(cmd), udata->inlen));
>> + if (err) {
>> + efa_err_rl(&dev->ibdev.dev,
>> + "Cannot copy udata for create_qp\n");
>> + goto err_out;
>> + }
>> +
>> + if (cmd.comp_mask) {
>> + efa_err_rl(&dev->ibdev.dev,
>> + "Incompatible ABI params, unknown fields in udata\n");
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + qp = kzalloc(sizeof(*qp), GFP_KERNEL);
>> + if (!qp) {
>> + err = -ENOMEM;
>> + goto err_out;
>> + }
>> +
>> + create_qp_params.uarn = ucontext->uarn;
>> + create_qp_params.pd = to_epd(ibpd)->pdn;
>> +
>> + if (init_attr->qp_type == IB_QPT_UD) {
>> + create_qp_params.qp_type = EFA_ADMIN_QP_TYPE_UD;
>> + } else if (cmd.driver_qp_type == EFA_QP_DRIVER_TYPE_SRD) {
>> + create_qp_params.qp_type = EFA_ADMIN_QP_TYPE_SRD;
>> + } else {
>> + efa_err(&dev->ibdev.dev,
>> + "Unsupported qp type %d driver qp type %d\n",
>> + init_attr->qp_type, cmd.driver_qp_type);
>> + err = -EOPNOTSUPP;
>> + goto err_free_qp;
>> + }
>> +
>> + efa_dbg(&dev->ibdev.dev, "Create QP: qp type %d driver qp type
>> %#x\n",
>> + init_attr->qp_type, cmd.driver_qp_type);
>> + create_qp_params.send_cq_idx = to_ecq(init_attr->send_cq)->cq_idx;
>> + create_qp_params.recv_cq_idx = to_ecq(init_attr->recv_cq)->cq_idx;
>> + create_qp_params.sq_depth = init_attr->cap.max_send_wr;
>> + create_qp_params.sq_ring_size_in_bytes = cmd.sq_ring_size;
>> +
>> + create_qp_params.rq_depth = init_attr->cap.max_recv_wr;
>> + create_qp_params.rq_ring_size_in_bytes = cmd.rq_ring_size;
>> + qp->rq_size = PAGE_ALIGN(create_qp_params.rq_ring_size_in_bytes);
>> + if (qp->rq_size) {
>> + qp->rq_cpu_addr = efa_zalloc_mapped(dev, &qp->rq_dma_addr,
>> + qp->rq_size,
>> DMA_TO_DEVICE);
>> + if (!qp->rq_cpu_addr) {
>> + err = -ENOMEM;
>> + goto err_free_qp;
>> + }
>> +
>> + efa_dbg(&dev->ibdev.dev,
>> + "qp->cpu_addr[%p] allocated: size[%lu], dma[%pad]\n",
>> + qp->rq_cpu_addr, qp->rq_size, &qp->rq_dma_addr);
>> + create_qp_params.rq_base_addr = qp->rq_dma_addr;
>> + }
>> +
>> + memset(&resp, 0, sizeof(resp));
>> + err = efa_com_create_qp(dev->edev, &create_qp_params,
>> + &create_qp_resp);
>> + if (err)
>> + goto err_free_mapped;
>> +
>> + WARN_ON_ONCE(create_qp_resp.sq_db_offset > dev->db_bar_len);
>> + WARN_ON_ONCE(create_qp_resp.rq_db_offset > dev->db_bar_len);
>> + WARN_ON_ONCE(create_qp_resp.llq_descriptors_offset >
>> + dev->mem_bar_len);
>
> I saw many instances of WARN or WARN_ON_ONCE in the driver. Is this what you
> really want? Perhaps a pr_ or dev_ warn variant?
This WARNs can happily retire by now :), I'll remove them.
>
> [...]
>> +static struct ib_cq *do_create_cq(struct ib_device *ibdev, int entries,
>> + int vector, struct ib_ucontext *ibucontext,
>> + struct ib_udata *udata)
>> +{
>> + struct efa_ibv_create_cq_resp resp = {};
>> + struct efa_com_create_cq_params params;
>> + struct efa_com_create_cq_result result;
>> + struct efa_dev *dev = to_edev(ibdev);
>> + struct efa_ibv_create_cq cmd = {};
>> + struct efa_cq *cq;
>> + int err;
>> +
>> + efa_dbg(&ibdev->dev, "create_cq entries %d udata %p\n", entries,
>> udata);
>> +
>> + if (entries < 1 || entries > dev->dev_attr.max_cq_depth) {
>> + efa_err(&ibdev->dev,
>> + "cq: requested entries[%u] non-positive or greater than
>> max[%u]\n",
>> + entries, dev->dev_attr.max_cq_depth);
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + if (!udata) {
>> + efa_err_rl(&ibdev->dev, "udata is NULL\n");
>> + err = -EOPNOTSUPP;
>> + goto err_out;
>> + }
>> +
>> + if (!field_avail(cmd, num_sub_cqs, udata->inlen)) {
>> + efa_err_rl(&ibdev->dev,
>> + "Incompatible ABI params, no input udata\n");
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + if (udata->inlen > sizeof(cmd) &&
>> + !ib_is_udata_cleared(udata, sizeof(cmd),
>> + udata->inlen - sizeof(cmd))) {
>> + efa_err_rl(&ibdev->dev,
>> + "Incompatible ABI params, unknown fields in udata\n");
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + err = ib_copy_from_udata(&cmd, udata,
>> + min(sizeof(cmd), udata->inlen));
>> + if (err) {
>> + efa_err_rl(&ibdev->dev,
>> + "Cannot copy udata for create_cq\n");
>> + goto err_out;
>> + }
>> +
>> + if (cmd.comp_mask || !is_reserved_cleared(cmd.reserved_50)) {
>> + efa_err_rl(&ibdev->dev,
>> + "Incompatible ABI params, unknown fields in udata\n");
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + if (!cmd.cq_entry_size) {
>> + efa_err(&ibdev->dev,
>> + "Invalid entry size [%u]\n", cmd.cq_entry_size);
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + if (cmd.num_sub_cqs != dev->dev_attr.sub_cqs_per_cq) {
>> + efa_err(&ibdev->dev,
>> + "Invalid number of sub cqs[%u] expected[%u]\n",
>> + cmd.num_sub_cqs, dev->dev_attr.sub_cqs_per_cq);
>> + err = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + cq = kzalloc(sizeof(*cq), GFP_KERNEL);
>> + if (!cq) {
>> + err = -ENOMEM;
>> + goto err_out;
>> + }
>> +
>> + memset(&resp, 0, sizeof(resp));
>
> This was already initialized. Few instances of these..
ACK.
>
>> +
>> +/*
>> + * initialize pbl indirect mode:
>> + * create a chunk list out of the dma addresses of the physical pages of
>> + * pbl buffer.
>> + */
>> +static int pbl_indirect_initialize(struct efa_dev *dev, struct pbl_context *pbl)
>> +{
>> + u32 size_in_pages = DIV_ROUND_UP(pbl->pbl_buf_size_in_bytes,
>> + EFA_PAGE_SIZE);
>> + struct scatterlist *sgl;
>> + int sg_dma_cnt, err;
>> +
>> + sgl = efa_vmalloc_buf_to_sg(pbl->pbl_buf, size_in_pages);
>> + if (!sgl)
>> + return -ENOMEM;
>> +
>> + sg_dma_cnt = dma_map_sg(&dev->pdev->dev, sgl, size_in_pages,
>> DMA_TO_DEVICE);
>> + if (!sg_dma_cnt) {
>> + err = -EINVAL;
>> + goto err_map;
>> + }
>> +
>> + pbl->phys.indirect.pbl_buf_size_in_pages = size_in_pages;
>> + pbl->phys.indirect.sgl = sgl;
>> + pbl->phys.indirect.sg_dma_cnt = sg_dma_cnt;
>> + err = pbl_chunk_list_create(dev, pbl);
>> + if (err) {
>> + efa_err(&dev->ibdev.dev,
>> + "chunk_list creation failed[%d]\n", err);
>> + goto err_chunk;
>> + }
>
> Seems like you have many error prints in this driver. Do you really want this to show up
> in dmesg or just the critical ones? Consider dev or pr_dbg?
I will audit these.
>
> Reviewed-by: Shiraz Saleem <shiraz.saleem@xxxxxxxxx>
Thanks Shiraz!
