Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

Yishai Hadas
2017-08-21T09:06:57Z

On 8/17/2017 9:03 PM, Jason Gunthorpe wrote:
> On Thu, Aug 17, 2017 at 08:39:32PM +0300, Leon Romanovsky wrote:
>> On Thu, Aug 17, 2017 at 11:33:20AM -0600, Jason Gunthorpe wrote:
>>> On Thu, Aug 17, 2017 at 04:36:38PM +0300, Yishai Hadas wrote:
>>>> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>>
>>>> The returned UAR pointer is actually void ** and points to whole
>>>> UAR database.
>>>>
>>>> Because user is not supposed to access it and expected to use
>>>> the CQ doorbell, we will return uar[0] (CQ doorbell) directly
>>>> and eliminate the following logic in the applications:
>>>>
>>>> uint64_t cq_db_reg = (uint64_t *)(((uint64_t)(uint64_t *)rxq->cq_uar) +
>>>> MLX5_CQ_DOORBELL;
>>>
>>> NAK, at least, as is..
>>>
>>> This changes the ABI and mlx5dv is a public interface now, so you have
>>> to do it properly.
>>>
>>> Looking at it, my advice is to rev the symbol version of
>>> mlx5dv_init_obj and consider providing a compat symbol.
>>>
>>> Also, you need to change the name of the 'uar' field:
>>>
>>>> - cq_out->uar = mctx->uar;
>>>> + cq_out->uar = mctx->uar[0];
>>>
>>> To something else, to make it clear, which of the two ABIs the
>>> application code is coded to - for a change like this, you *MUST*
>>> force old apps to have a compile failure.
>>
>> I know exactly who coded on top of this interface. They are the ones who
>> asked for this change and they are fully understand the implications,
>> inability to work with this interface on "old" libraries.
>
> I don't care who they are - you need to follow sane ABI rules if you
> are pushing public shared libraries into distros. This is not negotiable.
>

There is no ABI/API change but just a bug fix in mlx5 driver.
The API talked about void* but by mistake the mlx5 code returned void**,
this patch fixes that.

We don't care about potential break for direct mlx5 applications in this
arm_cq flow as code was buggy and couldn't be used properly.

Patch was merged.