>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().
[....]
>+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?
[...]
>+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..
>+
>+/*
>+ * 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?
Reviewed-by: Shiraz Saleem <shiraz.saleem@xxxxxxxxx>
