diff mbox series

[8/9] drm/xe/tests: Fix printf format specifiers in xe_migrate test

Message ID 20240221092728.1281499-9-davidgow@google.com (mailing list archive)
State Mainlined
Commit 689a930b93c5c20294df5da0407df361c5412eac
Headers show
Series kunit: Fix printf format specifier issues in KUnit assertions | expand

Commit Message

David Gow Feb. 21, 2024, 9:27 a.m. UTC
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(-)

Comments

Guenter Roeck Feb. 21, 2024, 1:26 p.m. UTC | #1
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
>
Lucas De Marchi Feb. 22, 2024, 5:05 a.m. UTC | #2
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
>
Linus Torvalds Feb. 22, 2024, 5:59 a.m. UTC | #3
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
Thomas Hellstrom Feb. 22, 2024, 9:52 a.m. UTC | #4
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 mbox series

Patch

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;
 	}