On 2018/3/15 11:30, Jason Gunthorpe wrote: > 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 Okay, I will. -- 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