RE: [PATCH] qed: Remove unused QED_RDMA_DEV_CAP_* symbols and dev->dev_caps

> From: Bjorn Helgaas > > The QED_RDMA_DEV_CAP_* symbols are only used to set bits in dev->dev_caps. > Nobody ever looks at those bits. Remove the symbols and dev_caps itself. > > Note that if these are ever used and added back, it looks incorrect to set >

RE: [PATCH net 1/3] qed: Release CQ resource under lock on failure

> The bitmap functions are being used by various bitmaps one of which is the > cid. > The cid code must allocate two consecutive cids. So here the lock is over two > calls > to the bit allocating call. > > I am planning to recode the cid to use one bit instead of two, this is further > down the

RE: [PATCH net 1/3] qed: Release CQ resource under lock on failure

> Minor suggestion. > Can you consider embedding the lock and unlock inside qed_bmap_release_id? > There are two places where this is bad idea as driver needs to release two > IDs but still one is in error flow and second is when destroying QP so for > most cases code may look a bit better. >

RE: [RFC v3 00/11] QLogic RDMA Driver (qedr) RFC

> The series adds on top of RFC v2: > * fix licensing header to dual license > * remove 'debug' module paramter and make use of pr_debug > * relocation of qedr user API to include/rdma/uapi/ > * use the __u32/64 in uapi and include types.h > * advance ABI version (since shifting to __u32/64

RE: [RFC v2 06/12] qedr: Add support for QP verbs

> > Do you have a git tree? > We don't have a publicly accessible git tree.

RE: [RFC v2 00/11] QLogic RDMA Driver (qedr) RFC

> > The module parameters is no-go. > We'll remove it.

RE: [RFC v2 04/12] qedr: Add support for user context verbs

> Are you sure that you want GPL-only license? Thanks for pointing this out. This is something we will obviously fix. > > +#define QEDR_ABI_VERSION (6) > > Is it possible to start new file with new driver from ABI version 1 and not 6? This is something that we cannot do since we

RE: [RFC v2 06/12] qedr: Add support for QP verbs

> > include/uapi/rdma/providers/qedr-abi.h | 35 + > > Please be aware that the last version for ABI header files doesn't have > "provider" > name in directory path (include/uapi/rdma/qedr-abi.h) OK. Note that I was using https://www.spinics.net/lists/linux-rdma/msg40414.html that dates

RE: [RFC v2 06/12] qedr: Add support for QP verbs

> Ugh, each patch keeps adding to this? The logic in the patch series is to have each patch contain only what is necessary for the specific functionality it adds. This made it harder on us to prepare but, IMHO, easier for the reviewer. If you'd like to have this file in one chunk, I can do this.

RE: [RFC v2 04/12] qedr: Add support for user context verbs

> uapi headers need the __ varients and the #include > > Follow what Leon has done. OK

RE: [RFC 03/11] Add support for RoCE HW init

> > > + dev->max_sge = min_t(u32, RDMA_MAX_SGE_PER_SQ_WQE, > > > + RDMA_MAX_SGE_PER_RQ_WQE); > > > > Our kernel target mode consumers sort of rely on max_sge_rd, you need > > to make sure to set it too. > Good catch. Thanks! Actually, as I come to code this, I just realized

RE: [RFC 06/11] Add support for QP verbs

> > + > > + rc = ib_get_cached_gid(ibqp->device, attr->ah_attr.port_num, > > + attr->ah_attr.grh.sgid_index, , _attr); > > + if (!rc && !memcmp(, , sizeof(gid))) > > + rc = -ENOENT; > > + > > + if (!rc && gid_attr.ndev) { > > + } > > + } > >

RE: [RFC 09/11] Add LL2 RoCE interface

> > + DP_ERR(cdev, > > + "QED RoCE set MAC filter failed - roce_info/ll2 NULL\n"); > > + return -EINVAL; > > + } > > + > > + p_ptt = qed_ptt_acquire(QED_LEADING_HWFN(cdev)); > > + if (!p_ptt) { > > + DP_ERR(cdev, > > + "qed roce

RE: [RFC 00/11] QLogic RDMA Driver (qedr) RFC

> > What do you mean by "standard kernel names"? > > By that I mean 'identical copies' do not copy the file and then randomly > change > it giving things different names or putting different content in structs. > > You will want to submit your user provider to rdma-plumbing to get it into the >

RE: [RFC 00/11] QLogic RDMA Driver (qedr) RFC

> Anything that is used with copy_to/from_user, ib_copy_to/from_udata, > etc, etc must be in a include/uapi header. > > Any constant you might want to copy into your userspace provider must > be in include/uapi. > I understand you mean something like

RE: [RFC 08/11] Add support for data path

> > + pbe = (struct regpair *)pbl_table->va; > > + num_pbes = 0; > > + > > + for (i = 0; i < mr->npages && > > +(total_num_pbes != mr->info.pbl_info.num_pbes); i++) { > > + u64 buf_addr = mr->pages[i]; > > + > > + pbe->lo = cpu_to_le32((u32)buf_addr); > > +

RE: [RFC 03/11] Add support for RoCE HW init

> > + dev->max_sge = min_t(u32, RDMA_MAX_SGE_PER_SQ_WQE, > > +RDMA_MAX_SGE_PER_RQ_WQE); > > Our kernel target mode consumers sort of rely on max_sge_rd, you need to > make sure to set it too. Good catch. Thanks!

RE: [RFC 02/11] Add RoCE driver framework

> > + if ((event != NETDEV_CHANGENAME) && (event != > > NETDEV_CHANGEADDR)) > > nit: You don't really need the extra parens here. > Sure, thanks. Will remove.

RE: [RFC 07/11] Add support for memory registeration verbs

> > +static inline struct qedr_ah *get_qedr_ah(struct ib_ah *ibah) { > > + return container_of(ibah, struct qedr_ah, ibah); } > > Little surprising to find that here... how is the ah related to this patch? Thanks, Sagi. Will move into a proper location.