On Fri, Feb 15, 2019 at 11:10:59AM -0600, Shiraz Saleem wrote:

> +static int irdma_alloc_pd(struct ib_pd *pd,
> +                       struct ib_ucontext *context,
> +                       struct ib_udata *udata)
> +{
> +     struct irdma_pd *iwpd = to_iwpd(pd);
> +     struct irdma_device *iwdev = to_iwdev(pd->device);
> +     struct irdma_sc_dev *dev = &iwdev->rf->sc_dev;
> +     struct irdma_pci_f *rf = iwdev->rf;
> +     struct irdma_alloc_pd_resp uresp = {};
> +     struct irdma_sc_pd *sc_pd;
> +     struct irdma_ucontext *ucontext;
> +     u32 pd_id = 0;
> +     int err;
> +
> +     if (iwdev->closing)
> +             return -ENODEV;

No crazy unlocked 'closing' flags. The core code takes care of
everything a driver needs to worry about if you use it properly.

> +/**
> + * irdma_create_cq - create cq
> + * @ibdev: device pointer from stack
> + * @attr: attributes for cq
> + * @context: user context created during alloc
> + * @udata: user data
> + */
> +static struct ib_cq *irdma_create_cq(struct ib_device *ibdev,
> +                                  const struct ib_cq_init_attr *attr,
> +                                  struct ib_ucontext *context,
> +                                  struct ib_udata *udata)
> +{
> +     struct irdma_device *iwdev = to_iwdev(ibdev);
> +     struct irdma_pci_f *rf = iwdev->rf;
> +     struct irdma_cq *iwcq;
> +     struct irdma_pbl *iwpbl;
> +     u32 cq_num = 0;
> +     struct irdma_sc_cq *cq;
> +     struct irdma_sc_dev *dev = &rf->sc_dev;
> +     struct irdma_cq_init_info info = {};
> +     enum irdma_status_code status;
> +     struct irdma_cqp_request *cqp_request;
> +     struct cqp_cmds_info *cqp_info;
> +     struct irdma_cq_uk_init_info *ukinfo = &info.cq_uk_init_info;
> +     unsigned long flags;
> +     int err_code;
> +     int entries = attr->cqe;
> +
> +     if (iwdev->closing)
> +             return ERR_PTR(-ENODEV);
> +
> +     if (entries > rf->max_cqe)
> +             return ERR_PTR(-EINVAL);
> +
> +     iwcq = kzalloc(sizeof(*iwcq), GFP_KERNEL);
> +     if (!iwcq)
> +             return ERR_PTR(-ENOMEM);
> +
> +     err_code = irdma_alloc_rsrc(rf, rf->allocated_cqs,
> +                                 rf->max_cq, &cq_num,
> +                                 &rf->next_cq);
> +     if (err_code)
> +             goto error;
> +
> +     cq = &iwcq->sc_cq;
> +     cq->back_cq = (void *)iwcq;
> +     spin_lock_init(&iwcq->lock);
> +     info.dev = dev;
> +     ukinfo->cq_size = max(entries, 4);
> +     ukinfo->cq_id = cq_num;
> +     iwcq->ibcq.cqe = info.cq_uk_init_info.cq_size;
> +     if (attr->comp_vector < rf->ceqs_count)
> +             info.ceq_id = attr->comp_vector;
> +     info.ceq_id_valid = true;
> +     info.ceqe_mask = 1;
> +     info.type = IRDMA_CQ_TYPE_IWARP;
> +     info.vsi = &iwdev->vsi;
> +
> +     if (context) {

Drivers should rarely write 'if context'. The test for userspaceness
is 'if (udata)' - and in this case context is guarenteed. Lots of
places with this wrong..

Also this will need to be rebased as this all changed.

> +     return (struct ib_cq *)iwcq;

And don't write casts like that, &iwcq->ib_qp or something. 

Find and fix them all please.

> +/**
> + * irdma_set_page - populate pbl list for fmr
> + * @ibmr: ib mem to access iwarp mr pointer
> + * @addr: page dma address fro pbl list
> + */
> +static int irdma_set_page(struct ib_mr *ibmr,
> +                       u64 addr)

Can you please read through this giant driver and hit various places
with wonky formatting with clang-format? We don't need to start out a
new driver with bonkers indentation.
> +
> +static const struct ib_device_ops irdma_roce_dev_ops = {
> +     .get_link_layer = irdma_get_link_layer,
> +     .query_ah = irdma_query_ah,
> +     .attach_mcast = irdma_attach_mcast,
> +     .detach_mcast = irdma_detach_mcast,
> +     .query_gid = irdma_query_gid_roce,
> +     .modify_qp = irdma_modify_qp_roce,
> +};
> +
> +static const struct ib_device_ops irdma_iw_dev_ops = {
> +     .query_gid = irdma_query_gid,
> +     .modify_qp = irdma_modify_qp,
> +};
> +
> +static const struct ib_device_ops irdma_dev_ops = {
> +     .get_port_immutable = irdma_port_immutable,
> +     .get_netdev = irdma_get_netdev,
> +     .query_port = irdma_query_port,
> +     .modify_port = irdma_modify_port,
> +     .query_pkey = irdma_query_pkey,
> +     .alloc_ucontext = irdma_alloc_ucontext,
> +     .dealloc_ucontext = irdma_dealloc_ucontext,
> +     .mmap = irdma_mmap,
> +     .alloc_pd = irdma_alloc_pd,
> +     .dealloc_pd = irdma_dealloc_pd,
> +     .create_qp = irdma_create_qp,
> +     .query_qp = irdma_query_qp,
> +     .destroy_qp = irdma_destroy_qp,
> +     .create_cq = irdma_create_cq,
> +     .destroy_cq = irdma_destroy_cq,
> +     .get_dma_mr = irdma_get_dma_mr,
> +     .reg_user_mr = irdma_reg_user_mr,
> +     .dereg_mr = irdma_dereg_mr,
> +     .alloc_mw = irdma_alloc_mw,
> +     .dealloc_mw = irdma_dealloc_mw,
> +     .alloc_hw_stats = irdma_alloc_hw_stats,
> +     .get_hw_stats = irdma_get_hw_stats,
> +     .query_device = irdma_query_device,
> +     .create_ah = irdma_create_ah,
> +     .destroy_ah = irdma_destroy_ah,
> +     .drain_sq = irdma_drain_sq,
> +     .drain_rq = irdma_drain_rq,
> +     .alloc_mr = irdma_alloc_mr,
> +     .map_mr_sg = irdma_map_mr_sg,
> +     .get_dev_fw_str = irdma_get_dev_fw_str,
> +     .poll_cq = irdma_poll_cq,
> +     .req_notify_cq = irdma_req_notify_cq,
> +     .post_send = irdma_post_send,
> +     .post_recv = irdma_post_recv,
> +     .disassociate_ucontext = irdma_disassociate_ucontext,
> +     INIT_RDMA_OBJ_SIZE(ib_pd, irdma_pd, ibpd),
> +};

All lists of things should be sorted. I saw many examples of unsorted
lists.

> +/**
> + * irdma_init_roce_device - initialization of iwarp rdma device
> + * @iwibdev: irdma ib device
> + */
> +static int irdma_init_iw_device(struct irdma_ib_device *iwibdev)
> +{
> +     struct net_device *netdev = iwibdev->iwdev->netdev;
> +
> +     iwibdev->ibdev.node_type = RDMA_NODE_RNIC;
> +     ether_addr_copy((u8 *)&iwibdev->ibdev.node_guid, netdev->dev_addr);
> +     iwibdev->ibdev.iwcm = kzalloc(sizeof(*iwibdev->ibdev.iwcm), GFP_KERNEL);
> +     if (!iwibdev->ibdev.iwcm)
> +             return -ENOMEM;
> +
> +     iwibdev->ibdev.iwcm->add_ref = irdma_add_ref;
> +     iwibdev->ibdev.iwcm->rem_ref = irdma_rem_ref;
> +     iwibdev->ibdev.iwcm->get_qp = irdma_get_qp;
> +     iwibdev->ibdev.iwcm->connect = irdma_connect;
> +     iwibdev->ibdev.iwcm->accept = irdma_accept;
> +     iwibdev->ibdev.iwcm->reject = irdma_reject;
> +     iwibdev->ibdev.iwcm->create_listen = irdma_create_listen;
> +     iwibdev->ibdev.iwcm->destroy_listen = irdma_destroy_listen;

Huh. These should probably be moved into the ops structure too.

> +/**
> + * irdma_register_rdma_device - register iwarp device to IB
> + * @iwdev: iwarp device
> + */
> +int irdma_register_rdma_device(struct irdma_device *iwdev)
> +{
> +     int ret;
> +     struct irdma_ib_device *iwibdev;
> +
> +     ret = irdma_init_rdma_device(iwdev);
> +     if (ret)
> +             return ret;
> +
> +     iwibdev = iwdev->iwibdev;
> +     rdma_set_device_sysfs_group(&iwibdev->ibdev, &irdma_attr_group);
> +     if (iwdev->rf->sc_dev.hw_attrs.hw_rev == IRDMA_GEN_1)
> +             /* backward compat with old user-space libi40iw */
> +             iwibdev->ibdev.driver_id = RDMA_DRIVER_I40IW;

Really? Then what is the problem in rdma-core? 

Why are we getting a replacement driver instead of fixing the old one?

This is very long, I didn't read it super closely :(

Jason

Reply via email to