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
