| Message ID | f6b876f1-4dd7-4d96-bee5-966817cc1644@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/selftests: fix an error code in copy() | expand |
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
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
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
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);
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(-)