Re: [PATCH v2 8/8] drm/i915/selftests: Use drm_* prints in selftest headers

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Invalid pointer cast in `__i915_subtests()` causes type confusion and kernel panics when tests pass non-i915 context objects.
- [Low] The `run_selftests` macro calls `__run_selftests` with 5 arguments, but the function signature expects 4 arguments.
- [Low] Conflicting function signatures for `__i915_subtests` between the header and source file.
- [Low] Passing `NULL` to `drm_info()` mangles log formatting in mock selftests and destroys `igt_hexdump()` alignment.
--

--- Patch [8]: [PATCH v2 8/8] drm/i915/selftests: Use drm_* prints in selftest headers ---
commit a417b9a673aa26bae15031a5fc6e3dc6e4262012
Author: Krzysztof Karas <[email protected]>
Subject: drm/i915/selftests: Use drm_* prints in selftest headers

This commit moves away from generic pr_ prints to device context aware drm
versions in the i915 selftests.

> diff --git a/drivers/gpu/drm/i915/i915_selftest.h b/drivers/gpu/drm/i915/i915_selftest.h
> index e29ca298e7eb1..66707d7b35d31 100644
> --- a/drivers/gpu/drm/i915/i915_selftest.h
> +++ b/drivers/gpu/drm/i915/i915_selftest.h
> @@ -84,6 +85,7 @@ int __intel_gt_live_setup(void *data);
>  int __intel_gt_live_teardown(int err, void *data);
>  
>  int __i915_subtests(const char *caller,
> +		    const struct drm_device *drm,
>  		    int (*setup)(void *data),

[Severity: Low]
Does this introduce a conflicting function signature?

The header file defines __i915_subtests() with 7 arguments here, but the source
file defines it with 6 arguments without the new drm parameter, which will
result in a conflicting types error during compilation.

> diff --git a/drivers/gpu/drm/i915/selftests/i915_selftest.c b/drivers/gpu/drm/i915/selftests/i915_selftest.c
> index 3adcfdab75ae1..6550b05e76360 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_selftest.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_selftest.c
> @@ -41,13 +43,13 @@ struct i915_selftest i915_selftest __read_mostly = {
>  
>  int i915_mock_sanitycheck(void)
>  {
> -	pr_info(DRIVER_NAME ": %s() - ok!\n", __func__);
> +	drm_info(NULL, "%s() - ok!\n", __func__);

[Severity: Low]
Will passing NULL to drm_info() mangle the log formatting?

drm_info(NULL, ...) resolves to dev_info(NULL, ...), which prepends
"(NULL device *): [drm] " to the logs. This replaces the clean logs from the
previous pr_info() calls.

[ ... ]

> @@ -224,6 +226,8 @@ static int __run_selftests(const char *name,
>  			   unsigned int count,
>  			   void *data)
>  {
> +	struct drm_i915_private *i915 = data;
> +	struct drm_device *drm = i915 ? &i915->drm : NULL;
>  	struct mm_struct *mm = NULL;

[ ... ]

> @@ -299,8 +303,8 @@ static int __run_selftests(const char *name,
>  	return err;
>  }
>  
> -#define run_selftests(x, data) \
> -	__run_selftests(#x, x##_selftests, ARRAY_SIZE(x##_selftests), data)
> +#define run_selftests(x, drm, data) \
> +	__run_selftests(#x, drm, x##_selftests, ARRAY_SIZE(x##_selftests), data)

[Severity: Low]
Is the run_selftests macro passing the correct number of arguments?

The run_selftests macro was updated to pass 5 arguments including the new drm
parameter, but the static function __run_selftests() was not modified to accept
it and still expects 4 arguments. This causes a build failure.

[ ... ]

> @@ -495,6 +499,8 @@ int __i915_subtests(const char *caller,
>  		    unsigned int count,
>  		    void *data)
>  {
> +	struct drm_i915_private *i915 = data;
> +	struct drm_device *drm = i915 ? &i915->drm : NULL;

[Severity: High]
Can this cast cause a kernel panic when testing non-i915 context objects?

This blindly casts the void *data argument to struct drm_i915_private * to
derive a drm pointer. However, callers often pass other types like
struct intel_gt *.

For intel_gt, offset 8 contains a string pointer (const char *name), which
perfectly aliases with drm->dev. When drm_info(drm, ...) passes this to
dev_info(), the kernel will attempt to dereference the string literal as a
struct device *, resulting in a fatal page fault.

[ ... ]

> @@ -557,7 +561,7 @@ void igt_hexdump(const void *buf, size_t len)
>  
>  		if (prev && !memcmp(prev, buf + pos, rowsize)) {
>  			if (!skip) {
> -				pr_info("*\n");
> +				drm_info(NULL, "*\n");
>  				skip = true;
>  			}
>  			continue;
> @@ -567,7 +571,7 @@ void igt_hexdump(const void *buf, size_t len)
>  						rowsize, sizeof(u32),
>  						line, sizeof(line),
>  						false) >= sizeof(line));
> -		pr_info("[%04zx] %s\n", pos, line);
> +		drm_info(NULL, "[%04zx] %s\n", pos, line);

[Severity: Low]
Does passing NULL here break the formatting of the hex dump output?

Similar to the mock selftest above, dev_info() will prepend
"(NULL device *): [drm] " to the prints, which destroys the clean column
alignment expected in igt_hexdump() output.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8