On Fri, May 26, 2023 at 02:58:01PM +0200, Andi Shyti wrote:
> Hi Dan,
>
> On Fri, May 26, 2023 at 02:59:31PM +0300, Dan Carpenter wrote:
> > Return the error code if i915_gem_object_create_internal() fails,
> > instead of returning success.
> >
> > Fixes: cf586021642d ("drm/i915/gt: Pipelined page migration")
> > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/selftest_migrate.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/selftest_migrate.c b/drivers/gpu/drm/i915/gt/selftest_migrate.c
> > index e677f2da093d..a26429fd5326 100644
> > --- a/drivers/gpu/drm/i915/gt/selftest_migrate.c
> > +++ b/drivers/gpu/drm/i915/gt/selftest_migrate.c
> > @@ -55,8 +55,10 @@ static int copy(struct intel_migrate *migrate,
> >
> > sz = src->base.size;
> > dst = i915_gem_object_create_internal(i915, sz);
> > - if (IS_ERR(dst))
> > + if (IS_ERR(dst)) {
> > + err = PTR_ERR(dst);
> > goto err_free_src;
> > + }
>
> I think it was intentional to return '0' when
> i915_gem_object_create_internal() failed, as we are not testing
> object creation here.
>
> I don't really mind this patch, but, on the other hand, returning
> an error value here might provide a biased information.
Something we could consider is to make it more obvious that it's
intentional. Smatch counts it as intentional if there is an "err = 0;"
within a few lines of the goto.
But let's just leave it. I've already marked this static checker
warning as dealt with. If I see it again and maybe that will motivate
me to add an err = 0; assignment. People imagine that kernel code must
be 100% perfect with no static checker warnings etc but really it's
almost the weekend and this is fine.
regards,
dan carpenter
