[Intel-gfx] [drm-intel:drm-intel-gt-next 8/14] drivers/gpu/drm/i915/gt/selftest_migrate.c:102 copy() error: uninitialized symbol 'rq'.
Matthew Auld
matthew.william.auld at gmail.com
Fri Jul 2 11:34:33 UTC 2021
On Fri, 2 Jul 2021 at 12:07, Dan Carpenter <dan.carpenter at oracle.com> wrote:
>
> On Fri, Jul 02, 2021 at 11:32:45AM +0100, Matthew Auld wrote:
> > On Fri, 2 Jul 2021 at 09:45, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> > >
> > > tree: git://anongit.freedesktop.org/drm-intel drm-intel-gt-next
> > > head: 5cd57f676bb946a00275408f0dd0d75dbc466d25
> > > commit: cf586021642d8017cde111b7dd1ba86224e9da51 [8/14] drm/i915/gt: Pipelined page migration
> > > config: x86_64-randconfig-m001-20210630 (attached as .config)
> > > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> > >
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <lkp at intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter at oracle.com>
> > >
> > > New smatch warnings:
> > > drivers/gpu/drm/i915/gt/selftest_migrate.c:102 copy() error: uninitialized symbol 'rq'.
> > > drivers/gpu/drm/i915/gt/selftest_migrate.c:113 copy() error: uninitialized symbol 'vaddr'.
> > >
> > > Old smatch warnings:
> > > drivers/gpu/drm/i915/gem/i915_gem_object.h:182 __i915_gem_object_lock() error: we previously assumed 'ww' could be null (see line 171)
> > >
> > > vim +/rq +102 drivers/gpu/drm/i915/gt/selftest_migrate.c
> > >
> > > cf586021642d80 Chris Wilson 2021-06-17 32 static int copy(struct intel_migrate *migrate,
> > > cf586021642d80 Chris Wilson 2021-06-17 33 int (*fn)(struct intel_migrate *migrate,
> > > cf586021642d80 Chris Wilson 2021-06-17 34 struct i915_gem_ww_ctx *ww,
> > > cf586021642d80 Chris Wilson 2021-06-17 35 struct drm_i915_gem_object *src,
> > > cf586021642d80 Chris Wilson 2021-06-17 36 struct drm_i915_gem_object *dst,
> > > cf586021642d80 Chris Wilson 2021-06-17 37 struct i915_request **out),
> > > cf586021642d80 Chris Wilson 2021-06-17 38 u32 sz, struct rnd_state *prng)
> > > cf586021642d80 Chris Wilson 2021-06-17 39 {
> > > cf586021642d80 Chris Wilson 2021-06-17 40 struct drm_i915_private *i915 = migrate->context->engine->i915;
> > > cf586021642d80 Chris Wilson 2021-06-17 41 struct drm_i915_gem_object *src, *dst;
> > > cf586021642d80 Chris Wilson 2021-06-17 42 struct i915_request *rq;
> > > cf586021642d80 Chris Wilson 2021-06-17 43 struct i915_gem_ww_ctx ww;
> > > cf586021642d80 Chris Wilson 2021-06-17 44 u32 *vaddr;
> > > cf586021642d80 Chris Wilson 2021-06-17 45 int err = 0;
> > >
> > > One way to silence these warnings would be to initialize err = -EINVAL.
> > > Then Smatch would know that we goto err_out for an empty list.
> > >
> > > cf586021642d80 Chris Wilson 2021-06-17 46 int i;
> > > cf586021642d80 Chris Wilson 2021-06-17 47
> > > cf586021642d80 Chris Wilson 2021-06-17 48 src = create_lmem_or_internal(i915, sz);
> > > cf586021642d80 Chris Wilson 2021-06-17 49 if (IS_ERR(src))
> > > cf586021642d80 Chris Wilson 2021-06-17 50 return 0;
> > > cf586021642d80 Chris Wilson 2021-06-17 51
> > > cf586021642d80 Chris Wilson 2021-06-17 52 dst = i915_gem_object_create_internal(i915, sz);
> > > cf586021642d80 Chris Wilson 2021-06-17 53 if (IS_ERR(dst))
> > > cf586021642d80 Chris Wilson 2021-06-17 54 goto err_free_src;
> > > cf586021642d80 Chris Wilson 2021-06-17 55
> > > cf586021642d80 Chris Wilson 2021-06-17 56 for_i915_gem_ww(&ww, err, true) {
> > > cf586021642d80 Chris Wilson 2021-06-17 57 err = i915_gem_object_lock(src, &ww);
> > > cf586021642d80 Chris Wilson 2021-06-17 58 if (err)
> > > cf586021642d80 Chris Wilson 2021-06-17 59 continue;
> > > cf586021642d80 Chris Wilson 2021-06-17 60
> > > cf586021642d80 Chris Wilson 2021-06-17 61 err = i915_gem_object_lock(dst, &ww);
> > > cf586021642d80 Chris Wilson 2021-06-17 62 if (err)
> > > cf586021642d80 Chris Wilson 2021-06-17 63 continue;
> > > cf586021642d80 Chris Wilson 2021-06-17 64
> > > cf586021642d80 Chris Wilson 2021-06-17 65 vaddr = i915_gem_object_pin_map(src, I915_MAP_WC);
> > > cf586021642d80 Chris Wilson 2021-06-17 66 if (IS_ERR(vaddr)) {
> > > cf586021642d80 Chris Wilson 2021-06-17 67 err = PTR_ERR(vaddr);
> > > cf586021642d80 Chris Wilson 2021-06-17 68 continue;
> > > cf586021642d80 Chris Wilson 2021-06-17 69 }
> > > cf586021642d80 Chris Wilson 2021-06-17 70
> > > cf586021642d80 Chris Wilson 2021-06-17 71 for (i = 0; i < sz / sizeof(u32); i++)
> > > cf586021642d80 Chris Wilson 2021-06-17 72 vaddr[i] = i;
> > > cf586021642d80 Chris Wilson 2021-06-17 73 i915_gem_object_flush_map(src);
> > > cf586021642d80 Chris Wilson 2021-06-17 74
> > > cf586021642d80 Chris Wilson 2021-06-17 75 vaddr = i915_gem_object_pin_map(dst, I915_MAP_WC);
> > > cf586021642d80 Chris Wilson 2021-06-17 76 if (IS_ERR(vaddr)) {
> > > cf586021642d80 Chris Wilson 2021-06-17 77 err = PTR_ERR(vaddr);
> > > cf586021642d80 Chris Wilson 2021-06-17 78 goto unpin_src;
> > > cf586021642d80 Chris Wilson 2021-06-17 79 }
> > > cf586021642d80 Chris Wilson 2021-06-17 80
> > > cf586021642d80 Chris Wilson 2021-06-17 81 for (i = 0; i < sz / sizeof(u32); i++)
> > > cf586021642d80 Chris Wilson 2021-06-17 82 vaddr[i] = ~i;
> > > cf586021642d80 Chris Wilson 2021-06-17 83 i915_gem_object_flush_map(dst);
> > > cf586021642d80 Chris Wilson 2021-06-17 84
> > > cf586021642d80 Chris Wilson 2021-06-17 85 err = fn(migrate, &ww, src, dst, &rq);
> > > cf586021642d80 Chris Wilson 2021-06-17 86 if (!err)
> > > cf586021642d80 Chris Wilson 2021-06-17 87 continue;
> > >
> > > Does fn() initialize "rq" on the success path? Anyway Smatch would
> > > complain anyway because it thinks the list could be empty or that we
> > > might hit and early continue for everything.
> >
> > The fn() will always first initialize the rq to NULL. If it returns
> > success then rq will always be a valid rq. If it returns an err then
> > the rq might be NULL, or a valid rq depending on how far the copy/fn
> > got.
> >
> > And for_i915_gem_ww() will always run at least once, since ww->loop =
> > true, so this looks like a false positive?
>
> You don't think i915_gem_object_lock(), i915_gem_object_pin_map() or
> i915_gem_object_pin_map() can fail?
Yeah, they can totally fail but then we mostly likely just hit the
err_out. The for_i915_gem_ww() is a little strange since it's not
really looping over anything, it's just about retrying the block if we
see -EDEADLK(which involves dropping some locks), if we see any other
error then the loop is terminated with ww->loop = false, which then
hits the goto err_out.
>
> regards,
> dan carpenter
>
More information about the Intel-gfx
mailing list