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

[Maíra Canal]

Hi Dan,

On 10/04/25 06:57, Dan Carpenter wrote:
> On Thu, Apr 10, 2025 at 11:27:43AM +0200, Christian König wrote:
> > 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;
> >      }
> > }
>
> Don't worry about silencing it.  I already silenced it in Smatch a couple
> days ago.

Thanks for silencing the warning!

Best Regards,
- Maíra

> regards,
> dan carpenter