| Message ID | 20240221092728.1281499-9-davidgow@google.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | kunit: Fix printf format specifier issues in KUnit assertions | expand |
On Wed, Feb 21, 2024 at 05:27:21PM +0800, David Gow wrote: > KUNIT_FAIL() is used to fail the xe_migrate test when an error occurs. > However, there's a mismatch in the format specifier: '%li' is used to > log 'err', which is an 'int'. > > Use '%i' instead of '%li', and for the case where we're printing an > error pointer, just use '%pe', instead of extracting the error code > manually with PTR_ERR(). (This also results in a nicer output when the > error code is known.) > > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs") > Signed-off-by: David Gow <davidgow@google.com> Tested-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/gpu/drm/xe/tests/xe_migrate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c > index a6523df0f1d3..c347e2c29f81 100644 > --- a/drivers/gpu/drm/xe/tests/xe_migrate.c > +++ b/drivers/gpu/drm/xe/tests/xe_migrate.c > @@ -114,21 +114,21 @@ static void test_copy(struct xe_migrate *m, struct xe_bo *bo, > region | > XE_BO_NEEDS_CPU_ACCESS); > if (IS_ERR(remote)) { > - KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %li\n", > - str, PTR_ERR(remote)); > + KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %pe\n", > + str, remote); > return; > } > > err = xe_bo_validate(remote, NULL, false); > if (err) { > - KUNIT_FAIL(test, "Failed to validate system bo for %s: %li\n", > + KUNIT_FAIL(test, "Failed to validate system bo for %s: %i\n", > str, err); > goto out_unlock; > } > > err = xe_bo_vmap(remote); > if (err) { > - KUNIT_FAIL(test, "Failed to vmap system bo for %s: %li\n", > + KUNIT_FAIL(test, "Failed to vmap system bo for %s: %i\n", > str, err); > goto out_unlock; > } > -- > 2.44.0.rc0.258.g7320e95886-goog >
On Wed, Feb 21, 2024 at 05:27:21PM +0800, David Gow wrote: >KUNIT_FAIL() is used to fail the xe_migrate test when an error occurs. >However, there's a mismatch in the format specifier: '%li' is used to >log 'err', which is an 'int'. > >Use '%i' instead of '%li', and for the case where we're printing an >error pointer, just use '%pe', instead of extracting the error code >manually with PTR_ERR(). (This also results in a nicer output when the >error code is known.) > >Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs") >Signed-off-by: David Gow <davidgow@google.com> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> this has a potential to cause conflicts with upcoming work, so I think it's better to apply this through drm-xe-next. Let me know if you agree. thanks Lucas De Marchi >--- > drivers/gpu/drm/xe/tests/xe_migrate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > >diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c >index a6523df0f1d3..c347e2c29f81 100644 >--- a/drivers/gpu/drm/xe/tests/xe_migrate.c >+++ b/drivers/gpu/drm/xe/tests/xe_migrate.c >@@ -114,21 +114,21 @@ static void test_copy(struct xe_migrate *m, struct xe_bo *bo, > region | > XE_BO_NEEDS_CPU_ACCESS); > if (IS_ERR(remote)) { >- KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %li\n", >- str, PTR_ERR(remote)); >+ KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %pe\n", >+ str, remote); > return; > } > > err = xe_bo_validate(remote, NULL, false); > if (err) { >- KUNIT_FAIL(test, "Failed to validate system bo for %s: %li\n", >+ KUNIT_FAIL(test, "Failed to validate system bo for %s: %i\n", > str, err); > goto out_unlock; > } > > err = xe_bo_vmap(remote); > if (err) { >- KUNIT_FAIL(test, "Failed to vmap system bo for %s: %li\n", >+ KUNIT_FAIL(test, "Failed to vmap system bo for %s: %i\n", > str, err); > goto out_unlock; > } >-- >2.44.0.rc0.258.g7320e95886-goog >
On Wed, 21 Feb 2024 at 21:05, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > this has a potential to cause conflicts with upcoming work, so I think > it's better to apply this through drm-xe-next. Let me know if you agree. I disagree. Violently. For this to be fixed, we need to have the printf format checking enabled. And we can't enable it until all the problems have been fixed. Which means that we should *not* have to wait for [N] different trees to fix their issues separately. This should get fixed in the Kunit tree, so that the Kunit tree can just send a pull request to me to enable format checking for the KUnit tests, together with all the fixes. Trying to spread those fixes out to different git branches will only result in pain and pointless dependencies between different trees. Honestly, the reason I noticed the problem in the first place was that the drm tree had a separate bug, that had been apparently noted in linux-next, and *despite* that it made it into a pull request to me and caused new build failures in rc5. So as things are, I am not IN THE LEAST interested in some kind of "let us fix this in the drm tree separately" garbage. We're not making things worse by trying to fix this in different trees. We're fixing this in the Kunit tree, and I do not want to get *more* problems from the drm side. I've had enough. Linus
On Wed, 2024-02-21 at 17:27 +0800, David Gow wrote: > KUNIT_FAIL() is used to fail the xe_migrate test when an error > occurs. > However, there's a mismatch in the format specifier: '%li' is used to > log 'err', which is an 'int'. > > Use '%i' instead of '%li', and for the case where we're printing an > error pointer, just use '%pe', instead of extracting the error code > manually with PTR_ERR(). (This also results in a nicer output when > the > error code is known.) > > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel > GPUs") > Signed-off-by: David Gow <davidgow@google.com> Ack to merge through the Kunit tree. Acked-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c index a6523df0f1d3..c347e2c29f81 100644 --- a/drivers/gpu/drm/xe/tests/xe_migrate.c +++ b/drivers/gpu/drm/xe/tests/xe_migrate.c @@ -114,21 +114,21 @@ static void test_copy(struct xe_migrate *m, struct xe_bo *bo, region | XE_BO_NEEDS_CPU_ACCESS); if (IS_ERR(remote)) { - KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %li\n", - str, PTR_ERR(remote)); + KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %pe\n", + str, remote); return; } err = xe_bo_validate(remote, NULL, false); if (err) { - KUNIT_FAIL(test, "Failed to validate system bo for %s: %li\n", + KUNIT_FAIL(test, "Failed to validate system bo for %s: %i\n", str, err); goto out_unlock; } err = xe_bo_vmap(remote); if (err) { - KUNIT_FAIL(test, "Failed to vmap system bo for %s: %li\n", + KUNIT_FAIL(test, "Failed to vmap system bo for %s: %i\n", str, err); goto out_unlock; }
KUNIT_FAIL() is used to fail the xe_migrate test when an error occurs. However, there's a mismatch in the format specifier: '%li' is used to log 'err', which is an 'int'. Use '%i' instead of '%li', and for the case where we're printing an error pointer, just use '%pe', instead of extracting the error code manually with PTR_ERR(). (This also results in a nicer output when the error code is known.) Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs") Signed-off-by: David Gow <davidgow@google.com> --- drivers/gpu/drm/xe/tests/xe_migrate.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)