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
