Re: drivers/gpu/drm/vc4/vc4_gem.c:604 vc4_lock_bo_reservations() error: uninitialized symbol 'ret'.

[Christian König]

Hi Maira,

Am 09.04.25 um 21:49 schrieb Maíra Canal:
> + König
>
> Hi Dan,
>
> On 02/04/25 05:43, Dan Carpenter wrote:
>> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>> head:   acc4d5ff0b61eb1715c498b6536c38c1feb7f3c1
>> commit: 04630796c437a9285643097825cbd3cd06603f47 drm/vc4: Use DRM Execution Contexts
>> date:   2 months ago
>> config: arm64-randconfig-r073-20250402 (https://download.01.org/0day-ci/archive/20250402/202504021500.3AM1hKKS-lkp@xxxxxxxxx/config)
>> compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@xxxxxxxxx>
>> | Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>> | Closes: https://lore.kernel.org/r/202504021500.3AM1hKKS-lkp@xxxxxxxxx/
>>
>> smatch warnings:
>> drivers/gpu/drm/vc4/vc4_gem.c:604 vc4_lock_bo_reservations() error: uninitialized symbol 'ret'.
>>
>> vim +/ret +604 drivers/gpu/drm/vc4/vc4_gem.c
>>
>> cdec4d3613230f Eric Anholt 2017-04-12  589  static int
>> 04630796c437a9 Maíra Canal 2024-12-20  590  vc4_lock_bo_reservations(struct vc4_exec_info *exec,
>> 04630796c437a9 Maíra Canal 2024-12-20  591               struct drm_exec *exec_ctx)
>> cdec4d3613230f Eric Anholt 2017-04-12  592  {
>> 04630796c437a9 Maíra Canal 2024-12-20  593      int ret;
>> cdec4d3613230f Eric Anholt 2017-04-12  594
>> cdec4d3613230f Eric Anholt 2017-04-12  595      /* Reserve space for our shared (read-only) fence references,
>> cdec4d3613230f Eric Anholt 2017-04-12  596       * before we commit the CL to the hardware.
>> cdec4d3613230f Eric Anholt 2017-04-12  597       */
>> 04630796c437a9 Maíra Canal 2024-12-20  598      drm_exec_init(exec_ctx, DRM_EXEC_INTERRUPTIBLE_WAIT, exec->bo_count);
>> 04630796c437a9 Maíra Canal 2024-12-20  599      drm_exec_until_all_locked(exec_ctx) {
>> 04630796c437a9 Maíra Canal 2024-12-20  600          ret = drm_exec_prepare_array(exec_ctx, exec->bo,
>> 04630796c437a9 Maíra Canal 2024-12-20  601                           exec->bo_count, 1);
>>
>> This is a false positive in Smatch.  I can silence the warning on my
>> end easily enough to say that we always enter the drm_exec_until_all_locked()
>> loop.  But the question is why do we only test the last "ret" instead of
>> testing all of them?
>
> AFAIU `drm_exec_until_all_locked` will loop until all GEM objects are
> locked and no more contention exists. As we have a single operation
> inside the loop, we don't need to check "ret" for every iteration.
>
> I believe Christian will possibly give you a more precise answer as he
> designed the API.

Yeah that explanation is absolutely correct.

The drm_exec_until_all_locked() helper loops until all contention is resolved and all buffer locked.

You could avoid the snatch warning if you move the error handling into the loop, e.g. something like this here:

drm_exec_until_all_locked(exec_ctx) {
    ret = drm_exec_prepare_array(exec_ctx, exec->bo, exec->bo_count, 1);
    drm_exec_continue_on_contention(exec_ctx);
    if (ret) {
        drm_exec_fini(exec_ctx);
        return ret;
    }
}

Regards,
Christian.

>
> Best Regards,
> - Maíra
>
>>
>> 04630796c437a9 Maíra Canal 2024-12-20  602      }
>> cdec4d3613230f Eric Anholt 2017-04-12  603
>> cdec4d3613230f Eric Anholt 2017-04-12 @604      if (ret) {
>> 04630796c437a9 Maíra Canal 2024-12-20  605          drm_exec_fini(exec_ctx);
>> cdec4d3613230f Eric Anholt 2017-04-12  606          return ret;
>> 7edabee06a5622 Eric Anholt 2016-09-27  607      }
>> d5b1a78a772f1e Eric Anholt 2015-11-30  608
>> cdec4d3613230f Eric Anholt 2017-04-12  609      return 0;
>> cdec4d3613230f Eric Anholt 2017-04-12  610  }
>>
>