> 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
> QED_RDMA_DEV_CAP_ATOMIC_OP based on PC
> 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
> 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.
>
The
> 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 changed
>
> Do you have a git tree?
>
We don't have a publicly accessible git tree.
>
> The module parameters is no-go.
>
We'll remove it.
> 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 alr
> > 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 t
> 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.
> uapi headers need the __ varients and the #include
>
> Follow what Leon has done.
OK
> > > + 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 th
> > +
> > + rc = ib_get_cached_gid(ibqp->device, attr->ah_attr.port_num,
> > + attr->ah_attr.grh.sgid_index, &gid, &gid_attr);
> > + if (!rc && !memcmp(&gid, &zgid, sizeof(gid)))
> > + rc = -ENOENT;
> > +
> > + if (!rc && gid_attr.ndev) {
> > +
> > + 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
> > 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
>
> 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
https://www.spinics.net/lists/linux-rdma/ms
> > + 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);
> > +
> > + 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!
> > + if ((event != NETDEV_CHANGENAME) && (event !=
> > NETDEV_CHANGEADDR))
>
> nit: You don't really need the extra parens here.
>
Sure, thanks. Will remove.
> > +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.
19 matches
Mail list logo
