On Thu, Mar 15, 2018 at 11:02:35AM +0800, Liuyixian (Eason) wrote:
>
>
> On 2018/3/15 4:30, Jason Gunthorpe wrote:
> > On Mon, Mar 05, 2018 at 09:53:05PM +0800, Yixian Liu wrote:
> >> diff --git a/providers/hns/hns_roce_u_abi.h b/providers/hns/hns_roce_u_abi.h
> >> index ec145bb..c4f36ec 100644
> >> +++ b/providers/hns/hns_roce_u_abi.h
> >> @@ -57,6 +57,7 @@ struct hns_roce_create_cq_resp {
> >> struct ib_uverbs_create_cq_resp ibv_resp;
> >> __u32 cqn;
> >> __u32 reserved;
> >> + __u64 cap_flags;
> >> };
> >
> > This struct is wrong.. the kernel does this:
> >
> > if (context) {
> > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) &&
> > (udata->outlen == sizeof(resp))) {
> > hr_cq->db_en = 1;
> > resp.cqn = hr_cq->cqn;
> > resp.cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB;
> > ret = ib_copy_to_udata(udata, &resp, sizeof(resp));
> > } else
> > ret = ib_copy_to_udata(udata, &hr_cq->cqn, sizeof(u64));
> >
> > So 'cqn' should be a u64, it is a mistake in the userspace to have
> > used a u32 here.
> >
> > You could correct it, but it would need to be used as u32
> > consistently.
> >
> > Instead I recommend changing the above struct (in user and kernel) to
> > simply:
> >
> > struct hns_roce_ib_create_cq_resp {
> > __u64 cqn; /* Only 32 bits used, 64 for compat */
> > __u64 cap_flags;
> > };
> >
> > And change the kernel to simply do:
> >
> > if (context) {
> > resp.cqn = hr_cq->cqn;
> > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) &&
> > (udata->outlen == sizeof(resp))) {
> > hr_cq->db_en = 1;
> > resp.cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB;
> > }
> > ret = ib_copy_to_udata(udata, &resp, sizeof(resp));
> >
> > ib_copy_to_udata() will do the right thing.
> >
> > Please send a patch for this immediately before this gets into a
> > released kernel..
> >
> > Also all these tests:
> >
> > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) &&
> > (udata->outlen == sizeof(resp)))
> >
> > Should probably be:
> >
> > if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) &&
> > (udata->outlen >= sizeof(resp)))
> >
> > Arguably a flag should have been added to the request for this new
> > behavior though, as is will limit what you can do in future.
> >
> > Jason
>
> Thank you very much! I will send the patch sets for kernel and userspace today.
Also make sure resp is zeroed in the kernel...
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
