Re: [Linaro-mm-sig] [PATCH v3 6/9] dma-buf: Move dma-buf attachment to dynamic locking specification

[Dmitry Osipenko]

On 8/24/22 20:45, Christian König wrote:
> Am 24.08.22 um 17:49 schrieb Dmitry Osipenko:
>> On 8/24/22 18:24, Christian König wrote:
>>> Am 24.08.22 um 12:22 schrieb Dmitry Osipenko:
>>>> Move dma-buf attachment API functions to the dynamic locking
>>>> specification.
>>>> The strict locking convention prevents deadlock situations for dma-buf
>>>> importers and exporters.
>>>>
>>>> Previously, the "unlocked" versions of the attachment API functions
>>>> weren't taking the reservation lock and this patch makes them to take
>>>> the lock.
>>>>
>>>> Intel and AMD GPU drivers already were mapping the attached dma-bufs
>>>> under
>>>> the held lock during attachment, hence these drivers are updated to use
>>>> the locked functions.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
>>>> ---
>>>>    drivers/dma-buf/dma-buf.c                  | 115
>>>> ++++++++++++++-------
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    |   4 +-
>>>>    drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |   8 +-
>>>>    drivers/gpu/drm/i915/gem/i915_gem_object.c |  12 +++
>>>>    include/linux/dma-buf.h                    |  20 ++--
>>>>    5 files changed, 110 insertions(+), 49 deletions(-)
>>>>
>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>>> index 4556a12bd741..f2a5a122da4a 100644
>>>> --- a/drivers/dma-buf/dma-buf.c
>>>> +++ b/drivers/dma-buf/dma-buf.c
>>>> @@ -559,7 +559,7 @@ static struct file *dma_buf_getfile(struct dma_buf
>>>> *dmabuf, int flags)
>>>>     * 2. Userspace passes this file-descriptors to all drivers it wants
>>>> this buffer
>>>>     *    to share with: First the file descriptor is converted to a
>>>> &dma_buf using
>>>>     *    dma_buf_get(). Then the buffer is attached to the device using
>>>> - *    dma_buf_attach().
>>>> + *    dma_buf_attach_unlocked().
>>> Now I get why this is confusing me so much.
>>>
>>> The _unlocked postfix implies that there is another function which
>>> should be called with the locks already held, but this is not the case
>>> for attach/detach (because they always need to grab the lock
>>> themselves).
>> That's correct. The attach/detach ops of exporter can take the lock
>> (like i915 exporter does it), hence importer must not grab the lock
>> around dma_buf_attach() invocation.
>>
>>> So I suggest to drop the _unlocked postfix for the attach/detach
>>> functions. Another step would then be to unify attach/detach with
>>> dynamic_attach/dynamic_detach when both have the same locking convention
>>> anyway.
>> It's not a problem to change the name, but it's unclear to me why we
>> should do it. The _unlocked postfix tells importer that reservation must
>> be unlocked and it must be unlocked in case of dma_buf_attach().
>>
>> Dropping the postfix will make dma_buf_attach() inconsistent with the
>> rest of the _unlocked functions(?). Are you sure we need to rename it?
> 
> The idea of the postfix was to distinguish between two different
> versions of the same function, e.g. dma_buf_vmap_unlocked() vs normal
> dma_buf_vmap().
> 
> When we don't have those two types of the same function I don't think it
> makes to much sense to keep that. We should just properly document which
> functions expect what and that's what your documentation patch does.

Thank you for the clarification. I'll change the names in v4 like you're
suggesting, we can always improve naming later on if will be necessary.

-- 
Best regards,
Dmitry