linux-kernel - Re: [PATCH v1 1/2] drm/amdgpu: make non-NULL out fence mandatory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8a5f0bc8-4d3a-4e47-902e-7527759d1494@amd.com>
Date: Tue, 16 Sep 2025 12:52:03 +0200
From: Christian König <christian.koenig@....com>
To: Pierre-Eric Pelloux-Prayer <pierre-eric@...sy.net>,
 Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@....com>,
 Alex Deucher <alexander.deucher@....com>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Sumit Semwal <sumit.semwal@...aro.org>
Cc: amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH v1 1/2] drm/amdgpu: make non-NULL out fence mandatory

On 16.09.25 11:46, Pierre-Eric Pelloux-Prayer wrote:
> 
> 
> Le 16/09/2025 à 11:25, Christian König a écrit :
>> On 16.09.25 09:08, Pierre-Eric Pelloux-Prayer wrote:
>>> amdgpu_ttm_copy_mem_to_mem has a single caller, make sure the out
>>> fence is non-NULL to simplify the code.
>>> Since none of the pointers should be NULL, we can enable
>>> __attribute__((nonnull))__.
>>>
>>> While at it make the function static since it's only used from
>>> amdgpuu_ttm.c.
>>>
>>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@....com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 17 ++++++++---------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  6 ------
>>>   2 files changed, 8 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 27ab4e754b2a..70b817b5578d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -284,12 +284,13 @@ static int amdgpu_ttm_map_buffer(struct ttm_buffer_object *bo,
>>>    * move and different for a BO to BO copy.
>>>    *
>>>    */
>>> -int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
>>> -                   const struct amdgpu_copy_mem *src,
>>> -                   const struct amdgpu_copy_mem *dst,
>>> -                   uint64_t size, bool tmz,
>>> -                   struct dma_resv *resv,
>>> -                   struct dma_fence **f)
>>> +__attribute__((nonnull))
>>
>> That looks fishy.
>>
>>> +static int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
>>> +                      const struct amdgpu_copy_mem *src,
>>> +                      const struct amdgpu_copy_mem *dst,
>>> +                      uint64_t size, bool tmz,
>>> +                      struct dma_resv *resv,
>>> +                      struct dma_fence **f)
>>
>> I'm not an expert for those, but looking at other examples that should be here and look something like:
>>
>> __attribute__((nonnull(7)))
> 
> Both syntax are valid. The GCC docs says:
> 
>    If no arg-index is given to the nonnull attribute, all pointer arguments are marked as non-null

Never seen that before. Is that gcc specifc or standardized?

> 
> 
>>
>> But I think for this case here it is also not a must have to have that.
> 
> I can remove it if you prefer, but it doesn't hurt to have the compiler validate usage of the functions.

Yeah it's clearly useful, but I'm worried that clang won't like it.

Christian.

> 
> Pierre-Eric
> 
> 
>>
>> Regards,
>> Christian.
>>
>>>   {
>>>       struct amdgpu_ring *ring = adev->mman.buffer_funcs_ring;
>>>       struct amdgpu_res_cursor src_mm, dst_mm;
>>> @@ -363,9 +364,7 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
>>>       }
>>>   error:
>>>       mutex_unlock(&adev->mman.gtt_window_lock);
>>> -    if (f)
>>> -        *f = dma_fence_get(fence);
>>> -    dma_fence_put(fence);
>>> +    *f = fence;
>>>       return r;
>>>   }
>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> index bb17987f0447..07ae2853c77c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>> @@ -170,12 +170,6 @@ int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t src_offset,
>>>                  struct dma_resv *resv,
>>>                  struct dma_fence **fence, bool direct_submit,
>>>                  bool vm_needs_flush, uint32_t copy_flags);
>>> -int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
>>> -                   const struct amdgpu_copy_mem *src,
>>> -                   const struct amdgpu_copy_mem *dst,
>>> -                   uint64_t size, bool tmz,
>>> -                   struct dma_resv *resv,
>>> -                   struct dma_fence **f);
>>>   int amdgpu_ttm_clear_buffer(struct amdgpu_bo *bo,
>>>                   struct dma_resv *resv,
>>>                   struct dma_fence **fence);