On Fri, 03 Dec 2021, Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> wrote:
> On Fri, 2021-12-03 at 13:02 +0200, Ville Syrjälä wrote:
>> On Fri, Dec 03, 2021 at 12:02:57PM +0530, Pallavi Mishra wrote:
>> > add null ptr checks to prevent crash/exceptions.
>>
>> BUG_ON()s aren't going to fix anything.
>>
>> > Signed-off-by: Pallavi Mishra <pallavi.mishra@xxxxxxxxx>
>
> Pallavi,
>
> The NULL pointer dereferences here are probably all false positives
> from a static analyzer. However the GEM_BUG_ONs are fine to assert that
> the assumption really holds and to clearly point out what's going wrong
> if they are hit in CI tests.
I think we're massively overusing GEM_BUG_ON() all over the place.
BR,
Jani.
>
> But the commit message must reflect that.
>
> /Thomas.
>
>
>> > ---
>> > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 +++
>> > drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 3 ++-
>> > 2 files changed, 5 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> > index 218a9b3037c7..997fe73c205b 100644
>> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> > @@ -906,6 +906,8 @@ vm_access_ttm(struct vm_area_struct *area,
>> > unsigned long addr,
>> > struct drm_i915_gem_object *obj =
>> > i915_ttm_to_gem(area->vm_private_data);
>> >
>> > + GEM_BUG_ON(!obj);
>> > +
>> > if (i915_gem_object_is_readonly(obj) && write)
>> > return -EACCES;
>> >
>> > @@ -966,6 +968,7 @@ static const struct drm_i915_gem_object_ops
>> > i915_gem_ttm_obj_ops = {
>> > void i915_ttm_bo_destroy(struct ttm_buffer_object *bo)
>> > {
>> > struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>> > + GEM_BUG_ON(!obj);
>> >
>> > i915_gem_object_release_memory_region(obj);
>> > mutex_destroy(&obj->ttm.get_io_page.lock);
>> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
>> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
>> > index 80df9f592407..2b684903a9f5 100644
>> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
>> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
>> > @@ -371,6 +371,7 @@ int i915_ttm_move_notify(struct
>> > ttm_buffer_object *bo)
>> > struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>> > int ret;
>> >
>> > + GEM_BUG_ON(!obj);
>> > ret = i915_gem_object_unbind(obj,
>> > I915_GEM_OBJECT_UNBIND_ACTIVE);
>> > if (ret)
>> > return ret;
>> > @@ -506,7 +507,7 @@ static void i915_ttm_memcpy_init(struct
>> > i915_ttm_memcpy_arg *arg,
>> >
>> > dst_reg = i915_ttm_region(bo->bdev, dst_mem->mem_type);
>> > src_reg = i915_ttm_region(bo->bdev, bo->resource-
>> > >mem_type);
>> > - GEM_BUG_ON(!dst_reg || !src_reg);
>> > + GEM_BUG_ON(!dst_reg || !src_reg || !obj);
>> >
>> > arg->dst_iter = !i915_ttm_cpu_maps_iomem(dst_mem) ?
>> > ttm_kmap_iter_tt_init(&arg->_dst_iter.tt, dst_ttm)
>> > :
>> > --
>> > 2.25.1
>>
>
>
--
Jani Nikula, Intel Open Source Graphics Center
