[PATCH] drm/i915/selftests: fix an error code in copy()

[PATCH] drm/i915/selftests: fix an error code in copy()

[Dan Carpenter]

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@linaro.org>
---
 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;
+	}
 
 	for_i915_gem_ww(&ww, err, true) {
 		err = i915_gem_object_lock(src, &ww);
-- 
2.39.2

Re: [PATCH] drm/i915/selftests: fix an error code in copy()

[Andi Shyti]

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@linaro.org>
> ---
>  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.

Thanks,
Andi

>  
>  	for_i915_gem_ww(&ww, err, true) {
>  		err = i915_gem_object_lock(src, &ww);
> -- 
> 2.39.2

Re: [PATCH] drm/i915/selftests: fix an error code in copy()

[Dan Carpenter]

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@linaro.org>
> > ---
> >  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

Re: [PATCH] drm/i915/selftests: fix an error code in copy()

[Andi Shyti]

Hi Dan,

On Fri, May 26, 2023 at 04:14:25PM +0300, Dan Carpenter wrote:
> 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@linaro.org>
> > > ---
> > >  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.

yes, I can accept an explicit "err = 0" with a comment on it. I
think it totally makes sense.

Do you want to do it or shall I take care of it?

Thanks,
Andi