Re: [Intel-gfx] [PATCH] drm/i915: Disable -Wuninitialized for intel_breadcrumbs.o
On Thu, Oct 25, 2018 at 12:36:01PM -0700, Nathan Chancellor wrote: > This warning is disabled by default in scripts/Makefile.extrawarn when > W= is not provided but this Makefile adds -Wall after this warning is > disabled so it shows up in the build when it shouldn't: > > In file included from drivers/gpu/drm/i915/intel_breadcrumbs.c:895: > drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c:350:34: error: > variable 'wq' is uninitialized when used within its own initialization > [-Werror,-Wuninitialized] > DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq); > ^~ > ./include/linux/wait.h:74:63: note: expanded from macro > 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK' > struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name) > ^~~~ > ./include/linux/wait.h:72:33: note: expanded from macro > '__WAIT_QUEUE_HEAD_INIT_ONSTACK' > ({ init_waitqueue_head(&name); name; }) >^~~~ > 1 error generated. > > This warning looks to be a false positive given that init_waitqueue_head > initializes name before it is used. Rather than disable the warning for > the full folder like commit 46e2068081e9 ("drm/i915: Disable some extra > clang warnings"), just disable it for the one problematic file because > it could be a useful warning for other cases. > > Link: https://github.com/ClangBuiltLinux/linux/issues/220 > Signed-off-by: Nathan Chancellor > --- > drivers/gpu/drm/i915/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 1c2857f13ad4..f36c420afb99 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -26,6 +26,7 @@ subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror > > # Fine grained warnings disable > CFLAGS_i915_pci.o = $(call cc-disable-warning, override-init) > +CFLAGS_intel_breadcrumbs.o = $(call cc-disable-warning, uninitialized) > CFLAGS_intel_fbdev.o = $(call cc-disable-warning, override-init) > > subdir-ccflags-y += \ > -- > 2.19.1 > Gentle ping for review. Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Disable -Wuninitialized for intel_breadcrumbs.o
On Tue, Dec 18, 2018 at 11:53:06AM +, Chris Wilson wrote: > Quoting Nick Desaulniers (2018-10-25 23:20:58) > > On Thu, Oct 25, 2018 at 12:36 PM Nathan Chancellor > > wrote: > > > > > > This warning is disabled by default in scripts/Makefile.extrawarn when > > > W= is not provided but this Makefile adds -Wall after this warning is > > > disabled so it shows up in the build when it shouldn't: > > > > > > In file included from drivers/gpu/drm/i915/intel_breadcrumbs.c:895: > > > drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c:350:34: error: > > > variable 'wq' is uninitialized when used within its own initialization > > > [-Werror,-Wuninitialized] > > > DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq); > > > ^~ > > > ./include/linux/wait.h:74:63: note: expanded from macro > > > 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK' > > > struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name) > > > ^~~~ > > > ./include/linux/wait.h:72:33: note: expanded from macro > > > '__WAIT_QUEUE_HEAD_INIT_ONSTACK' > > > ({ init_waitqueue_head(&name); name; }) > > >^~~~ > > > 1 error generated. > > > > > > This warning looks to be a false positive given that init_waitqueue_head > > > initializes name before it is used. Rather than disable the warning for > > > the full folder like commit 46e2068081e9 ("drm/i915: Disable some extra > > > > cc author/reviewer of 46e2068081e9. > > > > I'm fine with the patch as is, unless others prefer to disable it for > > the whole subdir? We could be playing whack-a-mole in the future > > disabling this warning for other translation units. > Hi Chris, > Yes, exactly this since the warning is generated by a core header and a > fairly common pattern its use is not restricted to any single file. > (Will not all selftests similarly explode?) > Well, -Wuninitialized is turned off for the whole kernel unless W= is passed. So I suppose it should be turned back on for the whole folder but I noticed that the i915 Makefile purposefully turns all of the disabled warnings back on for heavier coverage so it makes some sense to just leave it off for one translation unit when it's just one translation unit that has the problem. That said, I'm more than happy to send a v2 turning it off for the whole folder if you think that best. > The other false-positive clang-6 gave was for local_clock_us(). > Presumably that one is fixed? > -Chris With this patch, I can build i915 using defconfig and allyesconfig without any warnings with tip-of-tree Clang. Thank you for the comments! Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Clang warning in drivers/gpu/drm/i915/i915_debugfs.c
Hi all, Commit e845f099f1c6 ("drm/i915/dsc: Add Per connector debugfs node for DSC support/enable") causes a Clang warning: drivers/gpu/drm/i915/i915_debugfs.c:4961:17: warning: address of array 'intel_dp->dsc_dpcd' will always evaluate to 'true' [-Wpointer-bool-conversion] if (intel_dp->dsc_dpcd) ~~ ~~^~~~ 1 warning generated. Did you mean to dereference it or should that print statement just always show? I normally would send a patch myself but since I'm not familiar with this code, I'd rather not shoot in the dark :) especially since it's for a trivial logging statement. Thanks, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro
When building the kernel with Clang with defconfig and CONFIG_64BIT disabled, vmlinux fails to link because of the BUILD_BUG in _print_param. ld: drivers/gpu/drm/i915/i915_params.o: in function `i915_params_dump': i915_params.c:(.text+0x56): undefined reference to `__compiletime_assert_191' This function is semantically invalid unless the code is first inlined then constant folded, which doesn't work for Clang because semantic analysis happens before optimization/inlining. Converting this function to a macro avoids this problem and allows Clang to properly remove the BUILD_BUG during optimization. The output of 'objdump -D' is identically before and after this change for GCC regardless of if CONFIG_64BIT is set and allows Clang to link the kernel successfully with or without CONFIG_64BIT set. Link: https://github.com/ClangBuiltLinux/linux/issues/191 Suggested-by: Nick Desaulniers Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/i915_params.c | 29 + 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 295e981e4a39..a0f20b9b6f2d 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -174,22 +174,19 @@ i915_param_named(enable_dpcd_backlight, bool, 0600, i915_param_named(enable_gvt, bool, 0400, "Enable support for Intel GVT-g graphics virtualization host support(default:false)"); -static __always_inline void _print_param(struct drm_printer *p, -const char *name, -const char *type, -const void *x) -{ - if (!__builtin_strcmp(type, "bool")) - drm_printf(p, "i915.%s=%s\n", name, yesno(*(const bool *)x)); - else if (!__builtin_strcmp(type, "int")) - drm_printf(p, "i915.%s=%d\n", name, *(const int *)x); - else if (!__builtin_strcmp(type, "unsigned int")) - drm_printf(p, "i915.%s=%u\n", name, *(const unsigned int *)x); - else if (!__builtin_strcmp(type, "char *")) - drm_printf(p, "i915.%s=%s\n", name, *(const char **)x); - else - BUILD_BUG(); -} +#define _print_param(p, name, type, x)\ +do { \ + if (!__builtin_strcmp(type, "bool")) \ + drm_printf(p, "i915.%s=%s\n", name, yesno(*(const bool *)x)); \ + else if (!__builtin_strcmp(type, "int")) \ + drm_printf(p, "i915.%s=%d\n", name, *(const int *)x); \ + else if (!__builtin_strcmp(type, "unsigned int")) \ + drm_printf(p, "i915.%s=%u\n", name, *(const unsigned int *)x); \ + else if (!__builtin_strcmp(type, "char *"))\ + drm_printf(p, "i915.%s=%s\n", name, *(const char **)x);\ + else \ + BUILD_BUG(); \ +} while (0) /** * i915_params_dump - dump i915 modparams -- 2.19.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Ensure _print_param() builds with Clang
On Tue, Oct 16, 2018 at 03:29:37PM +0300, Jani Nikula wrote: > When building the kernel with Clang with defconfig and CONFIG_64BIT > disabled, vmlinux fails to link because of the BUILD_BUG in > _print_param. > > ld: drivers/gpu/drm/i915/i915_params.o: in function `i915_params_dump': > i915_params.c:(.text+0x56): undefined reference to > `__compiletime_assert_191' > > This function is semantically invalid unless the code is first inlined > then constant folded, which doesn't work for Clang because semantic > analysis happens before optimization/inlining. > > [The above written by Nathan Chancellor ] > > Use WARN_ONCE() instead of BUILD_BUG() to avoid the problem. The > WARN_ONCE() should get optimized away unless there's a type that's not > handled by _print_param(). > > References: https://github.com/ClangBuiltLinux/linux/issues/191 > References: > 20181009171401.14980-1-natechancellor@gmail.com">http://mid.mail-archive.com/20181009171401.14980-1-natechancellor@gmail.com > Cc: Nick Desaulniers > Cc: Nathan Chancellor > Cc: Chris Wilson > Reported-by: Nick Desaulniers > Reported-by: Nathan Chancellor > Signed-off-by: Jani Nikula Tested-by: Nathan Chancellor Thanks for the quick turnaround and help getting this fixed, Jani! > --- > drivers/gpu/drm/i915/i915_params.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index bd6bd8879cab..8d71886b5f03 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -184,7 +184,8 @@ static __always_inline void _print_param(struct > drm_printer *p, > else if (!__builtin_strcmp(type, "char *")) > drm_printf(p, "i915.%s=%s\n", name, *(const char **)x); > else > - BUILD_BUG(); > + WARN_ONCE(1, "no printer defined for param type %s (i915.%s)\n", > + type, name); > } > > /** > -- > 2.11.0 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Ensure intel_engine_init_execlist() builds with Clang
On Tue, Oct 16, 2018 at 03:29:38PM +0300, Jani Nikula wrote: > Clang build with UBSAN enabled leads to the following build error: > > drivers/gpu/drm/i915/intel_engine_cs.o: In function > `intel_engine_init_execlist': > drivers/gpu/drm/i915/intel_engine_cs.c:411: undefined reference to > `__compiletime_assert_411' > > Again, for this to work the code would first need to be inlined and then > constant folded, which doesn't work for Clang because semantic analysis > happens before optimization/inlining. > > Use GEM_BUG_ON() instead of BUILD_BUG_ON(). > > v2: Use is_power_of_2() from log2.h (Chris) > > References: > 20181015203410.155997-1-swboyd@chromium.org">http://mid.mail-archive.com/20181015203410.155997-1-swboyd@chromium.org > Reported-by: Stephen Boyd > Cc: Stephen Boyd > Cc: Chris Wilson > Signed-off-by: Jani Nikula > --- Tested-by: Nathan Chancellor > drivers/gpu/drm/i915/intel_engine_cs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c > b/drivers/gpu/drm/i915/intel_engine_cs.c > index f27dbe26bcc1..bc793b0c8806 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -466,7 +466,7 @@ static void intel_engine_init_execlist(struct > intel_engine_cs *engine) > struct intel_engine_execlists * const execlists = &engine->execlists; > > execlists->port_mask = 1; > - BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists)); > + GEM_BUG_ON(!is_power_of_2(execlists_num_ports(execlists))); > GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS); > > execlists->queue_priority = INT_MIN; > -- > 2.11.0 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Disable -Wuninitialized for intel_breadcrumbs.o
This warning is disabled by default in scripts/Makefile.extrawarn when W= is not provided but this Makefile adds -Wall after this warning is disabled so it shows up in the build when it shouldn't: In file included from drivers/gpu/drm/i915/intel_breadcrumbs.c:895: drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c:350:34: error: variable 'wq' is uninitialized when used within its own initialization [-Werror,-Wuninitialized] DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq); ^~ ./include/linux/wait.h:74:63: note: expanded from macro 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK' struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name) ^~~~ ./include/linux/wait.h:72:33: note: expanded from macro '__WAIT_QUEUE_HEAD_INIT_ONSTACK' ({ init_waitqueue_head(&name); name; }) ^~~~ 1 error generated. This warning looks to be a false positive given that init_waitqueue_head initializes name before it is used. Rather than disable the warning for the full folder like commit 46e2068081e9 ("drm/i915: Disable some extra clang warnings"), just disable it for the one problematic file because it could be a useful warning for other cases. Link: https://github.com/ClangBuiltLinux/linux/issues/220 Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 1c2857f13ad4..f36c420afb99 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -26,6 +26,7 @@ subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror # Fine grained warnings disable CFLAGS_i915_pci.o = $(call cc-disable-warning, override-init) +CFLAGS_intel_breadcrumbs.o = $(call cc-disable-warning, uninitialized) CFLAGS_intel_fbdev.o = $(call cc-disable-warning, override-init) subdir-ccflags-y += \ -- 2.19.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH v1 0/2] drm/i915/fence: A couple of build fixes
On Thu, Aug 29, 2024 at 09:10:41PM +0300, Andy Shevchenko wrote: > On Thu, Aug 29, 2024 at 07:53:25PM +0300, Andy Shevchenko wrote: > > On Thu, Aug 29, 2024 at 07:38:08PM +0300, Jani Nikula wrote: > > > On Thu, 29 Aug 2024, Andy Shevchenko > > > wrote: > > > > With CONFIG_WERROR=y and `make W=1` build fails on my x86_64 machine. > > > > This is due to some unused functions. Hence these quick fixes. > > > > > > Since when have we been getting the warnings for static inlines? Since commit 6863f5643dd7 ("kbuild: allow Clang to find unused static inline functions for W=1 build"). clang warns about unused static inline functions in .c files, unlike GCC (they both do not warn for functions coming from .h files). This difference is worked around for the normal build by adding '__maybe_unused' to the definition of 'inline' but Masahiro wanted to disable it for W=1 to allow this difference to find unused/dead code. There have not been too many complaints as far as I am aware but I can see how it is surprising. Cheers, Nathan
Re: [PATCH v1 0/2] drm/i915/fence: A couple of build fixes
On Thu, Aug 29, 2024 at 09:37:34PM +0300, Jani Nikula wrote: > On Thu, 29 Aug 2024, Nathan Chancellor wrote: > > Since commit 6863f5643dd7 ("kbuild: allow Clang to find unused static > > inline functions for W=1 build"). clang warns about unused static inline > > functions in .c files, unlike GCC (they both do not warn for functions > > coming from .h files). This difference is worked around for the normal > > build by adding '__maybe_unused' to the definition of 'inline' but > > Masahiro wanted to disable it for W=1 to allow this difference to find > > unused/dead code. There have not been too many complaints as far as I am > > aware but I can see how it is surprising. > > Heh, I was just going to reply citing the same commit. > > I occasionally build with clang myself, and we do enable most W=1 by > default in the drm subsystem, so I was wondering why I hadn't hit > this. The crucial difference is that we lack -DKBUILD_EXTRA_WARN1 which > W=1 adds. > > I see there's no subdir-cppflags-y, but I don't see any harm in us > adding -Wundef and -DKBUILD_EXTRA_WARN1 to subdir-ccflags-y. After we > fix the fallout, of course. Do you? No, that seems entirely reasonable when your goal is to enable W=1 for your subsystem. > I don't much like the __maybe_unused stuff, but I guess it's fine as a > stopgap measure, and then we can grep for that when running out of > things to do. :p Perhaps worth a TODO or something? Maybe a kernel newbie could work on that at some point if it is not high enough priority. Cheers, Nathan
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/fbc: Register per-crtc debugfs files
Hi Ville, On Mon, Dec 13, 2021 at 05:14:35PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > Expose FBC debugfs files for each crtc. These may or may not point > to the same FBC instance depending on the platform. > > We leave the old global debugfs files in place until > igt catches up to the new per-crtc approach. > > v2: Take a trip via intel_crtc_debugfs_add() (Jani) > > Cc: Jani Nikula > Signed-off-by: Ville Syrjälä Apologies if this has already been reported and fixed, I have not seen anything on lore.kernel.org or drm-intel about it. This patch as commit e74c6aa955ca ("drm/i915/fbc: Register per-crtc debugfs files") breaks the build when CONFIG_DEBUG_FS is disabled. drivers/gpu/drm/i915/display/intel_fbc.c: In function ‘intel_fbc_crtc_debugfs_add’: drivers/gpu/drm/i915/display/intel_fbc.c:1817:61: error: ‘struct drm_crtc’ has no member named ‘debugfs_entry’ 1817 | intel_fbc_debugfs_add(plane->fbc, crtc->base.debugfs_entry); | ^ Cheers, Nathan
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/fbc: Register per-crtc debugfs files
On Tue, Dec 21, 2021 at 06:05:34PM +0200, Ville Syrjälä wrote: > On Sat, Dec 18, 2021 at 06:00:47PM -0700, Nathan Chancellor wrote: > > Hi Ville, > > > > On Mon, Dec 13, 2021 at 05:14:35PM +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä > > > > > > Expose FBC debugfs files for each crtc. These may or may not point > > > to the same FBC instance depending on the platform. > > > > > > We leave the old global debugfs files in place until > > > igt catches up to the new per-crtc approach. > > > > > > v2: Take a trip via intel_crtc_debugfs_add() (Jani) > > > > > > Cc: Jani Nikula > > > Signed-off-by: Ville Syrjälä > > > > Apologies if this has already been reported and fixed, I have not seen > > anything on lore.kernel.org or drm-intel about it. > > > > This patch as commit e74c6aa955ca ("drm/i915/fbc: Register per-crtc > > debugfs files") breaks the build when CONFIG_DEBUG_FS is disabled. > > > > drivers/gpu/drm/i915/display/intel_fbc.c: In function > > ‘intel_fbc_crtc_debugfs_add’: > > drivers/gpu/drm/i915/display/intel_fbc.c:1817:61: error: ‘struct drm_crtc’ > > has no member named ‘debugfs_entry’ > > 1817 | intel_fbc_debugfs_add(plane->fbc, > > crtc->base.debugfs_entry); > > | ^ > > Doh. I guess I didn't actually build test the final version with > DEBUGFS=n. > > Does this fix it for you? Yes, it does. Tested-by: Nathan Chancellor > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 13eeba2a750a..4d01b4d89775 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1135,14 +1135,12 @@ struct drm_crtc { >*/ > spinlock_t commit_lock; > > -#ifdef CONFIG_DEBUG_FS > /** >* @debugfs_entry: >* >* Debugfs directory for this CRTC. >*/ > struct dentry *debugfs_entry; > -#endif > > /** >* @crc: > -- > 2.32.0 > > -- > Ville Syrjälä > Intel
[Intel-gfx] [PATCH] drm/i915: Zero initialize this_cpu in busywait_stop
When building with -Wsometimes-uninitialized, Clang warns: drivers/gpu/drm/i915/i915_request.c:1032:6: warning: variable 'this_cpu' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized] time_after expands to use two typecheck with logical ANDs between them. typecheck evaluates to 1 but Clang clearly gets confused with the logic that as semantic analysis happens early in the pipeline. Fix this by just zero initializing this_cpu as it will always be properly initialized before the comparison below. Link: https://github.com/ClangBuiltLinux/linux/issues/415 Signed-off-by: Nathan Chancellor --- Alternatively, this can be solved by having the return value of local_clock_us(&this_cpu) be a local variable but this seems less controversial. drivers/gpu/drm/i915/i915_request.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index c2a5c48c7541..06c0c952191f 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -1027,7 +1027,7 @@ static unsigned long local_clock_us(unsigned int *cpu) static bool busywait_stop(unsigned long timeout, unsigned int cpu) { - unsigned int this_cpu; + unsigned int this_cpu = 0; if (time_after(local_clock_us(&this_cpu), timeout)) return true; -- 2.21.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/3] drm/i915: Enable -Wsometimes-uninitialized
On Tue, Aug 24, 2021 at 03:54:24PM -0700, Nathan Chancellor wrote: > Commit 46e2068081e9 ("drm/i915: Disable some extra clang warnings") > disabled -Wsometimes-uninitialized as noisy but there have been a few > fixes to clang that make the false positive rate fairly low so it should > be enabled to help catch obvious mistakes. The first two patches fix > revent instances of this warning then enables it for i915 like the rest > of the tree. > > Cheers, > Nathan > > Nathan Chancellor (3): > drm/i915/selftests: Do not use import_obj uninitialized > drm/i915/selftests: Always initialize err in > igt_dmabuf_import_same_driver_lmem() > drm/i915: Enable -Wsometimes-uninitialized > > drivers/gpu/drm/i915/Makefile| 1 - > drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 7 --- > 2 files changed, 4 insertions(+), 4 deletions(-) > > > base-commit: fb43ebc83e069625cfeeb2490efc3ffa0013bfa4 > -- > 2.33.0 > > Ping, could this be picked up for an -rc as these are very clearly bugs? Cheers, Nathan
Re: [Intel-gfx] [PATCH 0/3] drm/i915: Enable -Wsometimes-uninitialized
On Tue, Sep 14, 2021 at 08:10:14PM +0300, Jani Nikula wrote: > On Mon, 13 Sep 2021, Nathan Chancellor wrote: > > On Tue, Aug 24, 2021 at 03:54:24PM -0700, Nathan Chancellor wrote: > >> Commit 46e2068081e9 ("drm/i915: Disable some extra clang warnings") > >> disabled -Wsometimes-uninitialized as noisy but there have been a few > >> fixes to clang that make the false positive rate fairly low so it should > >> be enabled to help catch obvious mistakes. The first two patches fix > >> revent instances of this warning then enables it for i915 like the rest > >> of the tree. > >> > >> Cheers, > >> Nathan > >> > >> Nathan Chancellor (3): > >> drm/i915/selftests: Do not use import_obj uninitialized > >> drm/i915/selftests: Always initialize err in > >> igt_dmabuf_import_same_driver_lmem() > >> drm/i915: Enable -Wsometimes-uninitialized > >> > >> drivers/gpu/drm/i915/Makefile| 1 - > >> drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 7 --- > >> 2 files changed, 4 insertions(+), 4 deletions(-) > >> > >> > >> base-commit: fb43ebc83e069625cfeeb2490efc3ffa0013bfa4 > >> -- > >> 2.33.0 > >> > >> > > > > Ping, could this be picked up for an -rc as these are very clearly bugs? > > Thanks for the patches and review. Pushed to drm-intel-gt-next and > cherry-picked to drm-intel-fixes, header to -rc2. Thanks a lot! Cheers, Nathan
[Intel-gfx] [PATCH v2] drm/i915: Clean up disabled warnings
i915 enables a wider set of warnings with '-Wall -Wextra' then disables several with cc-disable-warning. If an unknown flag gets added to KBUILD_CFLAGS when building with clang, all subsequent calls to cc-{disable-warning,option} will fail, meaning that all of these warnings do not get disabled [1]. A separate series will address the root cause of the issue by not adding these flags when building with clang [2]; however, the symptom of these extra warnings appearing can be addressed separately by just removing the calls to cc-disable-warning, which makes the build ever so slightly faster because the compiler does not need to be called as much before building. The following warnings are supported by GCC 4.9 and clang 10.0.1, which are the minimum supported versions of these compilers so the call to cc-disable-warning is not necessary. Masahiro cleaned this up for the reset of the kernel in commit 4c8dd95a723d ("kbuild: add some extra warning flags unconditionally"). * -Wmissing-field-initializers * -Wsign-compare * -Wtype-limits * -Wunused-parameter -Wunused-but-set-variable was implemented in clang 13.0.0 and -Wframe-address was implemented in clang 12.0.0 so the cc-disable-warning calls are kept for these two warnings. Lastly, -Winitializer-overrides is clang's version of -Woverride-init, which is disabled for the specific files that are problematic. clang added a compatibility alias in clang 8.0.0 so -Winitializer-overrides can be removed. [1]: https://lore.kernel.org/r/202108210311.cbtcgoul-...@intel.com/ [2]: https://lore.kernel.org/r/20210824022640.2170859-1-nat...@kernel.org/ Reviewed-by: Nick Desaulniers Signed-off-by: Nathan Chancellor --- v1 -> v2: https://lore.kernel.org/r/20210824232237.2085342-1-nat...@kernel.org/ * Rebase on drm-intel-gt-next now that the prerequisite patch series has been merged: https://lore.kernel.org/r/87wnnj13t5@intel.com/ * Add Nick's reviewed-by tag. drivers/gpu/drm/i915/Makefile | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index c584188aa15a..fd99374583d5 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -13,13 +13,11 @@ # will most likely get a sudden build breakage... Hopefully we will fix # new warnings before CI updates! subdir-ccflags-y := -Wall -Wextra -subdir-ccflags-y += $(call cc-disable-warning, unused-parameter) -subdir-ccflags-y += $(call cc-disable-warning, type-limits) -subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers) +subdir-ccflags-y += -Wno-unused-parameter +subdir-ccflags-y += -Wno-type-limits +subdir-ccflags-y += -Wno-missing-field-initializers +subdir-ccflags-y += -Wno-sign-compare subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable) -# clang warnings -subdir-ccflags-y += $(call cc-disable-warning, sign-compare) -subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) subdir-ccflags-y += $(call cc-disable-warning, frame-address) subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror base-commit: 43192617f7816bb74584c1df06f57363afd15337 -- 2.33.0
Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Clean up disabled warnings (rev2)
I don't really see how this patch could be responsible for either this or the sparse report that I got... it is a simple build time change, nothing at runtime should be affected. Cheers, Nathan On Wed, Sep 15, 2021 at 03:00:19PM -, Patchwork wrote: > == Series Details == > > Series: drm/i915: Clean up disabled warnings (rev2) > URL : https://patchwork.freedesktop.org/series/94009/ > State : failure > > == Summary == > > CI Bug Log - changes from CI_DRM_10590 -> Patchwork_21059 > > > Summary > --- > > **FAILURE** > > Serious unknown changes coming with Patchwork_21059 absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_21059, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > External URL: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/index.html > > Possible new issues > --- > > Here are the unknown changes that may have been introduced in > Patchwork_21059: > > ### IGT changes ### > > Possible regressions > > * igt@core_auth@basic-auth: > - fi-kbl-8809g: [PASS][1] -> [FAIL][2] >[1]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10590/fi-kbl-8809g/igt@core_a...@basic-auth.html >[2]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-kbl-8809g/igt@core_a...@basic-auth.html > > * igt@i915_module_load@reload: > - fi-skl-6700k2: [PASS][3] -> [INCOMPLETE][4] >[3]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10590/fi-skl-6700k2/igt@i915_module_l...@reload.html >[4]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-skl-6700k2/igt@i915_module_l...@reload.html > > > Warnings > > * igt@i915_module_load@reload: > - fi-kbl-soraka: [INCOMPLETE][5] ([i915#4136]) -> [INCOMPLETE][6] >[5]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10590/fi-kbl-soraka/igt@i915_module_l...@reload.html >[6]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-kbl-soraka/igt@i915_module_l...@reload.html > > > Suppressed > > The following results come from untrusted machines, tests, or statuses. > They do not affect the overall result. > > * igt@i915_module_load@reload: > - {fi-ehl-2}: [INCOMPLETE][7] ([i915#4136]) -> [INCOMPLETE][8] >[7]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10590/fi-ehl-2/igt@i915_module_l...@reload.html >[8]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-ehl-2/igt@i915_module_l...@reload.html > - {fi-jsl-1}: [INCOMPLETE][9] ([i915#4136]) -> [INCOMPLETE][10] >[9]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10590/fi-jsl-1/igt@i915_module_l...@reload.html >[10]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-jsl-1/igt@i915_module_l...@reload.html > > > Known issues > > > Here are the changes found in Patchwork_21059 that come from known issues: > > ### IGT changes ### > > Issues hit > > * igt@amdgpu/amd_basic@cs-compute: > - fi-cfl-guc: NOTRUN -> [SKIP][11] ([fdo#109271]) +17 similar > issues >[11]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-cfl-guc/igt@amdgpu/amd_ba...@cs-compute.html > > * igt@amdgpu/amd_basic@cs-sdma: > - fi-kbl-7500u: NOTRUN -> [SKIP][12] ([fdo#109271]) +17 similar > issues >[12]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-kbl-7500u/igt@amdgpu/amd_ba...@cs-sdma.html > > * igt@amdgpu/amd_basic@semaphore: > - fi-icl-y: NOTRUN -> [SKIP][13] ([fdo#109315]) +17 similar > issues >[13]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-icl-y/igt@amdgpu/amd_ba...@semaphore.html > > * igt@amdgpu/amd_cs_nop@fork-gfx0: > - fi-icl-u2: NOTRUN -> [SKIP][14] ([fdo#109315]) +17 similar > issues >[14]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-icl-u2/igt@amdgpu/amd_cs_...@fork-gfx0.html > > * igt@core_hotunplug@unbind-rebind: > - fi-rkl-guc: [PASS][15] -> [INCOMPLETE][16] ([i915#4130]) >[15]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10590/fi-rkl-guc/igt@core_hotunp...@unbind-rebind.html >[16]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-rkl-guc/igt@core_hotunp...@unbind-rebind.html > - fi-cfl-8109u: [PASS][17] -> [INCOMPLETE][18] ([i915#4130]) >[17]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10590/fi-cfl-8109u/igt@core_hotunp...@unbind-rebind.html >[18]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21059/fi-cfl-8109u/igt@core_hotunp...@unbind-rebind.html > > * igt@i915_module_load@reload: > - fi-skl-guc: NOTRUN -> [INCOMPLETE][19] ([i915#4136]) >[19]: > https://intel-gfx-ci.01.org/tree/drm-tip/Pa
Re: [Intel-gfx] [PATCH v3 03/13] drm/dp: add LTTPR DP 2.0 DPCD addresses
On Thu, Sep 09, 2021 at 03:51:55PM +0300, Jani Nikula wrote: > DP 2.0 brings some new DPCD addresses for PHY repeaters. > > Cc: dri-de...@lists.freedesktop.org > Reviewed-by: Manasi Navare > Signed-off-by: Jani Nikula > --- > include/drm/drm_dp_helper.h | 4 > 1 file changed, 4 insertions(+) > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 1d5b3dbb6e56..f3a61341011d 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -1319,6 +1319,10 @@ struct drm_panel; > #define DP_MAX_LANE_COUNT_PHY_REPEATER 0xf0004 /* > 1.4a */ > #define DP_Repeater_FEC_CAPABILITY 0xf0004 /* 1.4 */ > #define DP_PHY_REPEATER_EXTENDED_WAIT_TIMEOUT0xf0005 /* > 1.4a */ > +#define DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER 0xf0006 /* 2.0 */ > +# define DP_PHY_REPEATER_128B132B_SUPPORTED (1 << 0) > +/* See DP_128B132B_SUPPORTED_LINK_RATES for values */ > +#define DP_PHY_REPEATER_128B132B_RATES 0xf0007 /* > 2.0 */ > > enum drm_dp_phy { > DP_PHY_DPRX, > -- > 2.30.2 > > This patch causes a build failure in -next when combined with the AMD tree: In file included from drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c:33: In file included from ./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu.h:70: In file included from ./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu_mode.h:36: ./include/drm/drm_dp_helper.h:1322:9: error: 'DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER' macro redefined [-Werror,-Wmacro-redefined] #define DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER0xf0006 /* 2.0 */ ^ ./drivers/gpu/drm/amd/amdgpu/../display/dc/dc_dp_types.h:881:9: note: previous definition is here #define DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER0xF0006 ^ 1 error generated. Perhaps something like this should be applied during the merge of the second tree or maybe this patch should be in a branch that could be shared between the Intel and AMD trees so that this diff could be applied to the AMD tree directly? Not sure what the standard procedure for this is. Cheers, Nathan diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c index 234dfbea926a..279863b5c650 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c @@ -4590,7 +4590,7 @@ bool dp_retrieve_lttpr_cap(struct dc_link *link) DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV]; link->dpcd_caps.lttpr_caps.supported_128b_132b_rates.raw = - lttpr_dpcd_data[DP_PHY_REPEATER_128b_132b_RATES - + lttpr_dpcd_data[DP_PHY_REPEATER_128B132B_RATES - DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV]; #endif diff --git a/drivers/gpu/drm/amd/display/dc/dc_dp_types.h b/drivers/gpu/drm/amd/display/dc/dc_dp_types.h index a5e798b5da79..8caf9af5ffa2 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_dp_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_dp_types.h @@ -878,8 +878,6 @@ struct psr_caps { # define DP_DSC_DECODER_COUNT_MASK (0b111 << 5) # define DP_DSC_DECODER_COUNT_SHIFT5 #define DP_MAIN_LINK_CHANNEL_CODING_SET0x108 -#define DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER 0xF0006 -#define DP_PHY_REPEATER_128b_132b_RATES0xF0007 #define DP_128b_132b_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER10xF0022 #define DP_INTRA_HOP_AUX_REPLY_INDICATION (1 << 3) /* TODO - Use DRM header to replace above once available */
Re: [Intel-gfx] [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
Hi Will and Robin, On 7/6/2021 10:06 AM, Will Deacon wrote: On Tue, Jul 06, 2021 at 04:39:11PM +0100, Robin Murphy wrote: On 2021-07-06 15:05, Christoph Hellwig wrote: On Tue, Jul 06, 2021 at 03:01:04PM +0100, Robin Murphy wrote: FWIW I was pondering the question of whether to do something along those lines or just scrap the default assignment entirely, so since I hadn't got round to saying that I've gone ahead and hacked up the alternative (similarly untested) for comparison :) TBH I'm still not sure which one I prefer... Claire did implement something like your suggestion originally, but I don't really like it as it doesn't scale for adding multiple global pools, e.g. for the 64-bit addressable one for the various encrypted secure guest schemes. Ah yes, that had slipped my mind, and it's a fair point indeed. Since we're not concerned with a minimal fix for backports anyway I'm more than happy to focus on Will's approach. Another thing is that that looks to take us a quiet step closer to the possibility of dynamically resizing a SWIOTLB pool, which is something that some of the hypervisor protection schemes looking to build on top of this series may want to explore at some point. Ok, I'll split that nasty diff I posted up into a reviewable series and we can take it from there. For what it's worth, I attempted to boot Will's diff on top of Konrad's devel/for-linus-5.14 and it did not work; in fact, I got no output on my monitor period, even with earlyprintk=, and I do not think this machine has a serial console. Robin's fix does work, it survived ten reboots with no issues getting to X and I do not see the KASAN and slub debug messages anymore but I understand that this is not the preferred solution it seems (although Konrad did want to know if it works). I am happy to test any further patches or follow ups as needed, just keep me on CC. Cheers, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [vfio:next 33/38] drivers/gpu/drm/i915/i915_pci.c:975:2: warning: missing field 'override_only' initializer
On Fri, Oct 01, 2021 at 02:04:04PM +0300, Jani Nikula wrote: > On Fri, 27 Aug 2021, Jason Gunthorpe wrote: > > On Fri, Aug 27, 2021 at 03:12:36PM +, kernel test robot wrote: > >> tree: https://github.com/awilliam/linux-vfio.git next > >> head: ea870730d83fc13a5fa2bd0e175176d7ac8a400a > >> commit: 343b7258687ecfbb363bfda8833a7cf641aac524 [33/38] PCI: Add > >> 'override_only' field to struct pci_device_id > >> config: i386-randconfig-a004-20210827 (attached as .config) > >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project > >> 1076082a0d97bd5c16a25ee7cf3dbb6ee4b5a9fe) > >> reproduce (this is a W=1 build): > >> wget > >> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross > >> -O ~/bin/make.cross > >> chmod +x ~/bin/make.cross > >> # > >> https://github.com/awilliam/linux-vfio/commit/343b7258687ecfbb363bfda8833a7cf641aac524 > >> git remote add vfio https://github.com/awilliam/linux-vfio.git > >> git fetch --no-tags vfio next > >> git checkout 343b7258687ecfbb363bfda8833a7cf641aac524 > >> # save the attached .config to linux build tree > >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross > >> ARCH=i386 > >> > >> If you fix the issue, kindly add following tag as appropriate > >> Reported-by: kernel test robot > > > > Ugh, this is due to this code: > > > > #define INTEL_VGA_DEVICE(id, info) {\ > > 0x8086, id, \ > > ~0, ~0, \ > > 0x03, 0xff, \ > > (unsigned long) info } > > > > #define INTEL_QUANTA_VGA_DEVICE(info) { \ > > 0x8086, 0x16a, \ > > 0x152d, 0x8990, \ > > 0x03, 0xff, \ > > (unsigned long) info } > > > > > > Which really should be using the normal pattern for defining these > > structs: > > > > #define PCI_DEVICE_CLASS(dev_class,dev_class_mask) \ > > .class = (dev_class), .class_mask = (dev_class_mask), \ > > .vendor = PCI_ANY_ID, .device = PCI_ANY_ID, \ > > .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID > > > > The warning is also not a real issue, just clang being overzealous. > > Stumbled upon this old report, sorry for the delayed response. > > The reason it's not using designated initializers is that the same file > gets synced to some userspace projects (at least libdrm and > igt-gpu-tools) which use the macros to initialize slightly different > structs. For example, igt uses struct pci_id_match from libpciaccess-dev > (/usr/include/pciaccess.h) and can't easily adapt to different member > names. > > Anyway, we've got > > subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers) > subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) > > in drivers/gpu/drm/i915/Makefile, so I wonder why they're not respected. This report was from an i386 randconfig, which we recently had a lot of issues with: https://git.kernel.org/linus/7fa6a2746616c8de4c40b748c2bb0656e00624ff Applying my patch to remove most of the cc-disable-warnings in your Makefile would help avoid these reports in the future :) https://lore.kernel.org/r/20210914194944.4004260-1-nat...@kernel.org/ Cheers, Nathan
[Intel-gfx] [PATCH] drm/i915: Avoid bitwise vs logical OR warning in snb_wm_latency_quirk()
A new warning in clang points out a place in this file where a bitwise OR is being used with boolean types: drivers/gpu/drm/i915/intel_pm.c:3066:12: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical] changed = ilk_increase_wm_latency(dev_priv, dev_priv->wm.pri_latency, 12) | ^ This construct is intentional, as it allows every one of the calls to ilk_increase_wm_latency() to occur (instead of short circuiting with logical OR) while still caring about the result of each call. To make this clearer to the compiler, use the '|=' operator to assign the result of each ilk_increase_wm_latency() call to changed, which keeps the meaning of the code the same but makes it obvious that every one of these calls is expected to happen. Link: https://github.com/ClangBuiltLinux/linux/issues/1473 Reported-by: Nick Desaulniers Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/intel_pm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index f90fe39cf8ca..aaa3a0998e4c 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3050,9 +3050,9 @@ static void snb_wm_latency_quirk(struct drm_i915_private *dev_priv) * The BIOS provided WM memory latency values are often * inadequate for high resolution displays. Adjust them. */ - changed = ilk_increase_wm_latency(dev_priv, dev_priv->wm.pri_latency, 12) | - ilk_increase_wm_latency(dev_priv, dev_priv->wm.spr_latency, 12) | - ilk_increase_wm_latency(dev_priv, dev_priv->wm.cur_latency, 12); + changed = ilk_increase_wm_latency(dev_priv, dev_priv->wm.pri_latency, 12); + changed |= ilk_increase_wm_latency(dev_priv, dev_priv->wm.spr_latency, 12); + changed |= ilk_increase_wm_latency(dev_priv, dev_priv->wm.cur_latency, 12); if (!changed) return; base-commit: d73b17465d6da0a94bc0fcc86b150e1e923e8f71 -- 2.33.1.637.gf443b226ca
[Intel-gfx] [PATCH] drm/i915/selftest: Fix use of err in igt_reset_{fail, nop}_engine()
Clang warns: In file included from drivers/gpu/drm/i915/gt/intel_reset.c:1514: drivers/gpu/drm/i915/gt/selftest_hangcheck.c:465:62: warning: variable 'err' is uninitialized when used here [-Wuninitialized] pr_err("[%s] Create context failed: %d!\n", engine->name, err); ^~~ ... drivers/gpu/drm/i915/gt/selftest_hangcheck.c:580:62: warning: variable 'err' is uninitialized when used here [-Wuninitialized] pr_err("[%s] Create context failed: %d!\n", engine->name, err); ^~~ ... 2 warnings generated. This appears to be a copy and paste issue. Use ce directly using the %pe specifier to pretty print the error code so that err is not used uninitialized in these functions. Fixes: 3a7b72665ea5 ("drm/i915/selftest: Bump selftest timeouts for hangcheck") Reported-by: Dan Carpenter Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c index 08f011f893b2..2c1ed32ca5ac 100644 --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c @@ -462,7 +462,7 @@ static int igt_reset_nop_engine(void *arg) ce = intel_context_create(engine); if (IS_ERR(ce)) { - pr_err("[%s] Create context failed: %d!\n", engine->name, err); + pr_err("[%s] Create context failed: %pe!\n", engine->name, ce); return PTR_ERR(ce); } @@ -577,7 +577,7 @@ static int igt_reset_fail_engine(void *arg) ce = intel_context_create(engine); if (IS_ERR(ce)) { - pr_err("[%s] Create context failed: %d!\n", engine->name, err); + pr_err("[%s] Create context failed: %pe!\n", engine->name, ce); return PTR_ERR(ce); } base-commit: 927dfdd09d8c03ba100ed0c8c3915f8e1d1f5556 -- 2.33.0.rc2
Re: [Intel-gfx] [bug report] drm/i915/gem: Correct the locking and pin pattern for dma-buf (v8)
On 8/13/2021 4:31 AM, Dan Carpenter wrote: Nathan has probably already sent fixes for these. Nathan, could you CC kernel-janitors on static checker fixes? That way we wouldn't send so many duplicate patches. Sure. I did not send any fixes prior to this email but I just sent https://lore.kernel.org/r/20210813171158.2665823-1-nat...@kernel.org/ with kernel-janitors and you CC'd. Uninitialized variable warnings are really frustrating. We disabled the GCC warning for them because the new GCC is crap. So now they take up a lot of my time. A few of these are because Smatch doesn't always know when we enter loops, but more than half are clear bugs. drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c:189 i915_gem_dmabuf_attach() error: uninitialized symbol 'err'. drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:80 igt_dmabuf_import_self() error: uninitialized symbol 'import_obj'. drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:104 igt_dmabuf_import_same_driver_lmem() error: uninitialized symbol 'dmabuf'. drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:138 igt_dmabuf_import_same_driver_lmem() error: uninitialized symbol 'err'. drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:158 igt_dmabuf_import_same_driver() error: uninitialized symbol 'dmabuf'. drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:231 igt_dmabuf_import_same_driver() error: uninitialized symbol 'import_obj'. drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c:197 igt_lmem_pages_migrate() error: uninitialized symbol 'err'. ... drivers/gpu/drm/i915/gt/selftest_hangcheck.c:465 igt_reset_nop_engine() error: uninitialized symbol 'err'. drivers/gpu/drm/i915/gt/selftest_hangcheck.c:580 igt_reset_fail_engine() error: uninitialized symbol 'err'. Clang caught these two but none of the rest and it is an obvious fix so that is the only one I am going to fix. drivers/gpu/drm/i915/gt/selftest_migrate.c:102 copy() error: uninitialized symbol 'rq'. drivers/gpu/drm/i915/gt/selftest_migrate.c:113 copy() error: uninitialized symbol 'vaddr'. drivers/gpu/drm/i915/gt/selftest_migrate.c:182 clear() error: uninitialized symbol 'rq'. drivers/gpu/drm/i915/gt/selftest_migrate.c:193 clear() error: uninitialized symbol 'vaddr'. drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:819 __unwind_incomplete_requests() error: uninitialized symbol 'pl'. Cheers, Nathan
Re: [Intel-gfx] [PATCH] drm/i915/selftest: Fix use of err in igt_reset_{fail, nop}_engine()
Ping? This is a pretty clear bug and it is not fixed in -next or drm-intel at this point. On Fri, Aug 13, 2021 at 10:11:58AM -0700, Nathan Chancellor wrote: > Clang warns: > > In file included from drivers/gpu/drm/i915/gt/intel_reset.c:1514: > drivers/gpu/drm/i915/gt/selftest_hangcheck.c:465:62: warning: variable > 'err' is uninitialized when used here [-Wuninitialized] > pr_err("[%s] Create context failed: %d!\n", engine->name, err); > ^~~ > ... > drivers/gpu/drm/i915/gt/selftest_hangcheck.c:580:62: warning: variable > 'err' is uninitialized when used here [-Wuninitialized] > pr_err("[%s] Create context failed: %d!\n", engine->name, err); > ^~~ > ... > 2 warnings generated. > > This appears to be a copy and paste issue. Use ce directly using the %pe > specifier to pretty print the error code so that err is not used > uninitialized in these functions. > > Fixes: 3a7b72665ea5 ("drm/i915/selftest: Bump selftest timeouts for > hangcheck") > Reported-by: Dan Carpenter > Signed-off-by: Nathan Chancellor > --- > drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > index 08f011f893b2..2c1ed32ca5ac 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > @@ -462,7 +462,7 @@ static int igt_reset_nop_engine(void *arg) > > ce = intel_context_create(engine); > if (IS_ERR(ce)) { > - pr_err("[%s] Create context failed: %d!\n", > engine->name, err); > + pr_err("[%s] Create context failed: %pe!\n", > engine->name, ce); > return PTR_ERR(ce); > } > > @@ -577,7 +577,7 @@ static int igt_reset_fail_engine(void *arg) > > ce = intel_context_create(engine); > if (IS_ERR(ce)) { > - pr_err("[%s] Create context failed: %d!\n", > engine->name, err); > + pr_err("[%s] Create context failed: %pe!\n", > engine->name, ce); > return PTR_ERR(ce); > } > > > base-commit: 927dfdd09d8c03ba100ed0c8c3915f8e1d1f5556 > -- > 2.33.0.rc2
[Intel-gfx] [PATCH] drm/i915: Clean up disabled warnings
i915 enables a wider set of warnings with '-Wall -Wextra' then disables several with cc-disable-warning. If an unknown flag gets added to KBUILD_CFLAGS when building with clang, all subsequent calls to cc-{disable-warning,option} will fail, meaning that all of these warnings do not get disabled [1]. A separate series will address the root cause of the issue by not adding these flags when building with clang [2]; however, the symptom of these extra warnings appearing can be addressed separately by just removing the calls to cc-disable-warning, which makes the build ever so slightly faster because the compiler does not need to be called as much before building. The following warnings are supported by GCC 4.9 and clang 10.0.1, which are the minimum supported versions of these compilers so the call to cc-disable-warning is not necessary. Masahiro cleaned this up for the reset of the kernel in commit 4c8dd95a723d ("kbuild: add some extra warning flags unconditionally"). * -Wmissing-field-initializers * -Wsign-compare * -Wtype-limits * -Wunused-parameter -Wunused-but-set-variable was implemented in clang 13.0.0 and -Wframe-address was implemented in clang 12.0.0 so the cc-disable-warning calls are kept for these two warnings. Lastly, -Winitializer-overrides is clang's version of -Woverride-init, which is disabled for the specific files that are problematic. clang added a compatibility alias in clang 8.0.0 so -Winitializer-overrides can be removed. [1]: https://lore.kernel.org/r/202108210311.cbtcgoul-...@intel.com/ [2]: https://lore.kernel.org/r/20210824022640.2170859-1-nat...@kernel.org/ Signed-off-by: Nathan Chancellor --- NOTE: This is based on my series to enable -Wsometimes-initialized here: https://lore.kernel.org/r/20210824225427.2065517-1-nat...@kernel.org/ I sent it separately as this can go into whatever release but I would like for that series to go into 5.15. drivers/gpu/drm/i915/Makefile | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 335ba9f43d8f..6b38547543b1 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -13,13 +13,11 @@ # will most likely get a sudden build breakage... Hopefully we will fix # new warnings before CI updates! subdir-ccflags-y := -Wall -Wextra -subdir-ccflags-y += $(call cc-disable-warning, unused-parameter) -subdir-ccflags-y += $(call cc-disable-warning, type-limits) -subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers) +subdir-ccflags-y += -Wno-unused-parameter +subdir-ccflags-y += -Wno-type-limits +subdir-ccflags-y += -Wno-missing-field-initializers +subdir-ccflags-y += -Wno-sign-compare subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable) -# clang warnings -subdir-ccflags-y += $(call cc-disable-warning, sign-compare) -subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) subdir-ccflags-y += $(call cc-disable-warning, frame-address) subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror base-commit: fb43ebc83e069625cfeeb2490efc3ffa0013bfa4 prerequisite-patch-id: 31c28450ed7e8785dce967a16db6d52eff3d7d6d prerequisite-patch-id: 372dfa0e07249f207acc1942ab0e39b13ff229b2 prerequisite-patch-id: 1a585fa6cda50c32ad1e3ac8235d3cff1b599978 -- 2.33.0
[Intel-gfx] [PATCH 0/3] drm/i915: Enable -Wsometimes-uninitialized
Commit 46e2068081e9 ("drm/i915: Disable some extra clang warnings") disabled -Wsometimes-uninitialized as noisy but there have been a few fixes to clang that make the false positive rate fairly low so it should be enabled to help catch obvious mistakes. The first two patches fix revent instances of this warning then enables it for i915 like the rest of the tree. Cheers, Nathan Nathan Chancellor (3): drm/i915/selftests: Do not use import_obj uninitialized drm/i915/selftests: Always initialize err in igt_dmabuf_import_same_driver_lmem() drm/i915: Enable -Wsometimes-uninitialized drivers/gpu/drm/i915/Makefile| 1 - drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 7 --- 2 files changed, 4 insertions(+), 4 deletions(-) base-commit: fb43ebc83e069625cfeeb2490efc3ffa0013bfa4 -- 2.33.0
[Intel-gfx] [PATCH 1/3] drm/i915/selftests: Do not use import_obj uninitialized
Clang warns a couple of times: drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:63:6: warning: variable 'import_obj' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (import != &obj->base) { ^~~~ drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:80:22: note: uninitialized use occurs here i915_gem_object_put(import_obj); ^~ drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:63:2: note: remove the 'if' if its condition is always false if (import != &obj->base) { ^~~ drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:38:46: note: initialize the variable 'import_obj' to silence this warning struct drm_i915_gem_object *obj, *import_obj; ^ = NULL Shuffle the import_obj initialization above these if statements so that it is not used uninitialized. Fixes: d7b2cb380b3a ("drm/i915/gem: Correct the locking and pin pattern for dma-buf (v8)") Reported-by: Dan Carpenter Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c index ffae7df5e4d7..532c7955b300 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c @@ -59,13 +59,13 @@ static int igt_dmabuf_import_self(void *arg) err = PTR_ERR(import); goto out_dmabuf; } + import_obj = to_intel_bo(import); if (import != &obj->base) { pr_err("i915_gem_prime_import created a new object!\n"); err = -EINVAL; goto out_import; } - import_obj = to_intel_bo(import); i915_gem_object_lock(import_obj, NULL); err = __i915_gem_object_get_pages(import_obj); @@ -176,6 +176,7 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915, err = PTR_ERR(import); goto out_dmabuf; } + import_obj = to_intel_bo(import); if (import == &obj->base) { pr_err("i915_gem_prime_import reused gem object!\n"); @@ -183,8 +184,6 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915, goto out_import; } - import_obj = to_intel_bo(import); - i915_gem_object_lock(import_obj, NULL); err = __i915_gem_object_get_pages(import_obj); if (err) { -- 2.33.0
[Intel-gfx] [PATCH 3/3] drm/i915: Enable -Wsometimes-uninitialized
This warning helps catch uninitialized variables. It should have been enabled at the same time as commit b2423184ac33 ("drm/i915: Enable -Wuninitialized") but I did not realize they were disabled separately. Enable it now that i915 is clean so that it stays that way. Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 642a5b5a1b81..335ba9f43d8f 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -19,7 +19,6 @@ subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers) subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable) # clang warnings subdir-ccflags-y += $(call cc-disable-warning, sign-compare) -subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized) subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) subdir-ccflags-y += $(call cc-disable-warning, frame-address) subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror -- 2.33.0
[Intel-gfx] [PATCH 2/3] drm/i915/selftests: Always initialize err in igt_dmabuf_import_same_driver_lmem()
Clang warns: drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:127:13: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] } else if (PTR_ERR(import) != -EOPNOTSUPP) { ^~ drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:138:9: note: uninitialized use occurs here return err; ^~~ drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:127:9: note: remove the 'if' if its condition is always true } else if (PTR_ERR(import) != -EOPNOTSUPP) { ^~~~ drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:95:9: note: initialize the variable 'err' to silence this warning int err; ^ = 0 The test is expected to pass if i915_gem_prime_import() returns -EOPNOTSUPP so initialize err to zero in this case. Fixes: cdb35d1ed6d2 ("drm/i915/gem: Migrate to system at dma-buf attach time (v7)") Reported-by: Dan Carpenter Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c index 532c7955b300..4a6bb64c3a35 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c @@ -128,6 +128,8 @@ static int igt_dmabuf_import_same_driver_lmem(void *arg) pr_err("i915_gem_prime_import failed with the wrong err=%ld\n", PTR_ERR(import)); err = PTR_ERR(import); + } else { + err = 0; } dma_buf_put(dmabuf); -- 2.33.0
Re: [Intel-gfx] [PATCH] drm/i915: Clean up disabled warnings
On 8/25/2021 4:03 PM, Nick Desaulniers wrote: On Tue, Aug 24, 2021 at 4:23 PM Nathan Chancellor wrote: i915 enables a wider set of warnings with '-Wall -Wextra' then disables several with cc-disable-warning. If an unknown flag gets added to KBUILD_CFLAGS when building with clang, all subsequent calls to cc-{disable-warning,option} will fail, meaning that all of these warnings do not get disabled [1]. A separate series will address the root cause of the issue by not adding these flags when building with clang [2]; however, the symptom of these extra warnings appearing can be addressed separately by just removing the calls to cc-disable-warning, which makes the build ever so slightly faster because the compiler does not need to be called as much before building. The following warnings are supported by GCC 4.9 and clang 10.0.1, which are the minimum supported versions of these compilers so the call to cc-disable-warning is not necessary. Masahiro cleaned this up for the reset of the kernel in commit 4c8dd95a723d ("kbuild: add some extra warning flags unconditionally"). * -Wmissing-field-initializers * -Wsign-compare * -Wtype-limits * -Wunused-parameter -Wunused-but-set-variable was implemented in clang 13.0.0 and -Wframe-address was implemented in clang 12.0.0 so the cc-disable-warning calls are kept for these two warnings. Lastly, -Winitializer-overrides is clang's version of -Woverride-init, which is disabled for the specific files that are problematic. clang added a compatibility alias in clang 8.0.0 so -Winitializer-overrides can be removed. [1]: https://lore.kernel.org/r/202108210311.cbtcgoul-...@intel.com/ [2]: https://lore.kernel.org/r/20210824022640.2170859-1-nat...@kernel.org/ Signed-off-by: Nathan Chancellor Thanks for the patch! Do you need to re-ping, rebase, or resend that other series? Reviewed-by: Nick Desaulniers I assume you mean the series below rather than above? I sent this patch right after that series and it has one set of reviews so I am hoping the i915 maintainers will pick them up soon so this one can be applied afterwards or resent. Thank you for the review! Cheers, Nathan --- NOTE: This is based on my series to enable -Wsometimes-initialized here: https://lore.kernel.org/r/20210824225427.2065517-1-nat...@kernel.org/ I sent it separately as this can go into whatever release but I would like for that series to go into 5.15. drivers/gpu/drm/i915/Makefile | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 335ba9f43d8f..6b38547543b1 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -13,13 +13,11 @@ # will most likely get a sudden build breakage... Hopefully we will fix # new warnings before CI updates! subdir-ccflags-y := -Wall -Wextra -subdir-ccflags-y += $(call cc-disable-warning, unused-parameter) -subdir-ccflags-y += $(call cc-disable-warning, type-limits) -subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers) +subdir-ccflags-y += -Wno-unused-parameter +subdir-ccflags-y += -Wno-type-limits +subdir-ccflags-y += -Wno-missing-field-initializers +subdir-ccflags-y += -Wno-sign-compare subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable) -# clang warnings -subdir-ccflags-y += $(call cc-disable-warning, sign-compare) -subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) subdir-ccflags-y += $(call cc-disable-warning, frame-address) subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror base-commit: fb43ebc83e069625cfeeb2490efc3ffa0013bfa4 prerequisite-patch-id: 31c28450ed7e8785dce967a16db6d52eff3d7d6d prerequisite-patch-id: 372dfa0e07249f207acc1942ab0e39b13ff229b2 prerequisite-patch-id: 1a585fa6cda50c32ad1e3ac8235d3cff1b599978 -- 2.33.0
Re: [Intel-gfx] [PATCH] drm/i915: fix build issue when using clang
On Sat, Feb 12, 2022 at 10:51:06PM -0800, Tong Zhang wrote: > drm/i915 target adds some extra cflags, especially it does re-apply -Wall. > In clang this will override -Wno-format-security and cause the build to > fail when CONFIG_DRM_I915_WERROR=y. While with GCC this does not happen. > We reapply -Wno-format-security here to get around this issue. For what it's worth, GCC would warn in the exact same way if -Wformat-security was included within -Wall like it is for clang: drivers/gpu/drm/i915/gt/intel_gt.c: In function ‘intel_gt_invalidate_tlbs’: drivers/gpu/drm/i915/gt/intel_gt.c:988:9: error: format not a string literal and no format arguments [-Werror=format-security] 988 | GEM_TRACE("\n"); | ^ cc1: all warnings being treated as errors drivers/gpu/drm/i915/gt/selftest_execlists.c: In function ‘live_sanitycheck’: drivers/gpu/drm/i915/gt/selftest_execlists.c:142:25: error: format not a string literal and no format arguments [-Werror=format-security] 142 | GEM_TRACE("spinner failed to start\n"); | ^ drivers/gpu/drm/i915/gt/selftest_execlists.c: In function ‘live_preempt’: drivers/gpu/drm/i915/gt/selftest_execlists.c:1775:25: error: format not a string literal and no format arguments [-Werror=format-security] 1775 | GEM_TRACE("lo spinner failed to start\n"); | ^ drivers/gpu/drm/i915/gt/selftest_execlists.c:1792:25: error: format not a string literal and no format arguments [-Werror=format-security] 1792 | GEM_TRACE("hi spinner failed to start\n"); | ^ cc1: all warnings being treated as errors If fixing these warnings instead of just disabling the warning is undesirable (since I feel like the entire point of the i195 cflags situation is to enable more warnings than the main Makefile), I think the commit message should be rewritten to something along the lines of: "drm/i915 adds some extra cflags, namely -Wall, which causes instances of -Wformat-security to appear when building with clang, even though this warning is turned off kernel-wide in the main Makefile:" > drivers/gpu/drm/i915/gt/intel_gt.c:983:2: error: format string is not a > string literal (potentially insecure) [-Werror,-Wformat-security] > GEM_TRACE("ERROR\n"); > ^~~~ > ./drivers/gpu/drm/i915/i915_gem.h:76:24: note: expanded from macro 'GEM_TRACE' > #define GEM_TRACE(...) trace_printk(__VA_ARGS__) >^ > ./include/linux/kernel.h:369:3: note: expanded from macro 'trace_printk' > do_trace_printk(fmt, ##__VA_ARGS__);\ > ^~~ > ./include/linux/kernel.h:383:30: note: expanded from macro 'do_trace_printk' > __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \ >^~~~ > drivers/gpu/drm/i915/gt/intel_gt.c:983:2: note: treat the string as an > argument to avoid this "This does not happen with GCC because it does not enable -Wformat-security with -Wall. Disable -Wformat-security within the i915 Makefile so that these warnings do not show up with clang." The actual diff itself looks fine to me. Cheers, Nathan > Signed-off-by: Tong Zhang > --- > drivers/gpu/drm/i915/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 1b62b9f65196..c04e05a3d39f 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -13,6 +13,7 @@ > # will most likely get a sudden build breakage... Hopefully we will fix > # new warnings before CI updates! > subdir-ccflags-y := -Wall -Wextra > +subdir-ccflags-y += -Wno-format-security > subdir-ccflags-y += -Wno-unused-parameter > subdir-ccflags-y += -Wno-type-limits > subdir-ccflags-y += -Wno-missing-field-initializers > -- > 2.25.1 >
Re: [Intel-gfx] [PATCH v2] drm/i915: fix build issue when using clang
On Mon, Feb 14, 2022 at 11:58:20AM -0800, Tong Zhang wrote: > drm/i915 adds some extra cflags, namely -Wall, which causes > instances of -Wformat-security to appear when building with clang, even > though this warning is turned off kernel-wide in the main Makefile: > > > drivers/gpu/drm/i915/gt/intel_gt.c:983:2: error: format string is not a > > string literal (potentially insecure) [-Werror,-Wformat-security] > > GEM_TRACE("ERROR\n"); > > ^~~~ > > ./drivers/gpu/drm/i915/i915_gem.h:76:24: note: expanded from macro > > 'GEM_TRACE' > > #define GEM_TRACE(...) trace_printk(__VA_ARGS__) > >^ > > ./include/linux/kernel.h:369:3: note: expanded from macro 'trace_printk' > > do_trace_printk(fmt, ##__VA_ARGS__);\ > > ^~~ > > ./include/linux/kernel.h:383:30: note: expanded from macro 'do_trace_printk' > > __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \ > > ^~~~ > >drivers/gpu/drm/i915/gt/intel_gt.c:983:2: note: treat the string as an > >argument to avoid this > > This does not happen with GCC because it does not enable > -Wformat-security with -Wall. Disable -Wformat-security within the i915 > Makefile so that these warnings do not show up with clang. > > Signed-off-by: Tong Zhang Given this is not enabled for GCC and it is disabled in the main Makefile: Reviewed-by: Nathan Chancellor Additionally, it seems like trace_printk() is designed to be able to take a string literal without a format argument, so this should be fine. > --- > > v2: revise commit message > > drivers/gpu/drm/i915/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 1b62b9f65196..c04e05a3d39f 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -13,6 +13,7 @@ > # will most likely get a sudden build breakage... Hopefully we will fix > # new warnings before CI updates! > subdir-ccflags-y := -Wall -Wextra > +subdir-ccflags-y += -Wno-format-security > subdir-ccflags-y += -Wno-unused-parameter > subdir-ccflags-y += -Wno-type-limits > subdir-ccflags-y += -Wno-missing-field-initializers > -- > 2.25.1 > >
Re: [Intel-gfx] [PATCH] [v2] Kbuild: move to -std=gnu11
On Mon, Feb 28, 2022 at 12:57:55PM +0100, Arnd Bergmann wrote: > On Mon, Feb 28, 2022 at 12:47 PM Marco Elver wrote: > > On Mon, 28 Feb 2022 at 11:32, Arnd Bergmann wrote: > > > > > Nathan Chancellor reported an additional -Wdeclaration-after-statement > > > warning that appears in a system header on arm, this still needs a > > > workaround. > > > > On the topic of Wdeclaration-after-statement, Clang only respects this > > warning with C99 and later starting with Clang 14: > > https://github.com/llvm/llvm-project/commit/c65186c89f35#diff-ec770381d76c859f5f572db789175fe44410a72608f58ad5dbb14335ba56eb97R61 > > > > Until Clang 14, -Wdeclaration-after-statement is ignored by Clang in > > newer standards. If this is a big problem, we can probably convince > > the Clang stable folks to backport the fixes. However, the build won't > > fail, folks might just miss the warning if they don't also test with > > GCC. Unfortunately, none of the branches prior to release/14.x are going to see any more updates (at least as far as I am aware, as the LLVM community only supports one release branch at a time) but as Arnd mentioned below, I do not really see that as a problem, as newer versions of clang and GCC will catch these warnings. > I don't expect this is to be a big issue, as long as the latest clang behaves > as expected. There are many warnings that are only produced by one of the > two compilers, so this is something we already deal with. > > I think it's more important to address the extra warning that Nathan > reported, where clang now complains about the intermingled declaration > in a system header when previously neither gcc nor clang noticed this. Right. Based on the upstream LLVM bug, I think we should just fix arm_neon.h to avoid triggering -Wdeclaration-after-statement to have something that is (hopefully) relatively low risk for a clang-14 backport, rather than addressing the root cause of clang warning in system macros, as it sounds like fixing that has some risks that are not fully understood at this point. The kernel only uses very specific system headers after commit 04e85bbf71c9 ("isystem: delete global -isystem compile option"), so I don't think that my suggested approach will have many downsides. I think I see how to potentially fix arm_neon.h in clang/utils/TableGen/NeonEmitter.cpp, I just have to think about it a little more. Realistically, I don't think special casing this in lib/raid6 is the end of the world: diff --git a/lib/raid6/Makefile b/lib/raid6/Makefile index 45e17619422b..a41ff71b90af 100644 --- a/lib/raid6/Makefile +++ b/lib/raid6/Makefile @@ -38,6 +38,10 @@ ifeq ($(CONFIG_KERNEL_MODE_NEON),y) NEON_FLAGS := -ffreestanding # Enable NEON_FLAGS += -isystem $(shell $(CC) -print-file-name=include) +# https://github.com/ClangBuiltLinux/linux/issues/1603 +ifeq ($(CONFIG_CC_IS_CLANG)$(CONFIG_CPU_BIG_ENDIAN),yy) +NEON_FLAGS += -Wno-declaration-after-statement +endif ifeq ($(ARCH),arm) NEON_FLAGS += -march=armv7-a -mfloat-abi=softfp -mfpu=neon endif > > > The differences between gnu99, gnu11, gnu1x and gnu17 are fairly > > > minimal and mainly impact warnings at the -Wpedantic level that the > > > kernel never enables. Between these, gnu11 is the newest version > > > that is supported by all supported compiler versions, though it is > > > only the default on gcc-5, while all other supported versions of > > > gcc or clang default to gnu1x/gnu17. > > > > > > Link: > > > https://lore.kernel.org/lkml/CAHk-=wiych7xehcmifj-ygxuy2jaj7pnkdkpcovt8fybvfw...@mail.gmail.com/ > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1603 > > > Suggested-by: Linus Torvalds > > > Cc: Masahiro Yamada > > > Cc: linux-kbu...@vger.kernel.org > > > Cc: l...@lists.linux.dev > > > Signed-off-by: Arnd Bergmann > > > > Acked-by: Marco Elver Cheers, Nathan
Re: [Intel-gfx] [PATCH 1/2] drm/v3d: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
Hi Maira, On Thu, Jul 27, 2023 at 11:01:27AM -0300, Maira Canal wrote: > Hi Nathan, > > On 7/18/23 18:44, Nathan Chancellor wrote: > > A proposed update to clang's -Wconstant-logical-operand to warn when the > > left hand side is a constant shows the following instance in > > nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, > > such as CONFIG_HZ=300: > > > >In file included from drivers/gpu/drm/v3d/v3d_debugfs.c:12: > >drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with > > constant operand [-Wconstant-logical-operand] > > 343 | if (NSEC_PER_SEC % HZ && > > | ~ ^ > >drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise > > operation > > 343 | if (NSEC_PER_SEC % HZ && > > | ^~ > > | & > >drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence > > this warning > >1 warning generated. > > > > Turn this into an explicit comparison against zero to make the > > expression a boolean to make it clear this should be a logical check, > > not a bitwise one. > > > > Link: https://reviews.llvm.org/D142609 > > Signed-off-by: Nathan Chancellor > > Reviewed-by: Maíra Canal > > Thanks for all the hard work with clang! Let me know if you need someone > to push it to drm-misc-next. Yes I will, I do not have drm commit rights. I think the i915 patch can go to drm-misc as well with Tvrtko's ack. Thanks a lot for taking a look! Cheers, Nathan > Best Regards, > - Maíra > > > --- > > drivers/gpu/drm/v3d/v3d_drv.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h > > index b74b1351bfc8..7f664a4b2a75 100644 > > --- a/drivers/gpu/drm/v3d/v3d_drv.h > > +++ b/drivers/gpu/drm/v3d/v3d_drv.h > > @@ -340,7 +340,7 @@ struct v3d_submit_ext { > > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) > > { > > /* nsecs_to_jiffies64() does not guard against overflow */ > > - if (NSEC_PER_SEC % HZ && > > + if ((NSEC_PER_SEC % HZ) != 0 && > > div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ) > > return MAX_JIFFY_OFFSET; > >
[Intel-gfx] [PATCH] drm/i915: Fix clang -Wimplicit-fallthrough in intel_async_flip_check_hw()
Clang warns: drivers/gpu/drm/i915/display/intel_display.c:6012:3: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough] case I915_FORMAT_MOD_X_TILED: ^ drivers/gpu/drm/i915/display/intel_display.c:6012:3: note: insert 'break;' to avoid fall-through case I915_FORMAT_MOD_X_TILED: ^ break; 1 error generated. Clang is a little more pedantic than GCC, which does not warn when falling through to a case that is just break or return. Clang's version is more in line with the kernel's own stance in deprecated.rst, which states that all switch/case blocks must end in either break, fallthrough, continue, goto, or return. Add the missing break to silence the warning. Fixes: 937859485aef ("drm/i915: Support Async Flip on Linear buffers") Reported-by: kernel test robot Closes: https://lore.kernel.org/202305241902.uvhtmoxa-...@intel.com/ Reported-by: Naresh Kamboju Closes: https://lore.kernel.org/CA+G9fYv68V3ewK0Qj-syQj7qX-hQr0H1MFL=qfnudoe_j2z...@mail.gmail.com/ Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/display/intel_display.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 0490c6412ab5..6d49e0ab3e85 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -6008,6 +6008,7 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in plane->base.base.id, plane->base.name); return -EINVAL; } + break; case I915_FORMAT_MOD_X_TILED: case I915_FORMAT_MOD_Y_TILED: --- base-commit: 9a2cb1b31c040e2f1b313e2f7921f0f5e6b66d82 change-id: 20230524-intel_async_flip_check_hw-implicit-fallthrough-c4c40b03802f Best regards, -- Nathan Chancellor
Re: [Intel-gfx] next: clang: x86_64: /intel_display.c:6012:3: error: unannotated fall-through between switch labels [-Werror, -Wimplicit-fallthrough]
Hi Naresh, On Wed, May 24, 2023 at 12:32:24PM +0530, Naresh Kamboju wrote: > Linux next-20230523 and next-20230524 the x86_64 and i386 builds failed > with clang. > > Reported-by: Linux Kernel Functional Testing > > make --silent --keep-going \ > --jobs=8 O=/home/tuxbuild/.cache/tuxmake/builds/1/build ARCH=x86_64 \ > SRCARCH=x86 CROSS_COMPILE=x86_64-linux-gnu- \ > 'HOSTCC=sccache clang' 'CC=sccache clang' \ >LLVM=1 LLVM_IAS=1 > > drivers/gpu/drm/i915/display/intel_display.c:6012:3: error: > unannotated fall-through between switch labels > [-Werror,-Wimplicit-fallthrough] > case I915_FORMAT_MOD_X_TILED: > ^ > drivers/gpu/drm/i915/display/intel_display.c:6012:3: note: insert > 'break;' to avoid fall-through > case I915_FORMAT_MOD_X_TILED: > ^ > break; > 1 error generated. Thanks for the report, I have sent https://lore.kernel.org/20230524-intel_async_flip_check_hw-implicit-fallthrough-v1-1-83de89e37...@kernel.org/ for this. Cheers, Nathan
Re: [Intel-gfx] [PATCH] drm/i915: Fix clang -Wimplicit-fallthrough in intel_async_flip_check_hw()
On Wed, May 24, 2023 at 11:32:32AM -0700, Nick Desaulniers wrote: > On Wed, May 24, 2023 at 8:38 AM Nathan Chancellor wrote: > > > > Clang warns: > > > > drivers/gpu/drm/i915/display/intel_display.c:6012:3: error: unannotated > > fall-through between switch labels [-Werror,-Wimplicit-fallthrough] > > case I915_FORMAT_MOD_X_TILED: > > ^ > > drivers/gpu/drm/i915/display/intel_display.c:6012:3: note: insert > > 'break;' to avoid fall-through > > case I915_FORMAT_MOD_X_TILED: > > ^ > > break; > > 1 error generated. > > > > Clang is a little more pedantic than GCC, which does not warn when > > falling through to a case that is just break or return. Clang's version > > is more in line with the kernel's own stance in deprecated.rst, which > > states that all switch/case blocks must end in either break, > > fallthrough, continue, goto, or return. Add the missing break to silence > > the warning. > > > > Fixes: 937859485aef ("drm/i915: Support Async Flip on Linear buffers") > > Reported-by: kernel test robot > > Closes: https://lore.kernel.org/202305241902.uvhtmoxa-...@intel.com/ > > Reported-by: Naresh Kamboju > > Closes: > > https://lore.kernel.org/CA+G9fYv68V3ewK0Qj-syQj7qX-hQr0H1MFL=qfnudoe_j2z...@mail.gmail.com/ > > Signed-off-by: Nathan Chancellor > > Thanks for the patch! I've never seen the closes tag before, that's > new to me. Can you tell me more about it? It is new to me (at least in the context of the kernel) as well. I only used it over Link: because checkpatch.pl told me to: WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #26: Reported-by: kernel test robot Reported-by: Naresh Kamboju WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #27: Reported-by: Naresh Kamboju Signed-off-by: Nathan Chancellor It was Link: for a bit but commit 44c31888098a ("checkpatch: allow Closes tags with links") changed it to Closes:. Looks odd to me but whatever the linter says I suppose. Thanks for the review! Cheers, Nathan > A few more tags > > Reported-by: Tom Rix > Link: https://lore.kernel.org/all/20230523125116.1669057-1-t...@redhat.com/ > Reviewed-by: Nick Desaulniers > > > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index 0490c6412ab5..6d49e0ab3e85 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -6008,6 +6008,7 @@ static int intel_async_flip_check_hw(struct > > intel_atomic_state *state, struct in > > plane->base.base.id, > > plane->base.name); > > return -EINVAL; > > } > > + break; > > > > case I915_FORMAT_MOD_X_TILED: > > case I915_FORMAT_MOD_Y_TILED: > > > > --- > > base-commit: 9a2cb1b31c040e2f1b313e2f7921f0f5e6b66d82 > > change-id: > > 20230524-intel_async_flip_check_hw-implicit-fallthrough-c4c40b03802f > > > > Best regards, > > -- > > Nathan Chancellor > > > > > -- > Thanks, > ~Nick Desaulniers
[Intel-gfx] [PATCH 0/2] drm/i915/gt: Fix recent kCFI violations
Hi all, This series fixes a few clang kernel Control Flow Integrity (kCFI) violations that appear after commit 9275277d5324 ("drm/i915: use pat_index instead of cache_level"). They were found between run time testing on real hardware and compile time testing with -Wincompatible-function-pointer-types-strict (which is not yet enabled for the kernel but I build with it locally to catch new instances). If there are any problems or questions, please let me know. --- Nathan Chancellor (2): drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks drm/i915/gt: Fix parameter in gmch_ggtt_insert_{entries,page}() drivers/gpu/drm/i915/gt/intel_ggtt.c | 26 +- drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c | 8 2 files changed, 17 insertions(+), 17 deletions(-) --- base-commit: 08264f85c5c05ecc38d409c84d48cfb00ccd3bc4 change-id: 20230530-i915-gt-cache_level-wincompatible-function-pointer-types-strict-32a5c65249a5 Best regards, -- Nathan Chancellor
[Intel-gfx] [PATCH 2/2] drm/i915/gt: Fix parameter in gmch_ggtt_insert_{entries, page}()
When building with clang's -Wincompatible-function-pointer-types-strict, the following warnings occur: drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c:102:23: error: incompatible function pointer types assigning to 'void (*)(struct i915_address_space *, dma_addr_t, u64, unsigned int, u32)' (aka 'void (*)(struct i915_address_space *, unsigned int, unsigned long long, unsigned int, unsigned int)') from 'void (struct i915_address_space *, dma_addr_t, u64, enum i915_cache_level, u32)' (aka 'void (struct i915_address_space *, unsigned int, unsigned long long, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.insert_page = gmch_ggtt_insert_page; ^ ~ drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c:103:26: error: incompatible function pointer types assigning to 'void (*)(struct i915_address_space *, struct i915_vma_resource *, unsigned int, u32)' (aka 'void (*)(struct i915_address_space *, struct i915_vma_resource *, unsigned int, unsigned int)') from 'void (struct i915_address_space *, struct i915_vma_resource *, enum i915_cache_level, u32)' (aka 'void (struct i915_address_space *, struct i915_vma_resource *, enum i915_cache_level, unsigned int)') [-Werror, -Wincompatible-function-pointer-types-strict] ggtt->vm.insert_entries = gmch_ggtt_insert_entries; ^ 2 errors generated. The warning is pointing out that while 'enum i915_cache_level' and 'unsigned int' are ABI compatible, these indirect calls will fail clang's kernel Control Flow Integrity (kCFI) checks, as the callback's signature does not exactly match the prototype's signature. To fix this, replace the cache_level parameter with pat_index, as was done in other places within i915 where there is no difference between cache_level and pat_index on certain generations. Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c b/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c index d6a74ae2527b..866c416afb73 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c @@ -18,10 +18,10 @@ static void gmch_ggtt_insert_page(struct i915_address_space *vm, dma_addr_t addr, u64 offset, - enum i915_cache_level cache_level, + unsigned int pat_index, u32 unused) { - unsigned int flags = (cache_level == I915_CACHE_NONE) ? + unsigned int flags = (pat_index == I915_CACHE_NONE) ? AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY; intel_gmch_gtt_insert_page(addr, offset >> PAGE_SHIFT, flags); @@ -29,10 +29,10 @@ static void gmch_ggtt_insert_page(struct i915_address_space *vm, static void gmch_ggtt_insert_entries(struct i915_address_space *vm, struct i915_vma_resource *vma_res, -enum i915_cache_level cache_level, +unsigned int pat_index, u32 unused) { - unsigned int flags = (cache_level == I915_CACHE_NONE) ? + unsigned int flags = (pat_index == I915_CACHE_NONE) ? AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY; intel_gmch_gtt_insert_sg_entries(vma_res->bi.pages, vma_res->start >> PAGE_SHIFT, -- 2.40.1
[Intel-gfx] [PATCH 1/2] drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks
When booting a kernel compiled with CONFIG_CFI_CLANG (kCFI), there is a CFI failure in ggtt_probe_common() when trying to call hsw_pte_encode() via an indirect call: [5.030027] CFI failure at ggtt_probe_common+0xd1/0x130 [i915] (target: hsw_pte_encode+0x0/0x30 [i915]; expected type: 0xf5c1d0fc) With kCFI, indirect calls are validated against their expected type versus actual type and failures occur when the two types do not match. clang's -Wincompatible-function-pointer-types-strict can catch this at compile time but it is not enabled for the kernel yet: drivers/gpu/drm/i915/gt/intel_ggtt.c:1155:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = iris_pte_encode; ^ ~~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1157:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = hsw_pte_encode; ^ ~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1159:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = byt_pte_encode; ^ ~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1161:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = ivb_pte_encode; ^ ~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1163:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = snb_pte_encode; ^ ~~ 5 errors generated. In this case, the pre-gen8 pte_encode functions have a second parameter type of 'enum i915_cache_level' whereas the function pointer prototype in 'struct i915_address_space' expects a second parameter type of 'unsigned int'. Update the second parameter of the callbacks and the comment above them noting that these statements are still valid, which matches other functions and files, to clear up the kCFI failures at run time. Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gt/intel_ggtt.c | 26 +- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index 2a7942fac798..122197737ef2 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -1015,16 +1015,16 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt) /* * For pre-gen8 platforms pat_index is the same as enum i915_cache_level, - * so these PTE encode functions are left with using cache_level. + * so the switch-case statements in these PTE encode functions are still valid. * See translation table LEGACY_CACHELEVEL. */ static u64 snb_pte_encode(dma_addr_t addr, - enum i915_cache_level level, + unsigned int pat_index, u32 flags) { gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; - switch (level) { +
[Intel-gfx] [PATCH] drm/i915/pxp: Fix size_t format specifier in gsccs_send_message()
When building ARCH=i386 allmodconfig, the following warning occurs: In file included from include/linux/device.h:15, from include/linux/node.h:18, from include/linux/cpu.h:17, from include/linux/static_call.h:135, from arch/x86/include/asm/perf_event.h:5, from include/linux/perf_event.h:25, from drivers/gpu/drm/i915/i915_pmu.h:11, from drivers/gpu/drm/i915/gt/intel_engine_types.h:21, from drivers/gpu/drm/i915/gt/intel_context_types.h:18, from drivers/gpu/drm/i915/gem/i915_gem_context_types.h:20, from drivers/gpu/drm/i915/i915_request.h:34, from drivers/gpu/drm/i915/i915_active.h:13, from drivers/gpu/drm/i915/gt/intel_context.h:13, from drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:8: drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c: In function 'gsccs_send_message': include/drm/drm_print.h:456:39: error: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Werror=format=] 456 | dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__) | ^~~~ include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~ include/linux/dev_printk.h:146:61: note: in expansion of macro 'dev_fmt' 146 | dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~ include/drm/drm_print.h:456:9: note: in expansion of macro 'dev_warn' 456 | dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__) | ^~~~ include/drm/drm_print.h:466:9: note: in expansion of macro '__drm_printk' 466 | __drm_printk((drm), warn,, fmt, ##__VA_ARGS__) | ^~~~ drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:146:17: note: in expansion of macro 'drm_warn' 146 | drm_warn(&i915->drm, "caller with insufficient PXP reply size %u (%ld)\n", | ^~~~ cc1: all warnings being treated as errors Use the '%zu' format specifier, as the variable is a 'size_t'. Fixes: dc9ac125d81f ("drm/i915/pxp: Add GSC-CS backend to send GSC fw messages") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c index 8dc41de3f6f7..a217821eb0fb 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c @@ -143,7 +143,7 @@ gsccs_send_message(struct intel_pxp *pxp, reply_size = header->message_size - sizeof(*header); if (reply_size > msg_out_size_max) { - drm_warn(&i915->drm, "caller with insufficient PXP reply size %u (%ld)\n", + drm_warn(&i915->drm, "caller with insufficient PXP reply size %u (%zu)\n", reply_size, msg_out_size_max); reply_size = msg_out_size_max; } --- base-commit: 08264f85c5c05ecc38d409c84d48cfb00ccd3bc4 change-id: 20230530-i915-pxp-size_t-wformat-1d73ed1f8d23 Best regards, -- Nathan Chancellor
Re: [Intel-gfx] [PATCH 1/2] drm/i915: drop -Wall and related disables from cflags as redundant
On Fri, Oct 06, 2023 at 03:34:46PM +0300, Jani Nikula wrote: > The kernel top level Makefile, and recently scripts/Makefile.extrawarn, > have included -Wall, and the disables -Wno-format-security and > $(call cc-disable-warning,frame-address,) for a very long time. They're > redundant in our local subdir-ccflags-y and can be dropped. > > Cc: Arnd Bergmann > Cc: Nick Desaulniers > Cc: Nathan Chancellor > Cc: Masahiro Yamada > Signed-off-by: Jani Nikula Yeah, this seems totally reasonable. I always assumed the intention of -Wall was to re-enable some warnings that the rest of the kernel had turned off but I think we are getting better about auditing what warnings are explicitly turned off and getting some of those turned back on for the whole kernel, so I expect this to basically be a no-op. Reviewed-by: Nathan Chancellor > --- > drivers/gpu/drm/i915/Makefile | 8 +++- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index dec78efa452a..623f81217442 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -5,22 +5,20 @@ > > # Add a set of useful warning flags and enable -Werror for CI to prevent > # trivial mistakes from creeping in. We have to do this piecemeal as we > reject > -# any patch that isn't warning clean, so turning on -Wall -Wextra (or W=1) we > +# any patch that isn't warning clean, so turning on -Wextra (or W=1) we > # need to filter out dubious warnings. Still it is our interest > # to keep running locally with W=1 C=1 until we are completely clean. > # > -# Note the danger in using -Wall -Wextra is that when CI updates gcc we > +# Note the danger in using -Wextra is that when CI updates gcc we > # will most likely get a sudden build breakage... Hopefully we will fix > # new warnings before CI updates! > -subdir-ccflags-y := -Wall -Wextra > -subdir-ccflags-y += -Wno-format-security > +subdir-ccflags-y := -Wextra > subdir-ccflags-y += -Wno-unused-parameter > subdir-ccflags-y += -Wno-type-limits > subdir-ccflags-y += -Wno-missing-field-initializers > subdir-ccflags-y += -Wno-sign-compare > subdir-ccflags-y += -Wno-shift-negative-value > subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > -subdir-ccflags-y += $(call cc-disable-warning, frame-address) > subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror > > # Fine grained warnings disable > -- > 2.39.2 >
Re: [Intel-gfx] [PATCH 2/2] drm/i915: enable W=1 warnings by default
On Fri, Oct 06, 2023 at 03:34:47PM +0300, Jani Nikula wrote: > We enable a bunch more compiler warnings than the kernel > defaults. However, they've drifted to become a unique set of warnings, > and have increasingly fallen behind from the W=1 set. > > Align with the W=1 warnings from scripts/Makefile.extrawarn for clarity, > by copy-pasting them with s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it > easier to compare in the future. > > Cc: Arnd Bergmann > Cc: Nick Desaulniers > Cc: Nathan Chancellor > Cc: Masahiro Yamada > Signed-off-by: Jani Nikula One meta comment and review comment below. Feel free to carry forward Reviewed-by: Nathan Chancellor on future revisions. > --- > > An alternative or future option would be to have Makefile.extrawarn > assign W=1 etc. flags to intermediate variables, say KBUILD_CFLAGS_W1, > like this: > > KBUILD_CFLAGS_W1 += -Wextra -Wunused -Wno-unused-parameter > etc... > > export KBUILD_CFLAGS_W1 > > ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),) > > KBUILD_CFLAGS += $(KBUILD_CFLAGS_W1) > > else > etc... > > and then drivers and subsystems could simply use: > > subdir-ccflags-y += $(KBUILD_CFLAGS_W1) > > to enable and remain up-to-date with W=1 warnings. This has definitely come up a few times and while I am generally in favor of something like this, it makes adding warnings to W=1 a little bit harder because when we add warnings to W=1, we typically are not concerned with breaking the build, as W=1 is not the default build. If a subsystem has opted into "whatever the current W=1 is" by default, changes to W=1 will have to be reviewed/tested within a normal build. Doing something like this patch with a more regular cadence (maybe every update after the merge window) seems like a reasonable compromise to me, although I know that means more work for individual subsystem maintainers. > --- > drivers/gpu/drm/i915/Makefile | 33 ++--- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 623f81217442..0485157054fc 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -3,22 +3,25 @@ > # Makefile for the drm device driver. This driver provides support for the > # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher. > > -# Add a set of useful warning flags and enable -Werror for CI to prevent > -# trivial mistakes from creeping in. We have to do this piecemeal as we > reject > -# any patch that isn't warning clean, so turning on -Wextra (or W=1) we > -# need to filter out dubious warnings. Still it is our interest > -# to keep running locally with W=1 C=1 until we are completely clean. > -# > -# Note the danger in using -Wextra is that when CI updates gcc we > -# will most likely get a sudden build breakage... Hopefully we will fix > -# new warnings before CI updates! > -subdir-ccflags-y := -Wextra > -subdir-ccflags-y += -Wno-unused-parameter > -subdir-ccflags-y += -Wno-type-limits > -subdir-ccflags-y += -Wno-missing-field-initializers > -subdir-ccflags-y += -Wno-sign-compare > -subdir-ccflags-y += -Wno-shift-negative-value As the test robot points out, you'll want to keep these four, as they are only enabled for W=2 or W=3. With this diff on top of these two patches: diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 0485157054fc..9c4e09c8aa4e 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -21,6 +21,12 @@ subdir-ccflags-y += $(call cc-option, -Wstringop-overflow) subdir-ccflags-y += $(call cc-option, -Wstringop-truncation) # --- end copy-paste +# The following turn off the warnings enabled by -Wextra +subdir-ccflags-y += -Wno-type-limits +subdir-ccflags-y += -Wno-missing-field-initializers +subdir-ccflags-y += -Wno-sign-compare +subdir-ccflags-y += -Wno-shift-negative-value + # Enable -Werror in CI and development subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror I can continue to build i915 warning free with ARCH=x86_64 allmodconfig using all supported versions of LLVM for building the kernel. > +# Unconditionally enable W=1 warnings locally > +# --- begin copy-paste W=1 warnings from scripts/Makefile.extrawarn > +subdir-ccflags-y += -Wextra -Wunused -Wno-unused-parameter > +subdir-ccflags-y += -Wmissing-declarations > +subdir-ccflags-y += $(call cc-option, -Wrestrict) > +subdir-ccflags-y += -Wmissing-format-attribute > +subdir-ccflags-y += -Wmissing-prototypes > +subdir-ccflags-y += -Wold-style-definition > +subdir-ccflags-y += -Wmissing-include-dirs > subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > +subdir-ccflags-y += $(call cc-option, -Wunused-co
Re: [Intel-gfx] [PATCH 1/2] drm/i915: drop -Wall and related disables from cflags as redundant
On Sun, Oct 08, 2023 at 12:28:46AM +0900, Masahiro Yamada wrote: > On Fri, Oct 6, 2023 at 9:35 PM Jani Nikula wrote: > > > > The kernel top level Makefile, and recently scripts/Makefile.extrawarn, > > have included -Wall, and the disables -Wno-format-security and > > $(call cc-disable-warning,frame-address,) for a very long time. They're > > redundant in our local subdir-ccflags-y and can be dropped. > > > > Cc: Arnd Bergmann > > Cc: Nick Desaulniers > > Cc: Nathan Chancellor > > Cc: Masahiro Yamada > > Signed-off-by: Jani Nikula > > > I made a similar suggestion in the past > https://lore.kernel.org/dri-devel/20190515043753.9853-1-yamada.masah...@socionext.com/ > > So, I am glad that Intel has decided to de-duplicate the flags. > > > > I think you can drop more flags. > > For example, > > subdir-ccflags-y += -Wno-sign-compare > > > It is set by scripts/Makefile.extrawarn > unless W=3 is passed. > > > If W=3 is set by a user, -Wsign-compare should be warned > as it is the user's request. > > > drivers/gpu/drm/i915/Makefile negates W=3. > There is no good reason to do so. > > > Same applied to > > > subdir-ccflags-y += -Wno-shift-negative-value As I point out in my review of the second patch [1], I am not sure these should be dropped because -Wextra turns these warnings back on, at least for clang according to this build report [2] and my own testing, so they need to be disabled again. [1]: https://lore.kernel.org/20231006174550.GC3359308@dev-arch.thelio-3990X/ [2]: https://lore.kernel.org/202310070011.fji48ibk-...@intel.com/ Cheers, Nathan.
Re: [Intel-gfx] [PATCH v10 11/15] drm/atomic-helper: Set fence deadline for vblank
Hi Rob, On Wed, Mar 08, 2023 at 07:53:02AM -0800, Rob Clark wrote: > From: Rob Clark > > For an atomic commit updating a single CRTC (ie. a pageflip) calculate > the next vblank time, and inform the fence(s) of that deadline. > > v2: Comment typo fix (danvet) > v3: If there are multiple CRTCs, consider the time of the soonest vblank > > Signed-off-by: Rob Clark > Reviewed-by: Daniel Vetter > Signed-off-by: Rob Clark I apologize if this has already been reported or fixed, I searched lore but did not find anything. This change as commit d39e48ca80c0 ("drm/atomic-helper: Set fence deadline for vblank") in -next causes a hang while running LTP's read_all test on /proc on my Ampere Altra system (it seems it is hanging on a pagemap file?). Additionally, I have this splat in dmesg, which seems related based on the call stack. [ 20.542591] fbcon: Taking over console [ 20.550772] Unable to handle kernel NULL pointer dereference at virtual address 0074 [ 20.550776] Mem abort info: [ 20.550777] ESR = 0x9604 [ 20.550779] EC = 0x25: DABT (current EL), IL = 32 bits [ 20.550781] SET = 0, FnV = 0 [ 20.550782] EA = 0, S1PTW = 0 [ 20.550784] FSC = 0x04: level 0 translation fault [ 20.550785] Data abort info: [ 20.550786] ISV = 0, ISS = 0x0004 [ 20.550788] CM = 0, WnR = 0 [ 20.550789] user pgtable: 4k pages, 48-bit VAs, pgdp=080009d16000 [ 20.550791] [0074] pgd=, p4d= [ 20.550796] Internal error: Oops: 9604 [#1] SMP [ 20.550800] Modules linked in: ip6table_nat tun nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables nfnetlink qrtr sunrpc binfmt_misc vfat fat xfs snd_usb_audio snd_hwdep snd_usbmidi_lib snd_seq snd_pcm snd_rawmidi snd_timer snd_seq_device snd soundcore joydev mc ipmi_ssif ipmi_devintf ipmi_msghandler arm_spe_pmu arm_cmn arm_dsu_pmu arm_dmc620_pmu cppc_cpufreq loop zram crct10dif_ce polyval_ce nvme polyval_generic ghash_ce sbsa_gwdt igb nvme_core ast nvme_common i2c_algo_bit xgene_hwmon gpio_dwapb scsi_dh_rdac scsi_dh_emc scsi_dh_alua ip6_tables ip_tables dm_multipath fuse [ 20.550869] CPU: 12 PID: 469 Comm: kworker/12:1 Not tainted 6.3.0-rc2-8-gd39e48ca80c0 #1 [ 20.550872] Hardware name: ADLINK AVA Developer Platform/AVA Developer Platform, BIOS TianoCore 2.04.100.07 (SYS: 2.06.20220308) 09/08/2022 [ 20.550875] Workqueue: events fbcon_register_existing_fbs [ 20.550884] pstate: 2049 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 20.550888] pc : drm_crtc_next_vblank_start+0x2c/0x98 [ 20.550894] lr : drm_atomic_helper_wait_for_fences+0x90/0x240 [ 20.550898] sp : 8d583960 [ 20.550900] x29: 8d583960 x28: 07ff8fc187b0 x27: [ 20.550904] x26: 07ff99c08c00 x25: 0038 x24: 07ff99c0c000 [ 20.550908] x23: 0001 x22: 0038 x21: [ 20.550912] x20: 07ff9640a280 x19: x18: [ 20.550915] x17: x16: b24d2eece1c0 x15: 003038303178 [ 20.550919] x14: 303239310048 x13: x12: [ 20.550923] x11: x10: x9 : b24d2eeeaca0 [ 20.550926] x8 : 8d583628 x7 : 080077783000 x6 : [ 20.550930] x5 : 8d584000 x4 : 07ff99c0c000 x3 : 0130 [ 20.550934] x2 : x1 : 8d5839c0 x0 : 07ff99c0cc08 [ 20.550937] Call trace: [ 20.550939] drm_crtc_next_vblank_start+0x2c/0x98 [ 20.550942] drm_atomic_helper_wait_for_fences+0x90/0x240 [ 20.550946] drm_atomic_helper_commit+0xb0/0x188 [ 20.550949] drm_atomic_commit+0xb0/0xf0 [ 20.550953] drm_client_modeset_commit_atomic+0x218/0x280 [ 20.550957] drm_client_modeset_commit_locked+0x64/0x1a0 [ 20.550961] drm_client_modeset_commit+0x38/0x68 [ 20.550965] __drm_fb_helper_restore_fbdev_mode_unlocked+0xb0/0xf8 [ 20.550970] drm_fb_helper_set_par+0x44/0x88 [ 20.550973] fbcon_init+0x1e0/0x4a8 [ 20.550976] visual_init+0xbc/0x118 [ 20.550981] do_bind_con_driver.isra.0+0x194/0x3a0 [ 20.550984] do_take_over_console+0x50/0x70 [ 20.550987] do_fbcon_takeover+0x74/0xf8 [ 20.550989] do_fb_registered+0x13c/0x158 [ 20.550992] fbcon_register_existing_fbs+0x78/0xc0 [ 20.550995] process_one_work+0x1ec/0x478 [ 20.551000] worker_thread+0x74/0x418 [ 20.551002] kthread+0xec/0x100 [ 20.551005] ret_from_fork+0x10/0x20 [ 20.551011] Code: f944 b9409013 f940a082 9ba30a73 (b9407662) [ 20.551013] ---[ end trace ]--- If there is any additional information that I can provide or patches I can test, I am more than happy to do so. Cheers, Nathan # bad: [4b0f4525dc4fe8af17b3daefe585f0c2eb0fe0a5] Add linux-next specific files for
Re: [Intel-gfx] [PATCH v10 11/15] drm/atomic-helper: Set fence deadline for vblank
On Fri, Mar 31, 2023 at 03:14:30PM -0700, Rob Clark wrote: > On Fri, Mar 31, 2023 at 1:44 PM Nathan Chancellor wrote: > > > > Hi Rob, > > > > On Wed, Mar 08, 2023 at 07:53:02AM -0800, Rob Clark wrote: > > > From: Rob Clark > > > > > > For an atomic commit updating a single CRTC (ie. a pageflip) calculate > > > the next vblank time, and inform the fence(s) of that deadline. > > > > > > v2: Comment typo fix (danvet) > > > v3: If there are multiple CRTCs, consider the time of the soonest vblank > > > > > > Signed-off-by: Rob Clark > > > Reviewed-by: Daniel Vetter > > > Signed-off-by: Rob Clark > > > > I apologize if this has already been reported or fixed, I searched lore > > but did not find anything. > > > > This change as commit d39e48ca80c0 ("drm/atomic-helper: Set fence > > deadline for vblank") in -next causes a hang while running LTP's > > read_all test on /proc on my Ampere Altra system (it seems it is hanging > > on a pagemap file?). Additionally, I have this splat in dmesg, which > > seems related based on the call stack. > > Hi, I'm not familiar with this hardware.. do you know which drm driver > is used? I can't tell from the call-stack. I think it is drivers/gpu/drm/ast, as I see ast in lsmod? > > [ 20.542591] fbcon: Taking over console > > [ 20.550772] Unable to handle kernel NULL pointer dereference at virtual > > address 0074 > > [ 20.550776] Mem abort info: > > [ 20.550777] ESR = 0x9604 > > [ 20.550779] EC = 0x25: DABT (current EL), IL = 32 bits > > [ 20.550781] SET = 0, FnV = 0 > > [ 20.550782] EA = 0, S1PTW = 0 > > [ 20.550784] FSC = 0x04: level 0 translation fault > > [ 20.550785] Data abort info: > > [ 20.550786] ISV = 0, ISS = 0x0004 > > [ 20.550788] CM = 0, WnR = 0 > > [ 20.550789] user pgtable: 4k pages, 48-bit VAs, pgdp=080009d16000 > > [ 20.550791] [0074] pgd=, p4d= > > [ 20.550796] Internal error: Oops: 9604 [#1] SMP > > [ 20.550800] Modules linked in: ip6table_nat tun nft_fib_inet > > nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 > > nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack > > nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables nfnetlink qrtr sunrpc > > binfmt_misc vfat fat xfs snd_usb_audio snd_hwdep snd_usbmidi_lib snd_seq > > snd_pcm snd_rawmidi snd_timer snd_seq_device snd soundcore joydev mc > > ipmi_ssif ipmi_devintf ipmi_msghandler arm_spe_pmu arm_cmn arm_dsu_pmu > > arm_dmc620_pmu cppc_cpufreq loop zram crct10dif_ce polyval_ce nvme > > polyval_generic ghash_ce sbsa_gwdt igb nvme_core ast nvme_common > > i2c_algo_bit xgene_hwmon gpio_dwapb scsi_dh_rdac scsi_dh_emc scsi_dh_alua > > ip6_tables ip_tables dm_multipath fuse > > [ 20.550869] CPU: 12 PID: 469 Comm: kworker/12:1 Not tainted > > 6.3.0-rc2-8-gd39e48ca80c0 #1 > > [ 20.550872] Hardware name: ADLINK AVA Developer Platform/AVA Developer > > Platform, BIOS TianoCore 2.04.100.07 (SYS: 2.06.20220308) 09/08/2022 > > [ 20.550875] Workqueue: events fbcon_register_existing_fbs > > [ 20.550884] pstate: 2049 (nzCv daif +PAN -UAO -TCO -DIT -SSBS > > BTYPE=--) > > [ 20.550888] pc : drm_crtc_next_vblank_start+0x2c/0x98 > > [ 20.550894] lr : drm_atomic_helper_wait_for_fences+0x90/0x240 > > [ 20.550898] sp : 8d583960 > > [ 20.550900] x29: 8d583960 x28: 07ff8fc187b0 x27: > > > > [ 20.550904] x26: 07ff99c08c00 x25: 0038 x24: > > 07ff99c0c000 > > [ 20.550908] x23: 0001 x22: 0038 x21: > > > > [ 20.550912] x20: 07ff9640a280 x19: x18: > > > > [ 20.550915] x17: x16: b24d2eece1c0 x15: > > 003038303178 > > [ 20.550919] x14: 303239310048 x13: x12: > > > > [ 20.550923] x11: x10: x9 : > > b24d2eeeaca0 > > [ 20.550926] x8 : 8d583628 x7 : 080077783000 x6 : > > > > [ 20.550930] x5 : 8d584000 x4 : 07ff99c0c000 x3 : > > 0130 > > [ 20.550934] x2 : x1 : 8d5839c0 x0 : > > 07ff99c0cc08 > > [ 20.550937] Call trace: > > [ 20.550939] drm_crtc_next_vblank_start+0x2c/0x98 > > [ 20.550942] drm_atomic_helper_wait_for_fences+0
Re: [Intel-gfx] linux-next: manual merge of the drm-misc tree with the mm-stable tree
On Wed, Apr 19, 2023 at 06:24:37PM +0200, Daniel Vetter wrote: > On Tue, Apr 18, 2023 at 07:34:44PM +0100, Mark Brown wrote: > > On Sun, Apr 16, 2023 at 09:58:50AM +0200, Daniel Vetter wrote: > > > > > Note there was a ppc compile fail, which is why we pushed the ttm revert. > > > That /should/ be fixed now, but would be good if you can confirm? > > > > According to Nathan (CCed) there's still issues with the interaction > > with the PowerPC tree. > > So this revert was supposed to fix this: 56e51681246e ("drm/ttm: revert > "Reduce the number of used allocation orders for TTM pages"") > > If there's anything left then I need to chase that asap since the merge > window will open soon. I think we are talking about two different issues here. My issue is not a compilation failure, it is an incorrect merge resolution that is happening in -next because of two independent changes in the drm and powerpc tree, the thread below should have more information. https://lore.kernel.org/20230413184725.GA3183133@dev-arch.thelio-3990X/ I do not think this is something that either tree can solve independently of each other, -next has to resolve the conflict correctly (which is what I point out in the message above) and a note of it should be passed along to Linus so it can be resolved correctly in mainline when the time comes. Cheers, Nathan
Re: [Intel-gfx] [PATCH v4 18/21] compiler.h: RFC - s/__LINE__/__COUNTER__/ in __UNIQUE_ID fallback
Hi Jim On Thu, Jul 13, 2023 at 10:36:23AM -0600, Jim Cromie wrote: > We currently have 3 defns for __UNIQUE_ID(); gcc and clang are using > __COUNTER__ for real uniqueness, 3rd just uses __LINE__, which should > fail on this (and harder to avoid situations): > > DECLARE_FOO(); DECLARE_FOO(); > > Its 2023, can we haz a no-fallback __UNIQUE_ID ? Yeah, I fail to see how this fallback definition can actually be used after commit 95207db8166a ("Remove Intel compiler support"); even before that, it would be pretty unlikely since icc usage has not been visible for a long time. The kernel only officially supports clang or GCC now, so the definitions of __UNIQUE_ID() in include/linux/compiler-clang.h and include/linux/compiler-gcc.h should always be used because of the include in include/linux/compiler_types.h, right? I think the correct clean up is to just hoist the definition of __UNIQUE_ID() out of the individual compiler headers into the common one here but... > NOTE: > > This also changes __UNIQUE_ID_ to _kaUID_. Ive been getting > lkp-reports of collisions on names which should be unique; this > shouldnt happen on gcc & clang, but does on some older ones, on some > platforms, on some allyes & rand-configs. Like this: > > mips64-linux-ld: > drivers/gpu/drm/display/drm_dp_helper.o:(__dyndbg_class_users+0x0): > multiple definition of `__UNIQUE_ID_ddebug_class_user405'; > drivers/gpu/drm/drm_gem_shmem_helper.o:(__dyndbg_class_users+0x0): > first defined here This problem cannot be addressed with this patch given the above information, no? Seems like that might mean that __COUNTER__ has issues in earlier compilers? Cheers, Nathan > Like above, the collision reports appear to always be 3-digit > counters, which look like line-numbers. Changing to _kaUID_ in this > defn should make it more obvious (in *.i file) when a fallback has > happened. To be clear, I havent seen it yet. Nor have I seen the > multiple-defn problem above since adding this patch. > > Lets see what lkp-robot says about this. > > CC: Luc Van Oostenryck (maintainer:SPARSE > CHECKER) > CC: Nathan Chancellor (supporter:CLANG/LLVM BUILD SUPPORT) > CC: Nick Desaulniers (supporter:CLANG/LLVM BUILD > SUPPORT) > CC: Tom Rix (reviewer:CLANG/LLVM BUILD SUPPORT) > CC: linux-spa...@vger.kernel.org (open list:SPARSE CHECKER) > CC: linux-ker...@vger.kernel.org (open list) > CC: l...@lists.linux.dev (open list:CLANG/LLVM BUILD SUPPORT) > Signed-off-by: Jim Cromie > --- > include/linux/compiler.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index d7779a18b24f..677d6c47cd9e 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -177,9 +177,9 @@ void ftrace_likely_update(struct ftrace_likely_data *f, > int val, > __asm__ ("" : "=r" (var) : "0" (var)) > #endif > > -/* Not-quite-unique ID. */ > +/* JFTI: to fix Not-quite-unique ID */ > #ifndef __UNIQUE_ID > -# define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __LINE__) > +# define __UNIQUE_ID(prefix) __PASTE(__PASTE(_kaUID_, prefix), __COUNTER__) > #endif > > /** > -- > 2.41.0 >
[Intel-gfx] [PATCH 2/2] drm/i915: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
A proposed update to clang's -Wconstant-logical-operand to warn when the left hand side is a constant shows the following instance in nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, such as CONFIG_HZ=300: drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] 189 | if (NSEC_PER_SEC % HZ && | ~ ^ drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a bitwise operation 189 | if (NSEC_PER_SEC % HZ && | ^~ | & drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant to silence this warning 1 warning generated. Turn this into an explicit comparison against zero to make the expression a boolean to make it clear this should be a logical check, not a bitwise one. Link: https://reviews.llvm.org/D142609 Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index 4a33ad2d122b..d4b918fb11ce 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -186,7 +186,7 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj, static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) { /* nsecs_to_jiffies64() does not guard against overflow */ - if (NSEC_PER_SEC % HZ && + if ((NSEC_PER_SEC % HZ) != 0 && div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ) return MAX_JIFFY_OFFSET; -- 2.41.0
[Intel-gfx] [PATCH 0/2] Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
Hi all, A proposed update to clang's -Wconstant-logical-operand [1] to warn when the left hand side is a constant as well now triggers with the modulo expression in nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, such as CONFIG_HZ=300: drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] 189 | if (NSEC_PER_SEC % HZ && | ~ ^ drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a bitwise operation 189 | if (NSEC_PER_SEC % HZ && | ^~ | & drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant to silence this warning 1 warning generated. In file included from drivers/gpu/drm/v3d/v3d_debugfs.c:12: drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] 343 | if (NSEC_PER_SEC % HZ && | ~ ^ drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise operation 343 | if (NSEC_PER_SEC % HZ && | ^~ | & drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence this warning 1 warning generated. These patches add an explicit comparison to zero to make the expression a boolean, which clears up the warning. The patches have no real dependency on each other but I felt like they made send together since it is the same code. If these could go into mainline sooner rather than later to avoid breaking builds that can hit this with CONFIG_WERROR, that would be nice, but I won't insist since I don't think our own CI has builds that has those conditions, but others might. --- Nathan Chancellor (2): drm/v3d: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout() drm/i915: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout() drivers/gpu/drm/i915/gem/i915_gem_wait.c | 2 +- drivers/gpu/drm/v3d/v3d_drv.h| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) --- base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c change-id: 20230718-nsecs_to_jiffies_timeout-constant-logical-operand-4a944690f3e9 Best regards, -- Nathan Chancellor
[Intel-gfx] [PATCH 1/2] drm/v3d: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
A proposed update to clang's -Wconstant-logical-operand to warn when the left hand side is a constant shows the following instance in nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, such as CONFIG_HZ=300: In file included from drivers/gpu/drm/v3d/v3d_debugfs.c:12: drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] 343 | if (NSEC_PER_SEC % HZ && | ~ ^ drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise operation 343 | if (NSEC_PER_SEC % HZ && | ^~ | & drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence this warning 1 warning generated. Turn this into an explicit comparison against zero to make the expression a boolean to make it clear this should be a logical check, not a bitwise one. Link: https://reviews.llvm.org/D142609 Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/v3d/v3d_drv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index b74b1351bfc8..7f664a4b2a75 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -340,7 +340,7 @@ struct v3d_submit_ext { static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) { /* nsecs_to_jiffies64() does not guard against overflow */ - if (NSEC_PER_SEC % HZ && + if ((NSEC_PER_SEC % HZ) != 0 && div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ) return MAX_JIFFY_OFFSET; -- 2.41.0
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
On Thu, Jul 20, 2023 at 09:43:05AM +0100, Tvrtko Ursulin wrote: > > On 18/07/2023 22:44, Nathan Chancellor wrote: > > A proposed update to clang's -Wconstant-logical-operand to warn when the > > left hand side is a constant shows the following instance in > > nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, > > such as CONFIG_HZ=300: > > > >drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical > > '&&' with constant operand [-Wconstant-logical-operand] > > 189 | if (NSEC_PER_SEC % HZ && > > | ~ ^ > >drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a > > bitwise operation > > 189 | if (NSEC_PER_SEC % HZ && > > | ^~ > > | & > >drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant > > to silence this warning > >1 warning generated. > > > > Turn this into an explicit comparison against zero to make the > > expression a boolean to make it clear this should be a logical check, > > not a bitwise one. > > So -Wconstant-logical-operand only triggers when it is a > constant but not zero constant? Why does that make sense is not > a kludge to avoid too much noise? Yes, the warning purposefully does not trigger when the constant is a 1 or 0 (as those are usually indicative of an intentional logical operation): https://github.com/llvm/llvm-project/blob/dfdfd306cfaf54fbc43e2d5eb36489dac3eb9976/clang/lib/Sema/SemaExpr.cpp#L13917-L13919 In this case, it is 100, so I kind of understand why this might be ambiguous to the compiler. > Personally, it all feels a bit over the top as a warning, > since code in both cases should optimise away. And we may end I do not necessarily disagree, as you can see from the differential review that I linked in the message, but I also understand it is a fine line to tread when writing compiler warnings between wanting to catch as many potential problems as possible and having too much noise for developers to sift through. I think this is erring on the side of caution. > up papering over it if it becomes a default. diagtool tree tells me this warning is already on by default. > Then again this patch IMO does make the code more readable, so I think so too. > I am happy to take this one via our tree. Or either give ack to > bring it in via drm-misc-next: > > Acked-by: Tvrtko Ursulin > > Let me know which route works best. Thanks for the feedback! Either route is fine with me but if the v3d patch is going to go in via drm-misc-next, it seems like it would not be too much trouble to push this one with it. Cheers, Nathan
Re: [Intel-gfx] [RFC] drm: enable W=1 warnings by default across the subsystem
On Thu, Nov 30, 2023 at 10:52:17AM +0200, Jani Nikula wrote: > On Wed, 29 Nov 2023, Hamza Mahfooz wrote: > > Cc: Nathan Chancellor > > > > On 11/29/23 13:12, Jani Nikula wrote: > >> At least the i915 and amd drivers enable a bunch more compiler warnings > >> than the kernel defaults. > >> > >> Extend the W=1 warnings to the entire drm subsystem by default. Use the > >> copy-pasted warnings from scripts/Makefile.extrawarn with > >> s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it easier to compare and keep > >> up with them in the future. > >> > >> This is similar to the approach currently used in i915. > >> > >> Some of the -Wextra warnings do need to be disabled, just like in > >> Makefile.extrawarn, but take care to not disable them for W=2 or W=3 > >> builds, depending on the warning. > > > > I think this should go in after drm-misc-next has a clean build (for > > COMPILE_TEST builds) with this patch applied. Otherwise, it will break a > > lot of build configs. > > Oh, I'm absolutely not suggesting this should be merged before known > warnings have been addressed one way or another. Either by fixing them > or by disabling said warning in driver local Makefiles, depending on the > case. > > While I'm suggesting this, I obviously (I hope) can't take on fixing all > the warnings this exposes. One way to scale would be for folks to apply > this locally, see what it uncovers in their drivers, and fix some. > > As an intermediate step we might also apply this to a topic branch in > drm-tip, so you'd see the warnings when building drm-tip, but not in > indidual branches or in anything that's merged upstream. For what it's worth, I ran this through my personal build matrix with LLVM/clang and it only found a few instances of -Wunused-but-set-variable: drivers/gpu/drm/mediatek/mtk_disp_gamma.c:121:6: warning: variable 'cfg_val' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/nouveau/nouveau_svm.c:115:24: warning: variable 'priority' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/nouveau/nouveau_svm.c:929:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c:221:7: warning: variable 'loc' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/qxl/qxl_cmd.c:424:6: warning: variable 'count' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/qxl/qxl_ioctl.c:148:14: warning: variable 'num_relocs' set but not used [-Wunused-but-set-variable] I know that clang does not support all the warnings that are being enabled here but I suspect there won't be too many instances of the other warnings. -Wformat-overflow and -Wformat-truncation might be the big ones. I know -Wstringop-overflow is being turned on globally in -next so that is one that you shouldn't have to worry much about. Cheers, Nathan
[Intel-gfx] [PATCH] drm/i915: Remove tautological compare in eb_relocate_vma
-Wtautological-compare was recently added to -Wall in LLVM, which exposed an if statement in i915 that is always false: ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: result of comparison of constant 576460752303423487 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare] if (unlikely(remain > N_RELOC(ULONG_MAX))) ^ Since remain is an unsigned int, it can never be larger than UINT_MAX, which is less than ULONG_MAX / sizeof(struct drm_i915_gem_relocation_entry). Remove this statement to fix the warning. Fixes: 2889caa92321 ("drm/i915: Eliminate lots of iterations over the execobjects array") Link: https://github.com/ClangBuiltLinux/linux/issues/778 Link: https://github.com/llvm/llvm-project/commit/9740f9f0b6e5d7d5104027aee7edc9c5202dd052 Signed-off-by: Nathan Chancellor --- NOTE: Another possible fix for this is to change ULONG_MAX to UINT_MAX but I am not sure that is what was intended by this check. I'm happy to respin if that is the case. drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index f0998f1225af..9ed4379b4bc8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1482,8 +1482,6 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct i915_vma *vma) urelocs = u64_to_user_ptr(entry->relocs_ptr); remain = entry->relocation_count; - if (unlikely(remain > N_RELOC(ULONG_MAX))) - return -EINVAL; /* * We must check that the entire relocation array is safe -- 2.24.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [v2] drm/i915/perf: Reverse a ternary to make sparse happy
On Fri, Nov 01, 2019 at 07:21:16PM +, Chris Wilson wrote: > Avoid > > drivers/gpu/drm/i915/i915_perf.c:2442:85: warning: dubious: x | !y > > simply by inverting the predicate and reversing the ternary. > > v2: More the long lines into their own function so there is no confusion > on operator precedence. > > Signed-off-by: Chris Wilson > Cc: Umesh Nerlige Ramappa > Cc: Lionel Landwerlin I don't see this applied to drm-intel yet (unless I am blind) and clang warns about this as well. Reviewed-by: Nathan Chancellor ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Enable -Wuninitialized
-Wunintialized was disabled in commit c5627461490e ("drm/i915: Disable -Wuninitialized") because there were two warnings that were false positives. The first was due to DECLARE_WAIT_QUEUE_HEAD_ONSTACK, which was fixed in LLVM 9.0.0. The second was in busywait_stop, which was fixed in LLVM 10.0.0 (issue 415). The kernel's minimum version for LLVM is 10.0.1 so this warning can be safely enabled, where it has already caught a couple bugs. Link: https://github.com/ClangBuiltLinux/linux/issues/220 Link: https://github.com/ClangBuiltLinux/linux/issues/415 Link: https://github.com/ClangBuiltLinux/linux/issues/499 Link: https://github.com/llvm/llvm-project/commit/2e040398f8d691cc378c1abb098824ff49f3f28f Link: https://github.com/llvm/llvm-project/commit/c667cdc850c2aa821ffeedbc08c24bc985c59edd Fixes: c5627461490e ("drm/i915: Disable -Wuninitialized") References: 2ea4a7ba9bf6 ("drm/i915/gt: Avoid uninitialized use of rpcurupei in frequency_show") References: 2034c2129bc4 ("drm/i915/display: Ensure that ret is always initialized in icl_combo_phy_verify_state") Reported-by: Arnd Bergmann Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 6d9e81ea67f4..60b60204004f 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -21,7 +21,6 @@ subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable) subdir-ccflags-y += $(call cc-disable-warning, sign-compare) subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized) subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) -subdir-ccflags-y += $(call cc-disable-warning, uninitialized) subdir-ccflags-y += $(call cc-disable-warning, frame-address) subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror base-commit: f40ddce88593482919761f74910f42f4b84c004b -- 2.30.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/display: Ensure that ret is always initialized in icl_combo_phy_verify_state
Clang warns: drivers/gpu/drm/i915/display/intel_combo_phy.c:268:3: warning: variable 'ret' is uninitialized when used here [-Wuninitialized] ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_LN0(phy), ^~~ drivers/gpu/drm/i915/display/intel_combo_phy.c:261:10: note: initialize the variable 'ret' to silence this warning bool ret; ^ = 0 1 warning generated. In practice, the bug this warning appears to be concerned with would not actually matter because ret gets initialized to the return value of cnl_verify_procmon_ref_values. However, that does appear to be a bug since it means the first hunk of the patch this fixes won't actually do anything (since the values of check_phy_reg won't factor into the final ret value). Initialize ret to true then make all of the assignments a bitwise AND with itself so that the function always does what it should do. Fixes: 239bef676d8e ("drm/i915/display: Implement new combo phy initialization step") Link: https://github.com/ClangBuiltLinux/linux/issues/1094 Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/display/intel_combo_phy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c index eccaa79cb4a9..a4b8aa6d0a9e 100644 --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c @@ -258,7 +258,7 @@ static bool phy_is_master(struct drm_i915_private *dev_priv, enum phy phy) static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv, enum phy phy) { - bool ret; + bool ret = true; u32 expected_val = 0; if (!icl_combo_phy_enabled(dev_priv, phy)) @@ -276,7 +276,7 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv, DCC_MODE_SELECT_CONTINUOSLY); } - ret = cnl_verify_procmon_ref_values(dev_priv, phy); + ret &= cnl_verify_procmon_ref_values(dev_priv, phy); if (phy_is_master(dev_priv, phy)) { ret &= check_phy_reg(dev_priv, phy, ICL_PORT_COMP_DW8(phy), base-commit: ca0e494af5edb59002665bf12871e94b4163a257 -- 2.28.0.rc0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/display: Ensure that ret is always initialized in icl_combo_phy_verify_state
Hi Jose, On Tue, Jul 28, 2020 at 01:47:56AM +, Souza, Jose wrote: > Hi Nathan > > Are you planning to check this regressions and send another version with the > fix?Or can I do it on top of your patch? Unfortunately, I have had little time for kernel work (full time retail worker in the middle of a pandemic plus full time student is a real fun combination...). I would definitely appreciate a follow up fix if you can provide one, since I would imagine it would be a pre-existing issue since all my patch does is make the check in the icl_combo_phy_enabled if block work as expected (when it does not appear to be working before). Cheers, Nathan > On Thu, 2020-07-16 at 05:06 +, Patchwork wrote: > > Patch Details > > Series: drm/i915/display: Ensure that ret is always initialized in > > icl_combo_phy_verify_state > > URL:https://patchwork.freedesktop.org/series/79536/ > > State: failure > > Details: > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18185/index.html > > CI Bug Log - changes from CI_DRM_8753 -> Patchwork_18185 > > Summary > > FAILURE > > > > Serious unknown changes coming with Patchwork_18185 absolutely need to be > > verified manually. > > > > If you think the reported changes have nothing to do with the changes > > introduced in Patchwork_18185, please notify your bug team to allow them > > to document this new failure mode, which will reduce false positives in CI. > > > > External URL: > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18185/index.html > > > > Possible new issues > > Here are the unknown changes that may have been introduced in > > Patchwork_18185: > > > > IGT changes > > Possible regressions > > igt@i915_pm_rpm@module-reload: > > fi-tgl-y: PASS -> DMESG-WARN +4 similar issues > > Suppressed > > The following results come from untrusted machines, tests, or statuses. > > They do not affect the overall result. > > > > igt@i915_pm_rpm@basic-pci-d3-state: > > > > {fi-tgl-dsi}: DMESG-WARN (i915#1982) -> DMESG-WARN > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a: > > > > {fi-tgl-dsi}: PASS -> DMESG-WARN +4 similar issues > > Known issues > > Here are the changes found in Patchwork_18185 that come from known issues: > > > > IGT changes > > Issues hit > > igt@gem_exec_suspend@basic-s0: > > > > fi-tgl-u2: PASS -> FAIL (i915#1888) > > igt@kms_addfb_basic@bo-too-small: > > > > fi-tgl-y: PASS -> DMESG-WARN (i915#402) +1 similar issue > > Possible fixes > > igt@debugfs_test@read_all_entries: > > > > fi-bsw-nick: INCOMPLETE (i915#1250 / i915#1436) -> PASS > > igt@gem_exec_store@basic: > > > > fi-tgl-y: DMESG-WARN (i915#402) -> PASS +2 similar issues > > igt@i915_module_load@reload: > > > > fi-byt-j1900: DMESG-WARN (i915#1982) -> PASS > > > > fi-kbl-x1275: DMESG-WARN (i915#62 / i915#92) -> PASS > > > > igt@i915_pm_rpm@basic-pci-d3-state: > > > > fi-bsw-kefka: DMESG-WARN (i915#1982) -> PASS > > igt@i915_selftest@live@execlists: > > > > fi-kbl-r: INCOMPLETE (i915#794) -> PASS > > igt@i915_selftest@live@gt_lrc: > > > > fi-tgl-u2: DMESG-FAIL (i915#1233) -> PASS > > igt@kms_flip@basic-flip-vs-modeset@a-dsi1: > > > > {fi-tgl-dsi}: DMESG-WARN (i915#1982) -> PASS > > igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence: > > > > fi-tgl-u2: DMESG-WARN (i915#402) -> PASS > > Warnings > > igt@gem_exec_suspend@basic-s0: > > > > fi-kbl-x1275: DMESG-WARN (i915#62 / i915#92) -> DMESG-WARN (i915#1982 / > > i915#62 / i915#92 / i915#95) > > igt@kms_flip@basic-flip-vs-dpms@a-dp1: > > > > fi-kbl-x1275: DMESG-WARN (i915#62 / i915#92 / i915#95) -> DMESG-WARN > > (i915#62 / i915#92) > > igt@kms_force_connector_basic@force-edid: > > > > fi-kbl-x1275: DMESG-WARN (i915#62 / i915#92) -> DMESG-WARN (i915#62 / > > i915#92 / i915#95) +4 similar issues > > {name}: This element is suppressed. This means it is ignored when computing > > the status of the difference (SUCCESS, WARNING, or FAILURE). > > > > Participating hosts (46 -> 40) > > Missing (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan > > fi-byt-clapper fi-bdw-samus > > > > Build changes > > Linux: CI_DRM_8753 -> Patchwork_18185 > > CI-20190529: 20190529 > > CI_DRM_8753: 62f01b776240c4586b3cbbdbe6065d4473d45429 @ > > git://anongit.freedesktop.org/gfx-ci/linux > > IGT_5737: c18a9c1083ce9344ff71ae405b9f2deaa82b6702 @ > > git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > > Patchwork_18185: b0efac00fd8cdd3d7a3cc3140ba0df8bda56ebf3 @ > > git://anongit.freedesktop.org/gfx-ci/linux > > > > == Linux commits == > > > > b0efac00fd8c drm/i915/display: Ensure that ret is always initialized in > > icl_combo_phy_verify_state > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] drm/i915: Fix the static code analysis warning in debugfs
On Wed, Jan 09, 2019 at 01:14:14PM -0800, Radhakrishna Sripada wrote: > intel_dp->dsc_dpcd is defined as an array making the if check redundant. > > Cc: Rodrigo Vivi > Signed-off-by: Radhakrishna Sripada This fixes a Clang warning I saw after the patch that added this hit linux-next. Reviewed-by: Nathan Chancellor Tested-by: Nathan Chancellor > --- > drivers/gpu/drm/i915/i915_debugfs.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 95813e21ae02..908e41f9cb7d 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -4958,9 +4958,8 @@ static int i915_dsc_fec_support_show(struct seq_file > *m, void *data) > crtc_state = to_intel_crtc_state(crtc->state); > seq_printf(m, "DSC_Enabled: %s\n", > yesno(crtc_state->dsc_params.compression_enable)); > - if (intel_dp->dsc_dpcd) > - seq_printf(m, "DSC_Sink_Support: %s\n", > - > yesno(drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))); > + seq_printf(m, "DSC_Sink_Support: %s\n", > +yesno(drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))); > if (!intel_dp_is_edp(intel_dp)) > seq_printf(m, "FEC_Sink_Support: %s\n", > > yesno(drm_dp_sink_supports_fec(intel_dp->fec_capable))); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Disable -Wuninitialized
This warning is disabled by default in scripts/Makefile.extrawarn when W= is not provided but this Makefile adds -Wall after this warning is disabled so it shows up in the build when it shouldn't: In file included from drivers/gpu/drm/i915/intel_breadcrumbs.c:895: drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c:350:34: error: variable 'wq' is uninitialized when used within its own initialization [-Werror,-Wuninitialized] DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq); ^~ ./include/linux/wait.h:74:63: note: expanded from macro 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK' struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name) ^~~~ ./include/linux/wait.h:72:33: note: expanded from macro '__WAIT_QUEUE_HEAD_INIT_ONSTACK' ({ init_waitqueue_head(&name); name; }) ^~~~ 1 error generated. Explicitly disable the warning like commit 46e2068081e9 ("drm/i915: Disable some extra clang warnings"). Link: https://github.com/ClangBuiltLinux/linux/issues/220 Signed-off-by: Nathan Chancellor --- v1 -> v2: * Rather than disable the warning for the problematic folder, disable it for the entire folder like Matthias's commit. drivers/gpu/drm/i915/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 8300efe60fe1..210d0e8777b6 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -22,6 +22,7 @@ subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable) subdir-ccflags-y += $(call cc-disable-warning, sign-compare) subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized) subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) +subdir-ccflags-y += $(call cc-disable-warning, uninitialized) subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror # Fine grained warnings disable -- 2.20.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Disable -Wuninitialized
On Sat, Jan 26, 2019 at 12:11:23AM -0700, Nathan Chancellor wrote: > This warning is disabled by default in scripts/Makefile.extrawarn when > W= is not provided but this Makefile adds -Wall after this warning is > disabled so it shows up in the build when it shouldn't: > > In file included from drivers/gpu/drm/i915/intel_breadcrumbs.c:895: > drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c:350:34: error: > variable 'wq' is uninitialized when used within its own initialization > [-Werror,-Wuninitialized] > DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq); > ^~ > ./include/linux/wait.h:74:63: note: expanded from macro > 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK' > struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name) > ^~~~ > ./include/linux/wait.h:72:33: note: expanded from macro > '__WAIT_QUEUE_HEAD_INIT_ONSTACK' > ({ init_waitqueue_head(&name); name; }) >^~~~ > 1 error generated. > > Explicitly disable the warning like commit 46e2068081e9 ("drm/i915: > Disable some extra clang warnings"). > > Link: https://github.com/ClangBuiltLinux/linux/issues/220 > Signed-off-by: Nathan Chancellor > --- > > v1 -> v2: > > * Rather than disable the warning for the problematic folder, disable it > for the entire folder like Matthias's commit. > Link to v1: https://lore.kernel.org/lkml/20181025193601.11746-1-natechancel...@gmail.com/ Since I forgot '--in-reply-to' :( > drivers/gpu/drm/i915/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 8300efe60fe1..210d0e8777b6 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -22,6 +22,7 @@ subdir-ccflags-y += $(call cc-disable-warning, > unused-but-set-variable) > subdir-ccflags-y += $(call cc-disable-warning, sign-compare) > subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized) > subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) > +subdir-ccflags-y += $(call cc-disable-warning, uninitialized) > subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror > > # Fine grained warnings disable > -- > 2.20.1 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Disable -Wuninitialized
On Fri, Jan 25, 2019 at 11:34:19PM -0800, Nick Desaulniers wrote: > On Fri, Jan 25, 2019 at 11:13 PM Nathan Chancellor > wrote: > > > > This warning is disabled by default in scripts/Makefile.extrawarn when > > W= is not provided but this Makefile adds -Wall after this warning is > > disabled so it shows up in the build when it shouldn't: > > > > In file included from drivers/gpu/drm/i915/intel_breadcrumbs.c:895: > > drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c:350:34: error: > > variable 'wq' is uninitialized when used within its own initialization > > [-Werror,-Wuninitialized] > > DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq); > > ^~ > > ./include/linux/wait.h:74:63: note: expanded from macro > > 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK' > > struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name) > > ^~~~ > > ./include/linux/wait.h:72:33: note: expanded from macro > > '__WAIT_QUEUE_HEAD_INIT_ONSTACK' > > ({ init_waitqueue_head(&name); name; }) > >^~~~ > > 1 error generated. > > > > Explicitly disable the warning like commit 46e2068081e9 ("drm/i915: > > Disable some extra clang warnings"). > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/220 > > Signed-off-by: Nathan Chancellor > > Reviewed-by: Nick Desaulniers > > probably could give Chris Wilson the suggested by tag. > https://lore.kernel.org/lkml/154513398652.1108.7150969916024071452@skylake-alporthouse-com/ > Ugh you're right, completely slipped my mind. Suggested-by: Chris Wilson > > --- > > > > v1 -> v2: > > > > * Rather than disable the warning for the problematic folder, disable it > > for the entire folder like Matthias's commit. > > Thanks for following up with a v2. Thank you for the review! Nathan > > -- > Thanks, > ~Nick Desaulniers ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
Hi Will and Claire, On Wed, Jun 30, 2021 at 12:43:48PM +0100, Will Deacon wrote: > On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote: > > On Wed, Jun 30, 2021 at 9:43 AM Nathan Chancellor wrote: > > > > > > On Thu, Jun 24, 2021 at 11:55:20PM +0800, Claire Chang wrote: > > > > Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and > > > > use it to determine whether to bounce the data or not. This will be > > > > useful later to allow for different pools. > > > > > > > > Signed-off-by: Claire Chang > > > > Reviewed-by: Christoph Hellwig > > > > Tested-by: Stefano Stabellini > > > > Tested-by: Will Deacon > > > > Acked-by: Stefano Stabellini > > > > > > This patch as commit af452ec1b1a3 ("swiotlb: Use is_swiotlb_force_bounce > > > for swiotlb data bouncing") causes my Ryzen 3 4300G system to fail to > > > get to an X session consistently (although not every single time), > > > presumably due to a crash in the AMDGPU driver that I see in dmesg. > > > > > > I have attached logs at af452ec1b1a3 and f127c9556a8e and I am happy > > > to provide any further information, debug, or test patches as necessary. > > > > Are you using swiotlb=force? or the swiotlb_map is called because of > > !dma_capable? > > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/dma/direct.h#n93) > > The command line is in the dmesg: > > | Kernel command line: initrd=\amd-ucode.img > initrd=\initramfs-linux-next-llvm.img > root=PARTUUID=8680aa0c-cf09-4a69-8cf3-970478040ee7 rw intel_pstate=no_hwp > irqpoll > > but I worry that this looks _very_ similar to the issue reported by Qian > Cai which we thought we had fixed. Nathan -- is the failure deterministic? Yes, for the most part. It does not happen every single boot so when I was bisecting, I did a series of seven boots and only considered the revision good when all seven of them made it to LightDM's greeter. My results that I notated show most bad revisions failed anywhere from four to six times. > > `BUG: unable to handle page fault for address: 003a8290` and > > the fact it crashed at `_raw_spin_lock_irqsave` look like the memory > > (maybe dev->dma_io_tlb_mem) was corrupted? > > The dev->dma_io_tlb_mem should be set here > > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) > > through device_initialize. > > I'm less sure about this. 'dma_io_tlb_mem' should be pointing at > 'io_tlb_default_mem', which is a page-aligned allocation from memblock. > The spinlock is at offset 0x24 in that structure, and looking at the > register dump from the crash: > > Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:adb4013db9e8 EFLAGS: 00010006 > Jun 29 18:28:42 hp-4300G kernel: RAX: 003a8290 RBX: > RCX: 8900572ad580 > Jun 29 18:28:42 hp-4300G kernel: RDX: 89005653f024 RSI: 000c > RDI: 1d17 > Jun 29 18:28:42 hp-4300G kernel: RBP: 0a20d000 R08: 000c > R09: > Jun 29 18:28:42 hp-4300G kernel: R10: 0a20d000 R11: 89005653f000 > R12: 0212 > Jun 29 18:28:42 hp-4300G kernel: R13: 1000 R14: 0002 > R15: 0020 > Jun 29 18:28:42 hp-4300G kernel: FS: 7f1f8898ea40() > GS:89005728() knlGS: > Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: ES: CR0: > 80050033 > Jun 29 18:28:42 hp-4300G kernel: CR2: 003a8290 CR3: 0001020d > CR4: 00350ee0 > Jun 29 18:28:42 hp-4300G kernel: Call Trace: > Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50 > Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0 > > Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer and > RDX pointing at the spinlock. Yet RAX is holding junk :/ > > I agree that enabling KASAN would be a good idea, but I also think we > probably need to get some more information out of swiotlb_tbl_map_single() > to see see what exactly is going wrong in there. I can certainly enable KASAN and if there is any debug print I can add or dump anything, let me know! Cheers, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v15 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
Hi Will and Robin, On Fri, Jul 02, 2021 at 04:13:50PM +0100, Robin Murphy wrote: > On 2021-07-02 14:58, Will Deacon wrote: > > Hi Nathan, > > > > On Thu, Jul 01, 2021 at 12:52:20AM -0700, Nathan Chancellor wrote: > > > On 7/1/2021 12:40 AM, Will Deacon wrote: > > > > On Wed, Jun 30, 2021 at 08:56:51AM -0700, Nathan Chancellor wrote: > > > > > On Wed, Jun 30, 2021 at 12:43:48PM +0100, Will Deacon wrote: > > > > > > On Wed, Jun 30, 2021 at 05:17:27PM +0800, Claire Chang wrote: > > > > > > > `BUG: unable to handle page fault for address: 003a8290` > > > > > > > and > > > > > > > the fact it crashed at `_raw_spin_lock_irqsave` look like the > > > > > > > memory > > > > > > > (maybe dev->dma_io_tlb_mem) was corrupted? > > > > > > > The dev->dma_io_tlb_mem should be set here > > > > > > > (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/probe.c#n2528) > > > > > > > through device_initialize. > > > > > > > > > > > > I'm less sure about this. 'dma_io_tlb_mem' should be pointing at > > > > > > 'io_tlb_default_mem', which is a page-aligned allocation from > > > > > > memblock. > > > > > > The spinlock is at offset 0x24 in that structure, and looking at the > > > > > > register dump from the crash: > > > > > > > > > > > > Jun 29 18:28:42 hp-4300G kernel: RSP: 0018:adb4013db9e8 EFLAGS: > > > > > > 00010006 > > > > > > Jun 29 18:28:42 hp-4300G kernel: RAX: 003a8290 RBX: > > > > > > RCX: 8900572ad580 > > > > > > Jun 29 18:28:42 hp-4300G kernel: RDX: 89005653f024 RSI: > > > > > > 000c RDI: 1d17 > > > > > > Jun 29 18:28:42 hp-4300G kernel: RBP: 0a20d000 R08: > > > > > > 000c R09: > > > > > > Jun 29 18:28:42 hp-4300G kernel: R10: 0a20d000 R11: > > > > > > 89005653f000 R12: 0212 > > > > > > Jun 29 18:28:42 hp-4300G kernel: R13: 1000 R14: > > > > > > 0002 R15: 0020 > > > > > > Jun 29 18:28:42 hp-4300G kernel: FS: 7f1f8898ea40() > > > > > > GS:89005728() knlGS: > > > > > > Jun 29 18:28:42 hp-4300G kernel: CS: 0010 DS: ES: CR0: > > > > > > 80050033 > > > > > > Jun 29 18:28:42 hp-4300G kernel: CR2: 003a8290 CR3: > > > > > > 0001020d CR4: 00350ee0 > > > > > > Jun 29 18:28:42 hp-4300G kernel: Call Trace: > > > > > > Jun 29 18:28:42 hp-4300G kernel: _raw_spin_lock_irqsave+0x39/0x50 > > > > > > Jun 29 18:28:42 hp-4300G kernel: swiotlb_tbl_map_single+0x12b/0x4c0 > > > > > > > > > > > > Then that correlates with R11 holding the 'dma_io_tlb_mem' pointer > > > > > > and > > > > > > RDX pointing at the spinlock. Yet RAX is holding junk :/ > > > > > > > > > > > > I agree that enabling KASAN would be a good idea, but I also think > > > > > > we > > > > > > probably need to get some more information out of > > > > > > swiotlb_tbl_map_single() > > > > > > to see see what exactly is going wrong in there. > > > > > > > > > > I can certainly enable KASAN and if there is any debug print I can add > > > > > or dump anything, let me know! > > > > > > > > I bit the bullet and took v5.13 with swiotlb/for-linus-5.14 merged in, > > > > built > > > > x86 defconfig and ran it on my laptop. However, it seems to work fine! > > > > > > > > Please can you share your .config? > > > > > > Sure thing, it is attached. It is just Arch Linux's config run through > > > olddefconfig. The original is below in case you need to diff it. > > > > > > https://raw.githubusercontent.com/archlinux/svntogit-packages/9045405dc835527164f3034b3ceb9a67c7a53cd4/trunk/config > > > > > > If there is anything more that I can provide, please let me know. > > > > I eventually got this booting (for some reason it was causing LD to
Re: [RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR
On Tue, Mar 05, 2024 at 11:07:36AM +0200, Jani Nikula wrote: > Add kconfig to enable -Werror subsystem wide. This is useful for > development and CI to keep the subsystem warning free, while avoiding > issues outside of the subsystem that kernel wide CONFIG_WERROR=y might > hit. > > v2: Don't depend on COMPILE_TEST > > Reviewed-by: Hamza Mahfooz # v1 > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/Kconfig | 13 + > drivers/gpu/drm/Makefile | 3 +++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 6e853acf15da..c08e18108c2a 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -416,3 +416,16 @@ config DRM_LIB_RANDOM > config DRM_PRIVACY_SCREEN > bool > default n > + > +config DRM_WERROR > + bool "Compile the drm subsystem with warnings as errors" > + depends on EXPERT > + default n > + help > + A kernel build should not cause any compiler warnings, and this > + enables the '-Werror' flag to enforce that rule in the drm subsystem. > + > + The drm subsystem enables more warnings than the kernel default, so > + this config option is disabled by default. > + > + If in doubt, say N. While I understand the desire for an easy switch that maintainers and developers can use to ensure that their changes are warning free for the drm subsystem specifically, I think subsystem specific configuration options like this are actively detrimental to developers and continuous integration systems that build test the entire kernel. For example, we turned off CONFIG_WERROR for our Hexagon builds because of warnings that appear with -Wextra that are legitimate but require treewide changes to resolve in a manner sufficient for Linus: https://github.com/ClangBuiltLinux/linux/issues/1285 https://lore.kernel.org/all/CAHk-=wg80je=k7madf4e7wrrnp37e3qh6y10svhdc7o8sz_...@mail.gmail.com/ https://lore.kernel.org/all/20230522105049.1467313-1-schne...@linux.ibm.com/ But now, due to CONFIG_DRM_WERROR getting enabled by all{mod,yes}config and -Wextra being unconditionally enabled for DRM, those warnings hard break the build despite CONFIG_WERROR=n... https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2eEBDGEqfmMZjGg3ZvDx2af2pde/build.log Same thing with PowerPC allmodconfig because we see -Wframe-larger-than that appears because allmodconfig enables CONFIG_KASAN or CONFIG_KCSAN usually: https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2eE2HDsODudQGqkMKAPQnId7pRd/build.log I don't know what the solution for this conflict is through. I guess it is just the nature of the kernel being a federation of independent subsystems that want to have their own policies. I suppose we can just set CONFIG_DRM_WERROR=n and be done with it but I would like to avoid this issue from spreading to other subsystems because it does not scale for folks like us who do many builds across many trees. It would be nice if there was something like CONFIG_WERROR_DIRS or something that could take a set of directories that should have -Werror enabled so that you could do something like CONFIG_WERROR_DIRS="drivers/gpu/drm" and have -Werror automatically added to all commands within that directory like subdir-ccflags-y but it is explicitly opt in on the part of the developer/tester, rather than just happening to get enabled due to all{mod,yes}config. No idea if that is feasible or not though. > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index ea456f057e8a..a73c04d2d7a3 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -30,6 +30,9 @@ subdir-ccflags-y += -Wno-sign-compare > endif > # --- end copy-paste > > +# Enable -Werror in CI and development > +subdir-ccflags-$(CONFIG_DRM_WERROR) += -Werror > + > drm-y := \ > drm_aperture.o \ > drm_atomic.o \ > -- > 2.39.2 >
Re: [RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR
On Wed, Mar 27, 2024 at 09:59:01AM +0200, Jani Nikula wrote: > On Wed, 27 Mar 2024, Maxime Ripard wrote: > > On Tue, Mar 26, 2024 at 03:56:50PM -0700, Nathan Chancellor wrote: > >> On Tue, Mar 05, 2024 at 11:07:36AM +0200, Jani Nikula wrote: > >> > Add kconfig to enable -Werror subsystem wide. This is useful for > >> > development and CI to keep the subsystem warning free, while avoiding > >> > issues outside of the subsystem that kernel wide CONFIG_WERROR=y might > >> > hit. > >> > > >> > v2: Don't depend on COMPILE_TEST > >> > > >> > Reviewed-by: Hamza Mahfooz # v1 > >> > Signed-off-by: Jani Nikula > >> > --- > >> > drivers/gpu/drm/Kconfig | 13 + > >> > drivers/gpu/drm/Makefile | 3 +++ > >> > 2 files changed, 16 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > >> > index 6e853acf15da..c08e18108c2a 100644 > >> > --- a/drivers/gpu/drm/Kconfig > >> > +++ b/drivers/gpu/drm/Kconfig > >> > @@ -416,3 +416,16 @@ config DRM_LIB_RANDOM > >> > config DRM_PRIVACY_SCREEN > >> > bool > >> > default n > >> > + > >> > +config DRM_WERROR > >> > +bool "Compile the drm subsystem with warnings as errors" > >> > +depends on EXPERT > >> > +default n > >> > +help > >> > + A kernel build should not cause any compiler warnings, and > >> > this > >> > + enables the '-Werror' flag to enforce that rule in the drm > >> > subsystem. > >> > + > >> > + The drm subsystem enables more warnings than the kernel > >> > default, so > >> > + this config option is disabled by default. > >> > + > >> > + If in doubt, say N. > >> > >> While I understand the desire for an easy switch that maintainers and > >> developers can use to ensure that their changes are warning free for the > >> drm subsystem specifically, I think subsystem specific configuration > >> options like this are actively detrimental to developers and continuous > >> integration systems that build test the entire kernel. For example, we > >> turned off CONFIG_WERROR for our Hexagon builds because of warnings that > >> appear with -Wextra that are legitimate but require treewide changes to > >> resolve in a manner sufficient for Linus: > >> > >> https://github.com/ClangBuiltLinux/linux/issues/1285 > >> https://lore.kernel.org/all/CAHk-=wg80je=k7madf4e7wrrnp37e3qh6y10svhdc7o8sz_...@mail.gmail.com/ > >> https://lore.kernel.org/all/20230522105049.1467313-1-schne...@linux.ibm.com/ > >> > >> But now, due to CONFIG_DRM_WERROR getting enabled by all{mod,yes}config > >> and -Wextra being unconditionally enabled for DRM, those warnings hard > >> break the build despite CONFIG_WERROR=n... > > > > Would making DRM_WERROR depends on WERROR address your concerns? > > But then what would be the point of having DRM_WERROR at all? For me the > point is, "werror in drm, ignore the rest, they're someone else's > problem". Right, I do think this is a valid view point and one I am sympathetic to, especially since it is in the pursuit of increased code quality. I do not want to disrupt that. > An alternative would be to "depends on !COMPILE_TEST" that we have in > i915, but then some folks want to have COMPILE_TEST in drm, because some > drivers are otherwise hard for people to build. Right. I think it is unfortunate how (at least in my opinion) CONFIG_COMPILE_TEST has two meanings: genuinely just compile testing or "allmodconfig". For the first case, we would want CONFIG_DRM_WERROR=y but for the second case, it would be nice for CONFIG_DRM_WERROR to default to off (because CONFIG_WERROR is enabled) but allow developers to turn it on explicitly. Another lofty/wistful idea to solve this would be to implement something similar to compiler diagnostic groups for Kconfig, where there would be a hierarchy like CONFIG_WERROR CONFIG_DRM_WERROR CONFIG_SUBSYSTEM_A_WERROR CONFIG_SUBSYSTEM_B_WERROR where the value of CONFIG_WERROR is the same value for all subconfigurations under it but they could still be enabled individually without any additional dependencies (ala something like '-Wno-unused -Wunused-variable'), which would allow my use case of CONFIG_WERROR=n removing all instances of -Werror t
Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal
Hi Mitul, On Mon, Jun 10, 2024 at 12:52:02PM +0530, Mitul Golani wrote: ... > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c > b/drivers/gpu/drm/i915/display/intel_vrr.c > index 4ad99a54aa83..05f67dc9d98d 100644 > --- a/drivers/gpu/drm/i915/display/intel_vrr.c > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > @@ -12,6 +12,9 @@ > #include "intel_vrr_regs.h" > #include "intel_dp.h" > > +#define FIXED_POINT_PRECISION100 > +#define CMRR_PRECISION_TOLERANCE 10 > + > bool intel_vrr_is_capable(struct intel_connector *connector) > { > const struct drm_display_info *info = &connector->base.display_info; > @@ -107,6 +110,52 @@ int intel_vrr_vmax_vblank_start(const struct > intel_crtc_state *crtc_state) > return crtc_state->vrr.vmax - intel_vrr_vblank_exit_length(crtc_state); > } > > +static bool > +is_cmrr_frac_required(struct intel_crtc_state *crtc_state) > +{ > + int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line; > + struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); > + > + if (!HAS_CMRR(i915)) > + return false; > + > + actual_refresh_k = > + drm_mode_vrefresh(adjusted_mode) * FIXED_POINT_PRECISION; > + pixel_clock_per_line = > + adjusted_mode->crtc_clock * 1000 / adjusted_mode->crtc_htotal; > + calculated_refresh_k = > + pixel_clock_per_line * FIXED_POINT_PRECISION / > adjusted_mode->crtc_vtotal; > + > + if ((actual_refresh_k - calculated_refresh_k) < > CMRR_PRECISION_TOLERANCE) > + return false; > + > + return true; > +} > + > +static unsigned int > +cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool > video_mode_required) > +{ > + int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate; > + long long adjusted_pixel_rate; > + struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; > + > + desired_refresh_rate = drm_mode_vrefresh(adjusted_mode); > + > + if (video_mode_required) { > + multiplier_m = 1001; > + multiplier_n = 1000; > + } > + > + crtc_state->cmrr.cmrr_n = > + desired_refresh_rate * adjusted_mode->crtc_htotal * > multiplier_n; > + vtotal = (adjusted_mode->crtc_clock * 1000 * multiplier_n) / > crtc_state->cmrr.cmrr_n; > + adjusted_pixel_rate = adjusted_mode->crtc_clock * 1000 * multiplier_m; > + crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, > crtc_state->cmrr.cmrr_n); > + > + return vtotal; > +} This change is now in -next as commit 1676ecd303ac ("drm/i915: Compute CMRR and calculate vtotal"), where it breaks the xe build for 32-bit platforms with: $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- allmodconfig drivers/gpu/drm/xe/i915-display/intel_vrr.o In file included from arch/arm/include/asm/div64.h:107, from include/linux/math.h:6, from include/linux/kernel.h:27, from include/linux/cpumask.h:11, from include/linux/smp.h:13, from include/linux/lockdep.h:14, from include/linux/spinlock.h:63, from include/linux/kref.h:16, from include/drm/drm_device.h:5, from include/drm/drm_drv.h:35, from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:13, from drivers/gpu/drm/i915/display/intel_vrr.c:7: drivers/gpu/drm/i915/display/intel_vrr.c: In function 'cmrr_get_vtotal': include/asm-generic/div64.h:222:35: error: comparison of distinct pointer types lacks a cast [-Werror] 222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \ | ^~ drivers/gpu/drm/i915/display/intel_vrr.c:155:35: note: in expansion of macro 'do_div' 155 | crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, crtc_state->cmrr.cmrr_n); | ^~ cc1: all warnings being treated as errors Also, is do_div() correct here? It is different from the other div_() macros in that the "return value" is the remainder, not the result of the division. Cheers, Nathan
Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal
On Wed, Jun 19, 2024 at 06:10:34PM +, Golani, Mitulkumar Ajitkumar wrote: > Hi @Nathan Chancellor > > Probably fix is merged in drm-intel-next > related patch: https://patchwork.freedesktop.org/series/134860/ > > Can you please check and suggest if this patch is merged ? This is still reproducible at commit 851de367dede ("drm/i915: Enable plane/pipeDMC ATS fault interrupts on mtl") in drm-intel-next, which includes that change as commit e2dc7cb72b25 ("drm/i915/display: Update calculation to avoid overflow"). The issue is the dividend in do_div() is required to be an unsigned 64-bit type but you used a signed type. Updating adjusted_pixel_rate to be a u64 should resolve the issue and match the return type of mul_u32_u32(). I just wasn't sure if that was the only fix this code would need, as do_div() is not typically used with an assignment. Cheers, Nathan > > -Original Message- > > From: Nathan Chancellor > > Sent: Wednesday, June 19, 2024 9:12 PM > > To: Golani, Mitulkumar Ajitkumar > > Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K > > ; intel...@lists.freedesktop.org > > Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal > > > > Hi Mitul, > > > > On Mon, Jun 10, 2024 at 12:52:02PM +0530, Mitul Golani wrote: > > ... > > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c > > > b/drivers/gpu/drm/i915/display/intel_vrr.c > > > index 4ad99a54aa83..05f67dc9d98d 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > > > @@ -12,6 +12,9 @@ > > > #include "intel_vrr_regs.h" > > > #include "intel_dp.h" > > > > > > +#define FIXED_POINT_PRECISION100 > > > +#define CMRR_PRECISION_TOLERANCE 10 > > > + > > > bool intel_vrr_is_capable(struct intel_connector *connector) { > > > const struct drm_display_info *info = &connector->base.display_info; > > > @@ -107,6 +110,52 @@ int intel_vrr_vmax_vblank_start(const struct > > intel_crtc_state *crtc_state) > > > return crtc_state->vrr.vmax - > > > intel_vrr_vblank_exit_length(crtc_state); > > > } > > > > > > +static bool > > > +is_cmrr_frac_required(struct intel_crtc_state *crtc_state) { > > > + int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line; > > > + struct drm_display_mode *adjusted_mode = &crtc_state- > > >hw.adjusted_mode; > > > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); > > > + > > > + if (!HAS_CMRR(i915)) > > > + return false; > > > + > > > + actual_refresh_k = > > > + drm_mode_vrefresh(adjusted_mode) * > > FIXED_POINT_PRECISION; > > > + pixel_clock_per_line = > > > + adjusted_mode->crtc_clock * 1000 / adjusted_mode- > > >crtc_htotal; > > > + calculated_refresh_k = > > > + pixel_clock_per_line * FIXED_POINT_PRECISION / > > > +adjusted_mode->crtc_vtotal; > > > + > > > + if ((actual_refresh_k - calculated_refresh_k) < > > CMRR_PRECISION_TOLERANCE) > > > + return false; > > > + > > > + return true; > > > +} > > > + > > > +static unsigned int > > > +cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool > > > +video_mode_required) { > > > + int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate; > > > + long long adjusted_pixel_rate; > > > + struct drm_display_mode *adjusted_mode = > > > +&crtc_state->hw.adjusted_mode; > > > + > > > + desired_refresh_rate = drm_mode_vrefresh(adjusted_mode); > > > + > > > + if (video_mode_required) { > > > + multiplier_m = 1001; > > > + multiplier_n = 1000; > > > + } > > > + > > > + crtc_state->cmrr.cmrr_n = > > > + desired_refresh_rate * adjusted_mode->crtc_htotal * > > multiplier_n; > > > + vtotal = (adjusted_mode->crtc_clock * 1000 * multiplier_n) / > > crtc_state->cmrr.cmrr_n; > > > + adjusted_pixel_rate = adjusted_mode->crtc_clock * 1000 * > > multiplier_m; > > > + crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, > > > +crtc_state->cmrr.cmrr_n); > > > + > > > + return vtotal; > > > +} > > > > This change is now in -next as commit 1676ecd303ac ("drm/i915: Compute > > CMRR and calculate vtotal"), where it breaks the x
Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal
On Thu, Jun 20, 2024 at 03:59:03PM +0300, Jani Nikula wrote: > On Thu, 20 Jun 2024, "Golani, Mitulkumar Ajitkumar" > wrote: > > Hi @Nathan Chancellor, > > > > Yes, with do_div, we are expecting the remainder value. Regarding the > > warning related to the adjusted_pixel_rate type cast, I haven't been > > able to reproduce this locally, possibly due to differences in the > > cross-compiler. We should consider typecasting adjusted_pixel_rate or > > treating it as unsigned ? > > Please avoid top-posting on the mailing lists. > > I'm guessing this will be enough. Indeed, that works. > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c > b/drivers/gpu/drm/i915/display/intel_vrr.c > index 6430da25957d..5a0da64c7db3 100644 > --- a/drivers/gpu/drm/i915/display/intel_vrr.c > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > @@ -137,7 +137,7 @@ static unsigned int > cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool > video_mode_required) > { > int multiplier_m = 1, multiplier_n = 1, vtotal, desired_refresh_rate; > - long long adjusted_pixel_rate; > + u64 adjusted_pixel_rate; > struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; > > desired_refresh_rate = drm_mode_vrefresh(adjusted_mode); > > > BR, > Jani. > > > > > Adding @Nikula, Jani to suggest. > > > > Regards, > > Mitul > >> -Original Message- > >> From: Nathan Chancellor > >> Sent: Wednesday, June 19, 2024 11:56 PM > >> To: Golani, Mitulkumar Ajitkumar > >> Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K > >> ; intel...@lists.freedesktop.org > >> Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate vtotal > >> > >> On Wed, Jun 19, 2024 at 06:10:34PM +, Golani, Mitulkumar Ajitkumar > >> wrote: > >> > Hi @Nathan Chancellor > >> > > >> > Probably fix is merged in drm-intel-next related patch: > >> > https://patchwork.freedesktop.org/series/134860/ > >> > > >> > Can you please check and suggest if this patch is merged ? > >> > >> This is still reproducible at commit 851de367dede ("drm/i915: Enable > >> plane/pipeDMC ATS fault interrupts on mtl") in drm-intel-next, which > >> includes > >> that change as commit e2dc7cb72b25 ("drm/i915/display: Update calculation > >> to avoid overflow"). The issue is the dividend in do_div() is required to > >> be an > >> unsigned 64-bit type but you used a signed type. > >> Updating adjusted_pixel_rate to be a u64 should resolve the issue and match > >> the return type of mul_u32_u32(). I just wasn't sure if that was the only > >> fix > >> this code would need, as do_div() is not typically used with an assignment. > >> > >> Cheers, > >> Nathan > >> > >> > > -Original Message- > >> > > From: Nathan Chancellor > >> > > Sent: Wednesday, June 19, 2024 9:12 PM > >> > > To: Golani, Mitulkumar Ajitkumar > >> > > > >> > > Cc: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K > >> > > ; intel...@lists.freedesktop.org > >> > > Subject: Re: [PATCH v16 9/9] drm/i915: Compute CMRR and calculate > >> > > vtotal > >> > > > >> > > Hi Mitul, > >> > > > >> > > On Mon, Jun 10, 2024 at 12:52:02PM +0530, Mitul Golani wrote: > >> > > ... > >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c > >> > > > b/drivers/gpu/drm/i915/display/intel_vrr.c > >> > > > index 4ad99a54aa83..05f67dc9d98d 100644 > >> > > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c > >> > > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > >> > > > @@ -12,6 +12,9 @@ > >> > > > #include "intel_vrr_regs.h" > >> > > > #include "intel_dp.h" > >> > > > > >> > > > +#define FIXED_POINT_PRECISION 100 > >> > > > +#define CMRR_PRECISION_TOLERANCE 10 > >> > > > + > >> > > > bool intel_vrr_is_capable(struct intel_connector *connector) { > >> > > > const struct drm_display_info *info = > >> > > > &connector->base.display_info; @@ -107,6 +110,52 @@ int > >> > > > intel_vrr_vmax_vblank_start(const struct > >> > >
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Encapsulate kconfig constant values inside boolean predicates
On Fri, Oct 25, 2019 at 02:59:42PM +0100, Chris Wilson wrote: > Avoid angering clang and smatch by using a constant value in a '&&' test, > by forcing that constant value into a boolean. > > E.g., > drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c:159:13: warning: use of > logical '&&' with constant operand [-Wconstant-logical-operand] > if (!delay && CONFIG_DRM_I915_PREEMPT_TIMEOUT) { > ^ ~~~ > > Reported-by: kbuild test robot > Signed-off-by: Chris Wilson > Cc: Nathan Chancellor > Cc: Nick Desaulniers > Cc: Jani Nikula LGTM, thank you for the fix! Reviewed-by: Nathan Chancellor ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Cast remain to unsigned long in eb_relocate_vma
On Mon, Mar 16, 2020 at 02:41:23PM -0700, Nick Desaulniers wrote: > On Fri, Feb 14, 2020 at 7:36 AM Michel Dänzer wrote: > > > > On 2020-02-14 12:49 p.m., Jani Nikula wrote: > > > On Fri, 14 Feb 2020, Chris Wilson wrote: > > >> Quoting Jani Nikula (2020-02-14 06:36:15) > > >>> On Thu, 13 Feb 2020, Nathan Chancellor wrote: > > >>>> A recent commit in clang added -Wtautological-compare to -Wall, which > > >>>> is > > >>>> enabled for i915 after -Wtautological-compare is disabled for the rest > > >>>> of the kernel so we see the following warning on x86_64: > > >>>> > > >>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433:22: warning: > > >>>> result of comparison of constant 576460752303423487 with expression of > > >>>> type 'unsigned int' is always false > > >>>> [-Wtautological-constant-out-of-range-compare] > > >>>> if (unlikely(remain > N_RELOC(ULONG_MAX))) > > >>>> ^ > > >>>> ../include/linux/compiler.h:78:42: note: expanded from macro > > >>>> 'unlikely' > > >>>> # define unlikely(x)__builtin_expect(!!(x), 0) > > >>>> ^ > > >>>> 1 warning generated. > > >>>> > > >>>> It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not > > >>>> account for the case where this file is built for 32-bit x86, where > > >>>> ULONG_MAX == UINT_MAX and this check is still relevant. > > >>>> > > >>>> Cast remain to unsigned long, which keeps the generated code the same > > >>>> (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and > > >>>> the warning is silenced so we can catch more potential issues in the > > >>>> future. > > >>>> > > >>>> Link: https://github.com/ClangBuiltLinux/linux/issues/778 > > >>>> Suggested-by: Michel Dänzer > > >>>> Signed-off-by: Nathan Chancellor > > >>> > > >>> Works for me as a workaround, > > >> > > >> But the whole point was that the compiler could see that it was > > >> impossible and not emit the code. Doesn't this break that? > > > > > > It seems that goal and the warning are fundamentally incompatible. > > > > Not really: > > > > if (sizeof(remain) >= sizeof(unsigned long) && > > unlikely(remain > N_RELOC(ULONG_MAX))) > > return -EINVAL; > > > > In contrast to the cast, this doesn't generate any machine code on 64-bit: > > > > https://godbolt.org/z/GmUE4S > > > > but still generates the same code on 32-bit: > > > > https://godbolt.org/z/hAoz8L > > Exactly. > > This check is only a tautology when `sizeof(long) == sizeof(int)` (ie. > ILP32 platforms, like 32b x86), notice how BOTH GCC AND Clang generate > exactly the same code: https://godbolt.org/z/6ShrDM > > Both compilers eliminate the check when `-m32` is not set, and > generate the exact same check otherwise. How about: > ``` > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index d3f4f28e9468..25b9d3f3ad57 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -1415,8 +1415,10 @@ static int eb_relocate_vma(struct > i915_execbuffer *eb, struct eb_vma *ev) > > urelocs = u64_to_user_ptr(entry->relocs_ptr); > remain = entry->relocation_count; > +#ifndef CONFIG_64BIT > if (unlikely(remain > N_RELOC(ULONG_MAX))) > return -EINVAL; > +#endif > > /* > * We must check that the entire relocation array is safe > ``` > > We now have 4 proposed solutions: > 1. > https://lore.kernel.org/lkml/20191123195321.41305-1-natechancel...@gmail.com/ > 2. > https://lore.kernel.org/lkml/20200211050808.29463-1-natechancel...@gmail.com/ > 3. > https://lore.kernel.org/lkml/20200214054706.33870-1-natechancel...@gmail.com/ > 4. my diff above > Let's please come to a resolution on this. This is the only warning on an x86_64 defconfig build. Apologies if we are being too persistent or nagging but we need guidance from the i915 maintainers on which solution they would prefer so it can be picked up. I understand you all are busy and I appreciate the work you all do but I do not want this to fall between the cracks because it is annoying to constantly see this warning. Cheers, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [18/21] drm/i915: extract intel_runtime_pm.h from intel_drv.h
Hi Jani, On Mon, Apr 29, 2019 at 03:29:36PM +0300, Jani Nikula wrote: > It used to be handy that we only had a couple of headers, but over time > intel_drv.h has become unwieldy. Extract declarations to a separate > header file corresponding to the implementation module, clarifying the > modularity of the driver. > > Ensure the new header is self-contained, and do so with minimal further > includes, using forward declarations as needed. Include the new header > only where needed, and sort the modified include directives while at it > and as needed. > > No functional changes. > > Signed-off-by: Jani Nikula > --- > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h > b/drivers/gpu/drm/i915/intel_runtime_pm.h > new file mode 100644 > index 00..692277 > --- /dev/null > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h > @@ -0,0 +1,105 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright © 2019 Intel Corporation > + */ > + > +#ifndef __INTEL_RUNTIME_PM_H__ > +#define __INTEL_RUNTIME_PM_H__ > + > +#include > +#include > + > +struct drm_i915_private; > + > +typedef depot_stack_handle_t intel_wakeref_t; Just as an FYI, this particular change causes a slew of clang warnings: https://travis-ci.com/ClangBuiltLinux/continuous-integration/jobs/201754420#L2435 I am not sure how exactly you'd like this resolved so I didn't want to provide a useless patch. I figured I would let you know about it and leave it up to you. Cheers, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: remove duplicate typedef for intel_wakeref_t
On Wed, May 22, 2019 at 01:35:05PM +0300, Jani Nikula wrote: > Fix the duplicate typedef for intel_wakeref_t leading to Clang build > issues. While at it, actually make the intel_runtime_pm.h header > self-contained, which was claimed in the commit being fixed. > > Reported-by: Nathan Chancellor > Cc: Nathan Chancellor > Cc: Chris Wilson > References: http://mid.mail-archive.com/20190521183850.GA9157@archlinux-epyc > References: > https://travis-ci.com/ClangBuiltLinux/continuous-integration/jobs/201754420#L2435 > Fixes: 0d5adc5f2f01 ("drm/i915: extract intel_runtime_pm.h from intel_drv.h") > Signed-off-by: Jani Nikula Reviewed-by: Nathan Chancellor Tested-by: Nathan Chancellor > --- > drivers/gpu/drm/i915/Makefile.header-test | 1 + > drivers/gpu/drm/i915/intel_runtime_pm.h | 8 +--- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/Makefile.header-test > b/drivers/gpu/drm/i915/Makefile.header-test > index 2ca4a5f..3a9663 100644 > --- a/drivers/gpu/drm/i915/Makefile.header-test > +++ b/drivers/gpu/drm/i915/Makefile.header-test > @@ -55,6 +55,7 @@ header_test := \ > intel_pm.h \ > intel_psr.h \ > intel_quirks.h \ > + intel_runtime_pm.h \ > intel_sdvo.h \ > intel_sideband.h \ > intel_sprite.h \ > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h > b/drivers/gpu/drm/i915/intel_runtime_pm.h > index b964ca..0a4c4b 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h > @@ -6,12 +6,14 @@ > #ifndef __INTEL_RUNTIME_PM_H__ > #define __INTEL_RUNTIME_PM_H__ > > -#include > #include > > -struct drm_i915_private; > +#include "intel_display.h" > +#include "intel_wakeref.h" > > -typedef depot_stack_handle_t intel_wakeref_t; > +struct drm_i915_private; > +struct drm_printer; > +struct intel_encoder; > > enum i915_drm_suspend_mode { > I915_DRM_SUSPEND_IDLE, > -- > 2.20.1 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [18/21] drm/i915: extract intel_runtime_pm.h from intel_drv.h
On Wed, May 22, 2019 at 01:38:18PM +0300, Jani Nikula wrote: > On Tue, 21 May 2019, Nathan Chancellor wrote: > > Hi Jani, > > > > On Mon, Apr 29, 2019 at 03:29:36PM +0300, Jani Nikula wrote: > >> It used to be handy that we only had a couple of headers, but over time > >> intel_drv.h has become unwieldy. Extract declarations to a separate > >> header file corresponding to the implementation module, clarifying the > >> modularity of the driver. > >> > >> Ensure the new header is self-contained, and do so with minimal further > >> includes, using forward declarations as needed. Include the new header > >> only where needed, and sort the modified include directives while at it > >> and as needed. > >> > >> No functional changes. > >> > >> Signed-off-by: Jani Nikula > >> --- > > > > > > > >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h > >> b/drivers/gpu/drm/i915/intel_runtime_pm.h > >> new file mode 100644 > >> index 00..692277 > >> --- /dev/null > >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h > >> @@ -0,0 +1,105 @@ > >> +/* SPDX-License-Identifier: MIT */ > >> +/* > >> + * Copyright © 2019 Intel Corporation > >> + */ > >> + > >> +#ifndef __INTEL_RUNTIME_PM_H__ > >> +#define __INTEL_RUNTIME_PM_H__ > >> + > >> +#include > >> +#include > >> + > >> +struct drm_i915_private; > >> + > >> +typedef depot_stack_handle_t intel_wakeref_t; > > > > Just as an FYI, this particular change causes a slew of clang warnings: > > https://travis-ci.com/ClangBuiltLinux/continuous-integration/jobs/201754420#L2435 > > > > I am not sure how exactly you'd like this resolved so I didn't want to > > provide a useless patch. I figured I would let you know about it and > > leave it up to you. > > Thanks for the report, fix is [1]. Thanks for the quick fix! > > I wonder why gcc allows dupe definitions. *shrug* I was briefly curious then I realized I'd probably have to look at the GCC source to figure out why... Cheers, Nathan > > BR, > Jani. > > [1] > http://patchwork.freedesktop.org/patch/msgid/20190522103505.2082-1-jani.nik...@intel.com > > > -- > Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [CI,5/5] drm/i915: Expand subslice mask
Hi Stuart, On Fri, May 24, 2019 at 08:40:22AM -0700, Stuart Summers wrote: > Currently, the subslice_mask runtime parameter is stored as an > array of subslices per slice. Expand the subslice mask array to > better match what is presented to userspace through the > I915_QUERY_TOPOLOGY_INFO ioctl. The index into this array is > then calculated: > slice * subslice stride + subslice index / 8 > > v2: fix spacing in set_sseu_info args > use set_sseu_info to initialize sseu data when building > device status in debugfs > rename variables in intel_engine_types.h to avoid checkpatch > warnings > v3: update headers in intel_sseu.h > v4: add const to some sseu_dev_info variables > use sseu->eu_stride for EU stride calculations > v5: address review comments from Tvrtko and Daniele > v6: remove extra space in intel_sseu_get_subslices > return the correct subslice enable in for_each_instdone > add GEM_BUG_ON to ensure user doesn't pass invalid ss_mask size > use printk formatted string for subslice mask > v7: remove string.h header and rebase > > Cc: Daniele Ceraolo Spurio > Cc: Lionel Landwerlin > Acked-by: Lionel Landwerlin > Reviewed-by: Daniele Ceraolo Spurio > Signed-off-by: Stuart Summers > --- > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > b/drivers/gpu/drm/i915/intel_device_info.c > index 97f742530fa1..3625f777f3a3 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -92,9 +92,9 @@ static void sseu_dump(const struct sseu_dev_info *sseu, > struct drm_printer *p) > hweight8(sseu->slice_mask), sseu->slice_mask); > drm_printf(p, "subslice total: %u\n", intel_sseu_subslice_total(sseu)); > for (s = 0; s < sseu->max_slices; s++) { > - drm_printf(p, "slice%d: %u subslices, mask=%04x\n", > + drm_printf(p, "slice%d: %u subslices, mask=%08x\n", > s, intel_sseu_subslices_per_slice(sseu, s), > -sseu->subslice_mask[s]); > +intel_sseu_get_subslices(sseu, s)); > } > drm_printf(p, "EU total: %u\n", sseu->eu_total); > drm_printf(p, "EU per subslice: %u\n", sseu->eu_per_subslice); > @@ -117,10 +117,9 @@ void intel_device_info_dump_runtime(const struct > intel_runtime_info *info, > static int sseu_eu_idx(const struct sseu_dev_info *sseu, int slice, > int subslice) > { > - int subslice_stride = GEN_SSEU_STRIDE(sseu->max_eus_per_subslice); > - int slice_stride = sseu->max_subslices * subslice_stride; > + int slice_stride = sseu->max_subslices * sseu->eu_stride; > > - return slice * slice_stride + subslice * subslice_stride; > + return slice * slice_stride + subslice * sseu->eu_stride; > } > > static u16 sseu_get_eus(const struct sseu_dev_info *sseu, int slice, > @@ -129,7 +128,7 @@ static u16 sseu_get_eus(const struct sseu_dev_info *sseu, > int slice, > int i, offset = sseu_eu_idx(sseu, slice, subslice); > u16 eu_mask = 0; > > - for (i = 0; i < GEN_SSEU_STRIDE(sseu->max_eus_per_subslice); i++) { > + for (i = 0; i < sseu->eu_stride; i++) { > eu_mask |= ((u16)sseu->eu_mask[offset + i]) << > (i * BITS_PER_BYTE); > } > @@ -142,7 +141,7 @@ static void sseu_set_eus(struct sseu_dev_info *sseu, int > slice, int subslice, > { > int i, offset = sseu_eu_idx(sseu, slice, subslice); > > - for (i = 0; i < GEN_SSEU_STRIDE(sseu->max_eus_per_subslice); i++) { > + for (i = 0; i < sseu->eu_stride; i++) { > sseu->eu_mask[offset + i] = > (eu_mask >> (BITS_PER_BYTE * i)) & 0xff; > } > @@ -159,9 +158,9 @@ void intel_device_info_dump_topology(const struct > sseu_dev_info *sseu, > } > > for (s = 0; s < sseu->max_slices; s++) { > - drm_printf(p, "slice%d: %u subslice(s) (0x%hhx):\n", > + drm_printf(p, "slice%d: %u subslice(s) (0x%08x):\n", > s, intel_sseu_subslices_per_slice(sseu, s), > -sseu->subslice_mask[s]); > +intel_sseu_get_subslices(sseu, s)); > > for (ss = 0; ss < sseu->max_subslices; ss++) { > u16 enabled_eus = sseu_get_eus(sseu, s, ss); > @@ -190,15 +189,10 @@ static void gen11_sseu_info_init(struct > drm_i915_private *dev_priv) > u8 eu_en; > int s; > > - if (IS_ELKHARTLAKE(dev_priv)) { > - sseu->max_slices = 1; > - sseu->max_subslices = 4; > - sseu->max_eus_per_subslice = 8; > - } else { > - sseu->max_slices = 1; > - sseu->max_subslices = 8; > - sseu->max_eus_per_subslice = 8; > - } > + if (IS_ELKHARTLAKE(dev_priv)) > + intel_sseu_set_info(sseu, 1, 4, 8); > + else > + intel_sseu_set_info(sseu, 1, 8, 8); > > s_en = I915_READ(GEN11_GT_SLIC
Re: [Intel-gfx] [PATCH] drm/i915: re-disable -Wframe-address
On Sun, Apr 26, 2020 at 02:42:15PM -0700, 'Nick Desaulniers' via Clang Built Linux wrote: > The top level Makefile disables this warning. When building an > i386_defconfig with Clang, this warning is triggered a whole bunch via > includes of headers from perf. > > Link: https://github.com/ClangBuiltLinux/continuous-integration/pull/182 > Signed-off-by: Nick Desaulniers This is not technically a clang specific warning but I assume it is only visible with clang so this location is probably fine. Reviewed-by: Nathan Chancellor ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/gt: Avoid uninitialized use of rpcurupei in frequency_show
When building with clang + -Wuninitialized: drivers/gpu/drm/i915/gt/debugfs_gt_pm.c:407:7: warning: variable 'rpcurupei' is uninitialized when used here [-Wuninitialized] rpcurupei, ^ drivers/gpu/drm/i915/gt/debugfs_gt_pm.c:304:16: note: initialize the variable 'rpcurupei' to silence this warning u32 rpcurupei, rpcurup, rpprevup; ^ = 0 1 warning generated. rpupei is assigned twice; based on the second argument to intel_uncore_read, it seems this one should have been assigned to rpcurupei. Fixes: 9c878557b1eb ("drm/i915/gt: Use the RPM config register to determine clk frequencies") Link: https://github.com/ClangBuiltLinux/linux/issues/1016 Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gt/debugfs_gt_pm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/debugfs_gt_pm.c b/drivers/gpu/drm/i915/gt/debugfs_gt_pm.c index 3d3ef62ed89f..f6ba66206273 100644 --- a/drivers/gpu/drm/i915/gt/debugfs_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/debugfs_gt_pm.c @@ -336,7 +336,7 @@ static int frequency_show(struct seq_file *m, void *unused) rpdeclimit = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD); rpstat = intel_uncore_read(uncore, GEN6_RPSTAT1); - rpupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) & GEN6_CURICONT_MASK; + rpcurupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) & GEN6_CURICONT_MASK; rpcurup = intel_uncore_read(uncore, GEN6_RP_CUR_UP) & GEN6_CURBSYTAVG_MASK; rpprevup = intel_uncore_read(uncore, GEN6_RP_PREV_UP) & GEN6_CURBSYTAVG_MASK; rpcurdownei = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN_EI) & GEN6_CURIAVG_MASK; base-commit: 0fd02a5d3eb7020a7e1801f8d7f01891071c85e4 -- 2.26.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Remove duplicate inline specifier on write_pte
When building with clang: drivers/gpu/drm/i915/gt/gen8_ppgtt.c:392:24: warning: duplicate 'inline' declaration specifier [-Wduplicate-decl-specifier] declaration specifier [-Wduplicate-decl-specifier] static __always_inline inline void ^ include/linux/compiler_types.h:138:16: note: expanded from macro 'inline' #define inline inline __gnu_inline __inline_maybe_unused notrace ^ 1 warning generated. __always_inline is defined as 'inline __attribute__((__always_inline))' so we do not need to specify it twice. Fixes: 84eac0c65940 ("drm/i915/gt: Force pte cacheline to main memory") Link: https://github.com/ClangBuiltLinux/linux/issues/1024 Reported-by: kbuild test robot Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c index 2dc88e76ebec..699125928272 100644 --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c @@ -389,7 +389,7 @@ static int gen8_ppgtt_alloc(struct i915_address_space *vm, return err; } -static __always_inline inline void +static __always_inline void write_pte(gen8_pte_t *pte, const gen8_pte_t val) { /* Magic delays? Or can we refine these to flush all in one pass? */ base-commit: e098d7762d602be640c53565ceca342f81e55ad2 -- 2.26.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Mark check_shadow_context_ppgtt as maybe unused
When CONFIG_DRM_I915_DEBUG_GEM is not set, clang warns: drivers/gpu/drm/i915/gvt/scheduler.c:884:1: warning: function 'check_shadow_context_ppgtt' is not needed and will not be emitted [-Wunneeded-internal-declaration] check_shadow_context_ppgtt(struct execlist_ring_context *c, struct intel_vgpu_mm *m) ^ 1 warning generated. This warning is similar to -Wunused-function but rather than warning that the function is completely unused, it warns that it is used in some expression within the file but that expression will be evaluated to a constant or be optimized away in the final assembly, essentially making it appeared used but really isn't. Usually, this happens when a function or variable is only used in sizeof, where it will appear to be used but will be evaluated at compile time and not be required to be emitted. In this case, the function is only used in GEM_BUG_ON, which is defined as BUILD_BUG_ON_INVALID, which intentionally follows this pattern. To fix this warning, add __maybe_unused to make it clear that this is intentional depending on the configuration. Fixes: bec3df930fbd ("drm/i915/gvt: Support PPGTT table load command") Link: https://github.com/ClangBuiltLinux/linux/issues/1027 Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gvt/scheduler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c index f776c92de8d7..0fb1df71c637 100644 --- a/drivers/gpu/drm/i915/gvt/scheduler.c +++ b/drivers/gpu/drm/i915/gvt/scheduler.c @@ -880,7 +880,7 @@ static void update_guest_pdps(struct intel_vgpu *vgpu, gpa + i * 8, &pdp[7 - i], 4); } -static bool +static __maybe_unused bool check_shadow_context_ppgtt(struct execlist_ring_context *c, struct intel_vgpu_mm *m) { if (m->ppgtt_mm.root_entry_type == GTT_TYPE_PPGTT_ROOT_L4_ENTRY) { base-commit: bdecf38f228bcca73b31ada98b5b7ba1215eb9c9 -- 2.26.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Remove tautological compare in eb_relocate_vma
On Tue, Dec 03, 2019 at 10:45:22AM -0800, Nick Desaulniers wrote: > On Tue, Dec 3, 2019 at 5:42 AM Chris Wilson wrote: > > > > Quoting Nick Desaulniers (2019-12-02 19:18:20) > > > On Sat, Nov 23, 2019 at 12:05 PM Chris Wilson > > > wrote: > > > > > > > > Quoting Nathan Chancellor (2019-11-23 19:53:22) > > > > > -Wtautological-compare was recently added to -Wall in LLVM, which > > > > > exposed an if statement in i915 that is always false: > > > > > > > > > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > > > > > result of comparison of constant 576460752303423487 with expression of > > > > > type 'unsigned int' is always false > > > > > [-Wtautological-constant-out-of-range-compare] > > > > > if (unlikely(remain > N_RELOC(ULONG_MAX))) > > > > > ^ > > > > > > > > > > Since remain is an unsigned int, it can never be larger than UINT_MAX, > > > > > which is less than ULONG_MAX / sizeof(struct > > > > > drm_i915_gem_relocation_entry). > > > > > Remove this statement to fix the warning. > > > > > > > > The check should remain as we do want to document the overflow > > > > calculation, and it should represent the types used -- it's much easier > > > > > > What do you mean "represent the types used?" Are you concerned that > > > the type of drm_i915_gem_exec_object2->relocation_count might change > > > in the future? > > > > We may want to change the restriction, yes. > > > > > > to review a stub than trying to find a missing overflow check. If the > > > > overflow cannot happen as the types are wide enough, no problem, the > > > > compiler can remove the known false branch. > > > > > > What overflow are you trying to protect against here? > > > > These values are under user control, our validation steps should be > > clear and easy to check. If we have the types wrong, if the checks are > > wrong, we need to fix them. If the code is removed because it can be > > evaluated by the compiler to be redundant, it is much harder for us to > > verify that we have tried to validate user input. > > > > > > Tautology here has a purpose for conveying information to the reader. > > > > > > Well leaving a warning unaddressed is also not a solution. Either > > > replace it with a comment or turn off the warning for your subdir. > > > > My personal preference would be to use a bunch of central macros for the > > various type/kmalloc overflows, and have the warnings suppressed there > > since they are very much about documenting user input validation. > > -Chris > > Is kmalloc_array what you're looking for? Looks like it has the > `check_mul_overflow` call in it. I don't think kmalloc_array is right because we are not validating an allocation. I am not sure that any of these overflow macros are correct, we would probably need something new but I am not sure. Cheers, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Disable -Wtautological-constant-out-of-range-compare
A recent commit in clang added -Wtautological-compare to -Wall, which is enabled for i915 so we see the following warning: ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: result of comparison of constant 576460752303423487 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare] if (unlikely(remain > N_RELOC(ULONG_MAX))) ^ This warning only happens on x86_64 but that check is relevant for 32-bit x86 so we cannot remove it. -Wtautological-compare on a whole has good warnings but this one is not really relevant for the kernel because of all of the different configurations that are used to build the kernel. When -Wtautological-compare is enabled for the kernel, this option will remain disabled so do that for i915 now. Link: https://github.com/ClangBuiltLinux/linux/issues/778 Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 38df01c23176..55dbcca179c7 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -21,6 +21,7 @@ subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable) subdir-ccflags-y += $(call cc-disable-warning, sign-compare) subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized) subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) +subdir-ccflags-y += $(call cc-disable-warning, tautological-constant-out-of-range-compare) subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror # Fine grained warnings disable -- 2.25.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare
A recent commit in clang added -Wtautological-compare to -Wall, which is enabled for i915 so we see the following warning: ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: result of comparison of constant 576460752303423487 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare] if (unlikely(remain > N_RELOC(ULONG_MAX))) ^ This warning only happens on x86_64 but that check is relevant for 32-bit x86 so we cannot remove it. -Wtautological-compare on a whole has good warnings but this one is not really relevant for the kernel because of all of the different configurations that are used to build the kernel. When -Wtautological-compare is enabled for the kernel, this option will remain disabled so do that for i915 now. Link: https://github.com/ClangBuiltLinux/linux/issues/778 Signed-off-by: Nathan Chancellor --- v1 -> v2: https://lore.kernel.org/lkml/20200211050808.29463-1-natechancel...@gmail.com/ * Fix patch application due to basing on a local tree that had -Wuninitialized turned on. Can confirm that patch applies on latest -next now. drivers/gpu/drm/i915/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index b8c5f8934dbd..159355eb43a9 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -22,6 +22,7 @@ subdir-ccflags-y += $(call cc-disable-warning, sign-compare) subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized) subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) subdir-ccflags-y += $(call cc-disable-warning, uninitialized) +subdir-ccflags-y += $(call cc-disable-warning, tautological-constant-out-of-range-compare) subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror # Fine grained warnings disable -- 2.25.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare
On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote: > On 2020-02-11 7:13 a.m., Nathan Chancellor wrote: > > A recent commit in clang added -Wtautological-compare to -Wall, which is > > enabled for i915 so we see the following warning: > > > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > > result of comparison of constant 576460752303423487 with expression of > > type 'unsigned int' is always false > > [-Wtautological-constant-out-of-range-compare] > > if (unlikely(remain > N_RELOC(ULONG_MAX))) > > ^ > > > > This warning only happens on x86_64 but that check is relevant for > > 32-bit x86 so we cannot remove it. > > That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value > in both cases, and remain is a 32-bit value in both cases. How can it be > larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)? > Hi Michel, Can't this condition be true when UINT_MAX == ULONG_MAX? clang does not warn on a 32-bit x86 build from what I remember. Honestly, my understanding of overflow is pretty shoddy, this is mostly based on what I have heard from others. I sent a patch trying to remove that check but had it rejected: https://lore.kernel.org/lkml/20191123195321.41305-1-natechancel...@gmail.com/ Cheers, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare
On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote: > On 2020-02-11 9:39 p.m., Nathan Chancellor wrote: > > On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote: > >> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote: > >>> A recent commit in clang added -Wtautological-compare to -Wall, which is > >>> enabled for i915 so we see the following warning: > >>> > >>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > >>> result of comparison of constant 576460752303423487 with expression of > >>> type 'unsigned int' is always false > >>> [-Wtautological-constant-out-of-range-compare] > >>> if (unlikely(remain > N_RELOC(ULONG_MAX))) > >>> ^ > >>> > >>> This warning only happens on x86_64 but that check is relevant for > >>> 32-bit x86 so we cannot remove it. > >> > >> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value > >> in both cases, and remain is a 32-bit value in both cases. How can it be > >> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)? > >> > > > > Hi Michel, > > > > Can't this condition be true when UINT_MAX == ULONG_MAX? > > Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit. > > > Anyway, this suggests a possible better solution: > > #if UINT_MAX == ULONG_MAX > if (unlikely(remain > N_RELOC(ULONG_MAX))) > return -EINVAL; > #endif > > > Or if that can't be used for some reason, something like > > if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX))) > return -EINVAL; > > should silence the warning. I do like this one better than the former. > > > Either of these should be better than completely disabling the warning > for the whole file. Normally, I would agree but I am currently planning to leave -Wtautological-constant-out-of-range-compare disabled when I turn on -Wtautological-compare for the whole kernel because there are plenty of locations in the kernel where these kind of checks depend on various kernel configuration options and the general attitude of kernel developers is that this particular warning is not really helpful for that reason. I'll see if there is a general consensus before moving further since I know i915 turns on a bunch of extra warnings from the rest of the kernel (hence why we are in this situation). Cheers, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare
On Thu, Feb 13, 2020 at 04:37:15PM +0200, Jani Nikula wrote: > On Wed, 12 Feb 2020, Michel Dänzer wrote: > > On 2020-02-12 6:07 p.m., Nathan Chancellor wrote: > >> On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote: > >>> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote: > >>>> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote: > >>>>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote: > >>>>>> A recent commit in clang added -Wtautological-compare to -Wall, which > >>>>>> is > >>>>>> enabled for i915 so we see the following warning: > >>>>>> > >>>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > >>>>>> result of comparison of constant 576460752303423487 with expression of > >>>>>> type 'unsigned int' is always false > >>>>>> [-Wtautological-constant-out-of-range-compare] > >>>>>> if (unlikely(remain > N_RELOC(ULONG_MAX))) > >>>>>> ^ > >>>>>> > >>>>>> This warning only happens on x86_64 but that check is relevant for > >>>>>> 32-bit x86 so we cannot remove it. > >>>>> > >>>>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value > >>>>> in both cases, and remain is a 32-bit value in both cases. How can it be > >>>>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)? > >>>>> > >>>> > >>>> Hi Michel, > >>>> > >>>> Can't this condition be true when UINT_MAX == ULONG_MAX? > >>> > >>> Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit. > >>> > >>> > >>> Anyway, this suggests a possible better solution: > >>> > >>> #if UINT_MAX == ULONG_MAX > >>> if (unlikely(remain > N_RELOC(ULONG_MAX))) > >>> return -EINVAL; > >>> #endif > >>> > >>> > >>> Or if that can't be used for some reason, something like > >>> > >>> if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX))) > >>> return -EINVAL; > >>> > >>> should silence the warning. > >> > >> I do like this one better than the former. > > > > FWIW, one downside of this one compared to all alternatives (presumably) > > is that it might end up generating actual code even on 64-bit, which > > always ends up skipping the return. > > I like this better than the UINT_MAX == ULONG_MAX comparison because > that creates a dependency on the type of remain. > > Then again, a sufficiently clever compiler could see through the cast, > and flag the warning anyway... Would you prefer a patch that adds that cast rather than silencing the warning outright? It does appear to work for clang. Cheers, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare
On Thu, Feb 13, 2020 at 02:43:21PM -0800, Nick Desaulniers wrote: > On Wed, Feb 12, 2020 at 9:17 AM Michel Dänzer wrote: > > > > On 2020-02-12 6:07 p.m., Nathan Chancellor wrote: > > > On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote: > > >> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote: > > >>> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote: > > >>>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote: > > >>>>> A recent commit in clang added -Wtautological-compare to -Wall, which > > >>>>> is > > >>>>> enabled for i915 so we see the following warning: > > >>>>> > > >>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > > >>>>> result of comparison of constant 576460752303423487 with expression of > > >>>>> type 'unsigned int' is always false > > >>>>> [-Wtautological-constant-out-of-range-compare] > > >>>>> if (unlikely(remain > N_RELOC(ULONG_MAX))) > > >>>>> ^ > > >>>>> > > >>>>> This warning only happens on x86_64 but that check is relevant for > > >>>>> 32-bit x86 so we cannot remove it. > > >>>> > > >>>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value > > >>>> in both cases, and remain is a 32-bit value in both cases. How can it > > >>>> be > > >>>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)? > > >>>> > > >>> > > >>> Hi Michel, > > >>> > > >>> Can't this condition be true when UINT_MAX == ULONG_MAX? > > >> > > >> Oh, right, I think I was wrongly thinking long had 64 bits even on > > >> 32-bit. > > >> > > >> > > >> Anyway, this suggests a possible better solution: > > >> > > >> #if UINT_MAX == ULONG_MAX > > >> if (unlikely(remain > N_RELOC(ULONG_MAX))) > > >> return -EINVAL; > > >> #endif > > >> > > >> > > >> Or if that can't be used for some reason, something like > > >> > > >> if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX))) > > >> return -EINVAL; > > >> > > >> should silence the warning. > > > > > > I do like this one better than the former. > > > > FWIW, one downside of this one compared to all alternatives (presumably) > > is that it might end up generating actual code even on 64-bit, which > > always ends up skipping the return. > > The warning is pointing out that the conditional is always false, > which is correct on 64b. The check is only active for 32b. > https://godbolt.org/z/oQrgT_ > The cast silences the warning for 64b. (Note that GCC and Clang also > generate precisely the same instruction sequences in my example, just > GCC doesn't warn on such tautologies). Thanks for confirming! I'll send a patch to add the cast later tonight. Cheers, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Cast remain to unsigned long in eb_relocate_vma
A recent commit in clang added -Wtautological-compare to -Wall, which is enabled for i915 after -Wtautological-compare is disabled for the rest of the kernel so we see the following warning on x86_64: ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433:22: warning: result of comparison of constant 576460752303423487 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare] if (unlikely(remain > N_RELOC(ULONG_MAX))) ^ ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely' # define unlikely(x)__builtin_expect(!!(x), 0) ^ 1 warning generated. It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not account for the case where this file is built for 32-bit x86, where ULONG_MAX == UINT_MAX and this check is still relevant. Cast remain to unsigned long, which keeps the generated code the same (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and the warning is silenced so we can catch more potential issues in the future. Link: https://github.com/ClangBuiltLinux/linux/issues/778 Suggested-by: Michel Dänzer Signed-off-by: Nathan Chancellor --- Round 3 :) Previous threads/patches: https://lore.kernel.org/lkml/20191123195321.41305-1-natechancel...@gmail.com/ https://lore.kernel.org/lkml/20200211050808.29463-1-natechancel...@gmail.com/ drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 60c984e10c4a..47f4d8ab281e 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1430,7 +1430,7 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct i915_vma *vma) urelocs = u64_to_user_ptr(entry->relocs_ptr); remain = entry->relocation_count; - if (unlikely(remain > N_RELOC(ULONG_MAX))) + if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX))) return -EINVAL; /* -- 2.25.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Cast remain to unsigned long in eb_relocate_vma
On Fri, Feb 14, 2020 at 08:32:19AM +, Chris Wilson wrote: > Quoting Jani Nikula (2020-02-14 06:36:15) > > On Thu, 13 Feb 2020, Nathan Chancellor wrote: > > > A recent commit in clang added -Wtautological-compare to -Wall, which is > > > enabled for i915 after -Wtautological-compare is disabled for the rest > > > of the kernel so we see the following warning on x86_64: > > > > > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433:22: warning: > > > result of comparison of constant 576460752303423487 with expression of > > > type 'unsigned int' is always false > > > [-Wtautological-constant-out-of-range-compare] > > > if (unlikely(remain > N_RELOC(ULONG_MAX))) > > > ^ > > > ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely' > > > # define unlikely(x)__builtin_expect(!!(x), 0) > > > ^ > > > 1 warning generated. > > > > > > It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not > > > account for the case where this file is built for 32-bit x86, where > > > ULONG_MAX == UINT_MAX and this check is still relevant. > > > > > > Cast remain to unsigned long, which keeps the generated code the same > > > (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and > > > the warning is silenced so we can catch more potential issues in the > > > future. > > > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/778 > > > Suggested-by: Michel Dänzer > > > Signed-off-by: Nathan Chancellor > > > > Works for me as a workaround, > > But the whole point was that the compiler could see that it was > impossible and not emit the code. Doesn't this break that? > -Chris As noted in the commit message, I ran diff <(objdump -Dr) <(objdump -Dr) on objects files compiled with and without the patch with clang and gcc for x86_64 and gcc for i386 (i386 does not build with clang) and there was zero difference aside from the file names. At the end of the day, I do not really care how the warning get fixed, just that it does since it is the only one on x86_64 defconfig. Cheers, Nathan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v1] drm/i915: fix null pointer dereference
Hi all, Apologies in advance if you see this twice. I did not see the original make it to either lore.kernel.org or the freedesktop.org archives so I figured it might have been sent into the void. On Tue, Feb 01, 2022 at 04:33:54PM +0100, Lukasz Bartosik wrote: > From: Łukasz Bartosik > > Asus chromebook CX550 crashes during boot on v5.17-rc1 kernel. > The root cause is null pointer defeference of bi_next > in tgl_get_bw_info() in drivers/gpu/drm/i915/display/intel_bw.c. > > BUG: kernel NULL pointer dereference, address: 002e > PGD 0 P4D 0 > Oops: 0002 [#1] PREEMPT SMP NOPTI > CPU: 0 PID: 1 Comm: swapper/0 Tainted: G U5.17.0-rc1 > Hardware name: Google Delbin/Delbin, BIOS Google_Delbin.13672.156.3 05/14/2021 > RIP: 0010:tgl_get_bw_info+0x2de/0x510 > ... > [2.554467] Call Trace: > [2.554467] > [2.554467] intel_bw_init_hw+0x14a/0x434 > [2.554467] ? _printk+0x59/0x73 > [2.554467] ? _dev_err+0x77/0x91 > [2.554467] i915_driver_hw_probe+0x329/0x33e > [2.554467] i915_driver_probe+0x4c8/0x638 > [2.554467] i915_pci_probe+0xf8/0x14e > [2.554467] ? _raw_spin_unlock_irqrestore+0x12/0x2c > [2.554467] pci_device_probe+0xaa/0x142 > [2.554467] really_probe+0x13f/0x2f4 > [2.554467] __driver_probe_device+0x9e/0xd3 > [2.554467] driver_probe_device+0x24/0x7c > [2.554467] __driver_attach+0xba/0xcf > [2.554467] ? driver_attach+0x1f/0x1f > [2.554467] bus_for_each_dev+0x8c/0xc0 > [2.554467] bus_add_driver+0x11b/0x1f7 > [2.554467] driver_register+0x60/0xea > [2.554467] ? mipi_dsi_bus_init+0x16/0x16 > [2.554467] i915_init+0x2c/0xb9 > [2.554467] ? mipi_dsi_bus_init+0x16/0x16 > [2.554467] do_one_initcall+0x12e/0x2b3 > [2.554467] do_initcall_level+0xd6/0xf3 > [2.554467] do_initcalls+0x4e/0x79 > [2.554467] kernel_init_freeable+0xed/0x14d > [2.554467] ? rest_init+0xc1/0xc1 > [2.554467] kernel_init+0x1a/0x120 > [2.554467] ret_from_fork+0x1f/0x30 > [2.554467] > ... > Kernel panic - not syncing: Fatal exception > > Fixes: c64a9a7c05be ("drm/i915: Update memory bandwidth formulae") > Signed-off-by: Łukasz Bartosik > --- > drivers/gpu/drm/i915/display/intel_bw.c | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c > b/drivers/gpu/drm/i915/display/intel_bw.c > index 2da4aacc956b..bd0ed68b7faa 100644 > --- a/drivers/gpu/drm/i915/display/intel_bw.c > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > @@ -404,15 +404,17 @@ static int tgl_get_bw_info(struct drm_i915_private > *dev_priv, const struct intel > int clpchgroup; > int j; > > - if (i < num_groups - 1) > - bi_next = &dev_priv->max_bw[i + 1]; > - > clpchgroup = (sa->deburst * qi.deinterleave / num_channels) << > i; > > - if (i < num_groups - 1 && clpchgroup < clperchgroup) > - bi_next->num_planes = (ipqdepth - clpchgroup) / > clpchgroup + 1; > - else > - bi_next->num_planes = 0; > + if (i < num_groups - 1) { > + bi_next = &dev_priv->max_bw[i + 1]; > + > + if (clpchgroup < clperchgroup) > + bi_next->num_planes = (ipqdepth - clpchgroup) / > +clpchgroup + 1; > + else > + bi_next->num_planes = 0; > + } > > bi->num_qgv_points = qi.num_points; > bi->num_psf_gv_points = qi.num_psf_points; > -- > 2.35.0.rc2.247.g8bbb082509-goog > > Was this patch ever applied or was the issue fixed in a different way? If CONFIG_INIT_STACK_ALL_ZERO is enabled (it is on by default when the compiler supports it), bi_next will be deterministically initialized to NULL, which means 'bi_next->num_planes = 0' will crash when the first if statement is not taken (i.e. 'i > num_groups - 1'). This was reported to us at [1] so it impacts real users (and I have been applying this change locally for six months). I see some discussion in this thread, was it ever resolved? [1]: https://github.com/ClangBuiltLinux/linux/issues/1626 Cheers, Nathan
Re: [Intel-gfx] [PATCH v1] drm/i915: fix null pointer dereference
Hi all, On Tue, Feb 01, 2022 at 04:33:54PM +0100, Lukasz Bartosik wrote: > From: Łukasz Bartosik > > Asus chromebook CX550 crashes during boot on v5.17-rc1 kernel. > The root cause is null pointer defeference of bi_next > in tgl_get_bw_info() in drivers/gpu/drm/i915/display/intel_bw.c. > > BUG: kernel NULL pointer dereference, address: 002e > PGD 0 P4D 0 > Oops: 0002 [#1] PREEMPT SMP NOPTI > CPU: 0 PID: 1 Comm: swapper/0 Tainted: G U5.17.0-rc1 > Hardware name: Google Delbin/Delbin, BIOS Google_Delbin.13672.156.3 05/14/2021 > RIP: 0010:tgl_get_bw_info+0x2de/0x510 > ... > [2.554467] Call Trace: > [2.554467] > [2.554467] intel_bw_init_hw+0x14a/0x434 > [2.554467] ? _printk+0x59/0x73 > [2.554467] ? _dev_err+0x77/0x91 > [2.554467] i915_driver_hw_probe+0x329/0x33e > [2.554467] i915_driver_probe+0x4c8/0x638 > [2.554467] i915_pci_probe+0xf8/0x14e > [2.554467] ? _raw_spin_unlock_irqrestore+0x12/0x2c > [2.554467] pci_device_probe+0xaa/0x142 > [2.554467] really_probe+0x13f/0x2f4 > [2.554467] __driver_probe_device+0x9e/0xd3 > [2.554467] driver_probe_device+0x24/0x7c > [2.554467] __driver_attach+0xba/0xcf > [2.554467] ? driver_attach+0x1f/0x1f > [2.554467] bus_for_each_dev+0x8c/0xc0 > [2.554467] bus_add_driver+0x11b/0x1f7 > [2.554467] driver_register+0x60/0xea > [2.554467] ? mipi_dsi_bus_init+0x16/0x16 > [2.554467] i915_init+0x2c/0xb9 > [2.554467] ? mipi_dsi_bus_init+0x16/0x16 > [2.554467] do_one_initcall+0x12e/0x2b3 > [2.554467] do_initcall_level+0xd6/0xf3 > [2.554467] do_initcalls+0x4e/0x79 > [2.554467] kernel_init_freeable+0xed/0x14d > [2.554467] ? rest_init+0xc1/0xc1 > [2.554467] kernel_init+0x1a/0x120 > [2.554467] ret_from_fork+0x1f/0x30 > [2.554467] > ... > Kernel panic - not syncing: Fatal exception > > Fixes: c64a9a7c05be ("drm/i915: Update memory bandwidth formulae") > Signed-off-by: Łukasz Bartosik > --- > drivers/gpu/drm/i915/display/intel_bw.c | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c > b/drivers/gpu/drm/i915/display/intel_bw.c > index 2da4aacc956b..bd0ed68b7faa 100644 > --- a/drivers/gpu/drm/i915/display/intel_bw.c > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > @@ -404,15 +404,17 @@ static int tgl_get_bw_info(struct drm_i915_private > *dev_priv, const struct intel > int clpchgroup; > int j; > > - if (i < num_groups - 1) > - bi_next = &dev_priv->max_bw[i + 1]; > - > clpchgroup = (sa->deburst * qi.deinterleave / num_channels) << > i; > > - if (i < num_groups - 1 && clpchgroup < clperchgroup) > - bi_next->num_planes = (ipqdepth - clpchgroup) / > clpchgroup + 1; > - else > - bi_next->num_planes = 0; > + if (i < num_groups - 1) { > + bi_next = &dev_priv->max_bw[i + 1]; > + > + if (clpchgroup < clperchgroup) > + bi_next->num_planes = (ipqdepth - clpchgroup) / > +clpchgroup + 1; > + else > + bi_next->num_planes = 0; > + } > > bi->num_qgv_points = qi.num_points; > bi->num_psf_gv_points = qi.num_psf_points; > -- > 2.35.0.rc2.247.g8bbb082509-goog > > Was this patch ever applied or was the issue fixed in a different way? If CONFIG_INIT_STACK_ALL_ZERO is enabled (it is on by default when the compiler supports it), bi_next will be deterministically initialized to NULL, which means 'bi_next->num_planes = 0' will crash when the first if statement is not taken (i.e. 'i > num_groups - 1'). This was reported to us at [1] so it impacts real users (and I have been applying this change locally for six months). I see some discussion in the thread, was it ever resolved? [1]: https://github.com/ClangBuiltLinux/linux/issues/1626 Cheers, Nathan
Re: [Intel-gfx] [PATCH v1] drm/i915: fix null pointer dereference
On Thu, Aug 25, 2022 at 10:37:14AM +0300, Jani Nikula wrote: > On Tue, 23 Aug 2022, Łukasz Bartosik wrote: > >> > >> Hi all, > >> > >> Apologies in advance if you see this twice. I did not see the original > >> make it to either lore.kernel.org or the freedesktop.org archives so I > >> figured it might have been sent into the void. > >> > >> On Tue, Feb 01, 2022 at 04:33:54PM +0100, Lukasz Bartosik wrote: > >> > From: Łukasz Bartosik > >> > > >> > Asus chromebook CX550 crashes during boot on v5.17-rc1 kernel. > >> > The root cause is null pointer defeference of bi_next > >> > in tgl_get_bw_info() in drivers/gpu/drm/i915/display/intel_bw.c. > >> > > >> > BUG: kernel NULL pointer dereference, address: 002e > >> > PGD 0 P4D 0 > >> > Oops: 0002 [#1] PREEMPT SMP NOPTI > >> > CPU: 0 PID: 1 Comm: swapper/0 Tainted: G U5.17.0-rc1 > >> > Hardware name: Google Delbin/Delbin, BIOS Google_Delbin.13672.156.3 > >> > 05/14/2021 > >> > RIP: 0010:tgl_get_bw_info+0x2de/0x510 > >> > ... > >> > [2.554467] Call Trace: > >> > [2.554467] > >> > [2.554467] intel_bw_init_hw+0x14a/0x434 > >> > [2.554467] ? _printk+0x59/0x73 > >> > [2.554467] ? _dev_err+0x77/0x91 > >> > [2.554467] i915_driver_hw_probe+0x329/0x33e > >> > [2.554467] i915_driver_probe+0x4c8/0x638 > >> > [2.554467] i915_pci_probe+0xf8/0x14e > >> > [2.554467] ? _raw_spin_unlock_irqrestore+0x12/0x2c > >> > [2.554467] pci_device_probe+0xaa/0x142 > >> > [2.554467] really_probe+0x13f/0x2f4 > >> > [2.554467] __driver_probe_device+0x9e/0xd3 > >> > [2.554467] driver_probe_device+0x24/0x7c > >> > [2.554467] __driver_attach+0xba/0xcf > >> > [2.554467] ? driver_attach+0x1f/0x1f > >> > [2.554467] bus_for_each_dev+0x8c/0xc0 > >> > [2.554467] bus_add_driver+0x11b/0x1f7 > >> > [2.554467] driver_register+0x60/0xea > >> > [2.554467] ? mipi_dsi_bus_init+0x16/0x16 > >> > [2.554467] i915_init+0x2c/0xb9 > >> > [2.554467] ? mipi_dsi_bus_init+0x16/0x16 > >> > [2.554467] do_one_initcall+0x12e/0x2b3 > >> > [2.554467] do_initcall_level+0xd6/0xf3 > >> > [2.554467] do_initcalls+0x4e/0x79 > >> > [2.554467] kernel_init_freeable+0xed/0x14d > >> > [2.554467] ? rest_init+0xc1/0xc1 > >> > [2.554467] kernel_init+0x1a/0x120 > >> > [2.554467] ret_from_fork+0x1f/0x30 > >> > [2.554467] > >> > ... > >> > Kernel panic - not syncing: Fatal exception > >> > > >> > Fixes: c64a9a7c05be ("drm/i915: Update memory bandwidth formulae") > >> > Signed-off-by: Łukasz Bartosik > >> > --- > >> > drivers/gpu/drm/i915/display/intel_bw.c | 16 +--- > >> > 1 file changed, 9 insertions(+), 7 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c > >> > b/drivers/gpu/drm/i915/display/intel_bw.c > >> > index 2da4aacc956b..bd0ed68b7faa 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_bw.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > >> > @@ -404,15 +404,17 @@ static int tgl_get_bw_info(struct drm_i915_private > >> > *dev_priv, const struct intel > >> > int clpchgroup; > >> > int j; > >> > > >> > - if (i < num_groups - 1) > >> > - bi_next = &dev_priv->max_bw[i + 1]; > >> > - > >> > clpchgroup = (sa->deburst * qi.deinterleave / > >> > num_channels) << i; > >> > > >> > - if (i < num_groups - 1 && clpchgroup < clperchgroup) > >> > - bi_next->num_planes = (ipqdepth - clpchgroup) / > >> > clpchgroup + 1; > >> > - else > >> > - bi_next->num_planes = 0; > >> > + if (i < num_groups - 1) { > >> > + bi_next = &dev_priv->max_bw[i + 1]; > >> > + > >> > + if (clpchgroup < clperchgroup) > >> > + bi_next->num_planes = (ipqdepth - > >> > clpchgroup) / > >> > +clpchgroup + 1; > >> > + else > >> > + bi_next->num_planes = 0; > >> > + } > >> > > >> > bi->num_qgv_points = qi.num_points; > >> > bi->num_psf_gv_points = qi.num_psf_points; > >> > -- > >> > 2.35.0.rc2.247.g8bbb082509-goog > >> > > >> > > >> > >> Was this patch ever applied or was the issue fixed in a different way? > >> If CONFIG_INIT_STACK_ALL_ZERO is enabled (it is on by default when the > >> compiler supports it), bi_next will be deterministically initialized to > >> NULL, which means 'bi_next->num_planes = 0' will crash when the first if > >> statement is not taken (i.e. 'i > num_groups - 1'). This was reported to > >> us at [1] so it impacts real users (and I have been applying this change > >> locally for six months). I see some discussion in this thread, was it > >> ever resolved? > >> > >> [1]: https://github.com/ClangBuiltLinux/linux/issues/1626 > >> > >> Cheers, > >> Nathan > > > > The patch was n
Re: [Intel-gfx] [PATCH v3 14/17] fbcon: Move console_lock for register/unlink/unregister
Hi Daniel, On Tue, Apr 05, 2022 at 11:03:32PM +0200, Daniel Vetter wrote: > Ideally console_lock becomes an implementation detail of fbcon.c and > doesn't show up anywhere in fbmem.c. We're still pretty far from that, > but at least the register/unregister code is there now. > > With this the do_fb_ioctl() handler is the only code in fbmem.c still > calling console_lock(). > > Acked-by: Sam Ravnborg > Signed-off-by: Daniel Vetter > Cc: Daniel Vetter > Cc: Thomas Zimmermann > Cc: Du Cheng > Cc: Claudio Suarez > Cc: Greg Kroah-Hartman > Cc: Tetsuo Handa > Cc: Matthew Wilcox > Cc: Sam Ravnborg > Cc: Zheyu Ma > Cc: Guenter Roeck > Cc: Alex Deucher > Cc: Zhen Lei > Cc: Xiyu Yang This patch as commit 6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister") in next-20220411 causes a lock up on my test Intel desktop; I get no output on my display and I cannot ssh into the machine. Bisect log below. If back out of this patch and the ones that follow in this series on top of next-20220411, everything works properly (i.e. 'git diff 6e7da3af008b^..efc3acbc105a | git apply -R'). What information would be helpful for debugging this? The system has an i7-11700 in it and it is booting under UEFI, so I assume it should be using EFI_FB. I am happy to offer any debugging information or test any patches. # bad: [d12d7e1cfe38e0c36d28c7a9fbbc436ad0d17c14] Add linux-next specific files for 20220411 # good: [8b57b3046107b50ebecb65537a172ef3d6cec673] Merge tag 'tty-5.18-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty git bisect start 'd12d7e1cfe38e0c36d28c7a9fbbc436ad0d17c14' '8b57b3046107b50ebecb65537a172ef3d6cec673' # bad: [8f0b3ef070bd35e80da1caa85824fc344fb6b82e] Merge branch 'for-linux-next' of git://anongit.freedesktop.org/drm/drm-misc git bisect bad 8f0b3ef070bd35e80da1caa85824fc344fb6b82e # good: [69ae2d6587df089ec1c21bc7852df403d3ce] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git git bisect good 69ae2d6587df089ec1c21bc7852df403d3ce # good: [43ffc7321ed6ce13e5424fa687576f4442c25f9d] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git git bisect good 43ffc7321ed6ce13e5424fa687576f4442c25f9d # good: [d44c2642c40b6c11e4a0afc76d491287c074376c] drm/gem: Delete gem array fencing helpers git bisect good d44c2642c40b6c11e4a0afc76d491287c074376c # good: [3223e922ccf8b5c3dd0b05faeaba407655ee0774] orinoco: Prepare cleanup of powerpc's asm/prom.h git bisect good 3223e922ccf8b5c3dd0b05faeaba407655ee0774 # good: [689333136327b6cd618df85d83d79f2aa620d585] fbcon: Move fbcon_bmove(_rec) functions git bisect good 689333136327b6cd618df85d83d79f2aa620d585 # good: [840db0076b05a1dd4c2ded8d08f8481e0297d09f] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git git bisect good 840db0076b05a1dd4c2ded8d08f8481e0297d09f # bad: [6e7da3af008b72520f5318507f455f344b27f022] fbcon: Move console_lock for register/unlink/unregister git bisect bad 6e7da3af008b72520f5318507f455f344b27f022 # good: [6b2060cf9138a2cd5f3468a949d3869abed049ef] fb: Delete fb_info->queue git bisect good 6b2060cf9138a2cd5f3468a949d3869abed049ef # good: [d443d93864726ad68c0a741d1e7b03934a9af143] fbcon: move more common code into fb_open() git bisect good d443d93864726ad68c0a741d1e7b03934a9af143 # good: [43553559121ca90965b572cf8a1d6d0fd618b449] fbcon: Consistently protect deferred_takeover with console_lock() git bisect good 43553559121ca90965b572cf8a1d6d0fd618b449 # first bad commit: [6e7da3af008b72520f5318507f455f344b27f022] fbcon: Move console_lock for register/unlink/unregister Cheers, Nathan
Re: [Intel-gfx] [PATCH] fbcon: Fix delayed takeover locking
On Wed, Apr 13, 2022 at 11:25:02AM +0200, Daniel Vetter wrote: > On Wed, Apr 13, 2022 at 10:21:28AM +0200, Daniel Vetter wrote: > > I messed up the delayed takover path in the locking conversion in > > 6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister"). > > > > Fix it by re-extracting the lockless function and using it in the > > delayed takeover path, where we need to hold the lock already to > > iterate over the list of already registered fb. Well the current code > > still is broken in there (since the list is protected by a > > registration_lock, which we can't take here because it nests the other > > way round with console_lock), but in the future this will be a list > > protected by console_lock when this is all sorted out. > > > > Reported-by: Nathan Chancellor > > Cc: Nathan Chancellor > > Nathan, if you can supply a tested-by today still I could push it before I > disappear into easter w/e. I'm pretty sure this is it, but better safe > than sorry. > -Daniel Yup, sorry for the delay, timezones and such :( This patch resolves the problem I was seeing, thank you for the quick response and fix! Tested-by: Nathan Chancellor > > Fixes: 6e7da3af008b ("fbcon: Move console_lock for > > register/unlink/unregister") > > Cc: Sam Ravnborg > > Cc: Thomas Zimmermann > > Cc: Du Cheng > > Cc: Claudio Suarez > > Cc: Greg Kroah-Hartman > > Cc: Tetsuo Handa > > Cc: Matthew Wilcox > > Cc: Zheyu Ma > > Cc: Guenter Roeck > > Cc: Helge Deller > > Cc: Geert Uytterhoeven > > Cc: Javier Martinez Canillas > > Signed-off-by: Daniel Vetter > > --- > > drivers/video/fbdev/core/fbcon.c | 28 ++-- > > 1 file changed, 18 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/video/fbdev/core/fbcon.c > > b/drivers/video/fbdev/core/fbcon.c > > index 6a7d470beec7..b4e43b39d9a8 100644 > > --- a/drivers/video/fbdev/core/fbcon.c > > +++ b/drivers/video/fbdev/core/fbcon.c > > @@ -2772,7 +2772,6 @@ static void fbcon_unbind(void) > > static inline void fbcon_unbind(void) {} > > #endif /* CONFIG_VT_HW_CONSOLE_BINDING */ > > > > -/* called with console_lock held */ > > void fbcon_fb_unbind(struct fb_info *info) > > { > > int i, new_idx = -1; > > @@ -2822,7 +2821,6 @@ void fbcon_fb_unbind(struct fb_info *info) > > console_unlock(); > > } > > > > -/* called with console_lock held */ > > void fbcon_fb_unregistered(struct fb_info *info) > > { > > int i, idx; > > @@ -2928,14 +2926,11 @@ MODULE_PARM_DESC(lockless_register_fb, > > "Lockless framebuffer registration for debugging [default=off]"); > > > > /* called with console_lock held */ > > -int fbcon_fb_registered(struct fb_info *info) > > +static int do_fb_registered(struct fb_info *info) > > { > > int ret = 0, i, idx; > > > > - if (!lockless_register_fb) > > - console_lock(); > > - else > > - atomic_inc(&ignore_console_lock_warning); > > + WARN_CONSOLE_UNLOCKED(); > > > > fbcon_registered_fb[info->node] = info; > > fbcon_num_registered_fb++; > > @@ -2945,7 +2940,7 @@ int fbcon_fb_registered(struct fb_info *info) > > > > if (deferred_takeover) { > > pr_info("fbcon: Deferring console take-over\n"); > > - goto out; > > + return 0; > > } > > > > if (info_idx == -1) { > > @@ -2965,7 +2960,20 @@ int fbcon_fb_registered(struct fb_info *info) > > } > > } > > > > -out: > > + return ret; > > +} > > + > > +int fbcon_fb_registered(struct fb_info *info) > > +{ > > + int ret; > > + > > + if (!lockless_register_fb) > > + console_lock(); > > + else > > + atomic_inc(&ignore_console_lock_warning); > > + > > + ret = do_fb_registered(info); > > + > > if (!lockless_register_fb) > > console_unlock(); > > else > > @@ -3280,7 +3288,7 @@ static void fbcon_register_existing_fbs(struct > > work_struct *work) > > logo_shown = FBCON_LOGO_DONTSHOW; > > > > fbcon_for_each_registered_fb(i) > > - fbcon_fb_registered(fbcon_registered_fb[i]); > > + do_fb_registered(fbcon_registered_fb[i]); > > > > console_unlock(); > > } > > -- > > 2.34.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Re: [Intel-gfx] linux-next: manual merge of the drm-misc tree with the origin tree
Hi Stephen, On Wed, Nov 16, 2022 at 10:57:02AM +1100, Stephen Rothwell wrote: > Hi all, > > Today's linux-next merge of the drm-misc tree got a conflict in: > > drivers/gpu/drm/vc4/vc4_hdmi.c > > between commit: > > 682f99b8ae88 ("drm/vc4: hdmi: Take our lock to reset the link") > > from the origin tree and commits: > > d218750805a3 ("drm/vc4: hdmi: Pass vc4_hdmi to > vc4_hdmi_supports_scrambling()") > 0a99962c0dbf ("drm/vc4: hdmi: Fix pointer dereference before check") > > from the drm-misc tree. > > I fixed it up (see below) and can carry the fix as necessary. This > is now fixed as far as linux-next is concerned, but any non trivial > conflicts should be mentioned to your upstream maintainer when your tree > is submitted for merging. You may also want to consider cooperating > with the maintainer of the conflicting tree to minimise any particularly > complex conflicts. > > -- > Cheers, > Stephen Rothwell > > diff --cc drivers/gpu/drm/vc4/vc4_hdmi.c > index d7fcc7a4c082,6b223a5fcf6f.. > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@@ -349,12 -348,9 +348,13 @@@ static int vc4_hdmi_reset_link(struct d > if (!crtc_state->active) > return 0; > > +mutex_lock(&vc4_hdmi->mutex); > + > - if (!vc4_hdmi_supports_scrambling(encoder)) { > + vc4_hdmi = connector_to_vc4_hdmi(connector); > -if (!vc4_hdmi_supports_scrambling(vc4_hdmi)) > ++if (!vc4_hdmi_supports_scrambling(vc4_hdmi)) { > +mutex_unlock(&vc4_hdmi->mutex); > return 0; > +} > > scrambling_needed = > vc4_hdmi_mode_needs_scrambling(&vc4_hdmi->saved_adjusted_mode, > vc4_hdmi->output_bpc, This resolution is not quite right, as pointed out by clang: drivers/gpu/drm/vc4/vc4_hdmi.c:351:14: error: variable 'vc4_hdmi' is uninitialized when used here [-Werror,-Wuninitialized] mutex_lock(&vc4_hdmi->mutex); ^~~~ ./include/linux/mutex.h:187:44: note: expanded from macro 'mutex_lock' #define mutex_lock(lock) mutex_lock_nested(lock, 0) ^~~~ drivers/gpu/drm/vc4/vc4_hdmi.c:322:27: note: initialize the variable 'vc4_hdmi' to silence this warning struct vc4_hdmi *vc4_hdmi; ^ = NULL 1 error generated. Obviously, the assignment of vc4_hdmi should be before mutex_lock(). Cheers, Nathan
Re: [Intel-gfx] linux-next: manual merge of the drm-misc tree with the origin tree
On Fri, Nov 18, 2022 at 09:06:36AM +1100, Stephen Rothwell wrote: > Hi Nathan, > > On Thu, 17 Nov 2022 10:29:33 -0700 Nathan Chancellor > wrote: > > > > This resolution is not quite right, as pointed out by clang: > > > > drivers/gpu/drm/vc4/vc4_hdmi.c:351:14: error: variable 'vc4_hdmi' is > > uninitialized when used here [-Werror,-Wuninitialized] > > mutex_lock(&vc4_hdmi->mutex); > > ^~~~ > > ./include/linux/mutex.h:187:44: note: expanded from macro 'mutex_lock' > > #define mutex_lock(lock) mutex_lock_nested(lock, 0) > >^~~~ > > drivers/gpu/drm/vc4/vc4_hdmi.c:322:27: note: initialize the variable > > 'vc4_hdmi' to silence this warning > > struct vc4_hdmi *vc4_hdmi; > > ^ > > = NULL > > 1 error generated. > > > > Obviously, the assignment of vc4_hdmi should be before mutex_lock(). > > Thanks for pointing that out (silly me :-) ). I have fixed up the > resolution for today. Great, thank you so much! One less warning to worry about :) Cheers, Nathan
Re: [Intel-gfx] [PATCH] drm/i915: Fix CFI violations in gt_sysfs
On Fri, Sep 23, 2022 at 09:57:47PM -0700, Kees Cook wrote: > On Thu, Sep 22, 2022 at 12:51:27PM -0700, Nathan Chancellor wrote: > > [...] > > To make everything work properly, adjust certain functions to match the > > type of the ->show() and ->store() members in 'struct kobj_attribute'. > > Add a macro to generate functions for that can be called via both > > dev_attr_{show,store}() or kobj_attr_{show,store}() so that they can be > > called through both kobject locations without violating kCFI and adjust > > the attribute groups to account for this. > > This was quite a roller coaster! I think the solution looks good, even > if I'm suspicious of the original design that has the same stuff > available twice in different places. (I have a dim memory of rdma > needing a refactoring like this too?) Right, I noticed this comment in intel_gt_sysfs_register() once I fully saw what was going on: /* * We need to make things right with the * ABI compatibility. The files were originally * generated under the parent directory. * * We generate the files only for gt 0 * to avoid duplicates. */ Makes it seem like there will be userspace breakage if these files do not exist? I figured this was the cleanest solution within those parameters. > Reviewed-by: Kees Cook Thanks for looking it over! Cheers, Nathan
Re: [Intel-gfx] [PATCH] drm/i915: Fix CFI violations in gt_sysfs
On Thu, Sep 29, 2022 at 06:46:34PM +0200, Andi Shyti wrote: > Hi Nathan, > > thanks for this refactoring... looks good even though i would > have split it in more patches as this is doing quite many things. Right, sorry about that :/ I initially thought the problem was much simpler and the diff was more reasonable before I truly saw what was happening and by that point, trying to break things apart felt like navigating a mine field. I will definitely keep that in mind for the future though. > But I will not be stubborn, I understand that it's not trivial to > have things split. I will give my r-b for now but I will check it > again before applying it as it's very easy to get tangled-up in > between all those defines: > > Reviewed-by: Andi Shyti Appreciate it! I don't have access to some of the hardware that is special cased in this code so I definitely would not mind some additional eyes and testing for this change. > For now I'm quite surprised to see how easily our CI gives green > lights :-P > > On Sat, Sep 24, 2022 at 09:39:30PM -0700, Nathan Chancellor wrote: > > On Fri, Sep 23, 2022 at 09:57:47PM -0700, Kees Cook wrote: > > > On Thu, Sep 22, 2022 at 12:51:27PM -0700, Nathan Chancellor wrote: > > > > [...] > > > > To make everything work properly, adjust certain functions to match the > > > > type of the ->show() and ->store() members in 'struct kobj_attribute'. > > > > Add a macro to generate functions for that can be called via both > > > > dev_attr_{show,store}() or kobj_attr_{show,store}() so that they can be > > > > called through both kobject locations without violating kCFI and adjust > > > > the attribute groups to account for this. > > > > > > This was quite a roller coaster! I think the solution looks good, even > > > if I'm suspicious of the original design that has the same stuff > > > available twice in different places. (I have a dim memory of rdma > > > needing a refactoring like this too?) > > > > Right, I noticed this comment in intel_gt_sysfs_register() once I fully > > saw what was going on: > > > > /* > > * We need to make things right with the > > * ABI compatibility. The files were originally > > * generated under the parent directory. > > * > > * We generate the files only for gt 0 > > * to avoid duplicates. > > */ > > > > Makes it seem like there will be userspace breakage if these files do > > not exist? I figured this was the cleanest solution within those > > parameters. > > i915 went multi-gt (multitile) so that some interfaces, like the > power management files, have effect only on one of the tiles. > This means that we needed to move some of the files inside the > newly created gt0/gt1 directory. > > But, because some of those files are part of an ABI > specification, we can't simply transfer them from the original > position so that we needed to make a "hard" copy (actually the > original files now take the role of affecting all the GTs instead > of only one). > > The complexity of this file comes from the necessity of > minimizing code duplication, otherwise we could have had two > perfectly identical files (which looking at the final result it > wouldn't have been a completely bad idea after all). > > Thanks again, will let you know when I will get this into our > branch. Ah, that all makes sense, good to know that this solution will allow all of that to continue to work. If there are any issues or further comments, I am happy to follow up in whatever way I need to. Thanks again for the review and tentative acceptance! Cheers, Nathan
Re: [Intel-gfx] [PATCH] drm/i915: Fix CFI violations in gt_sysfs
On Fri, Sep 30, 2022 at 12:34:41AM +0200, Andrzej Hajda wrote: > On 22.09.2022 21:51, Nathan Chancellor wrote: > > When booting with clang's kernel control flow integrity series [1], > > there are numerous violations when accessing the files under > > /sys/devices/pci:00/:00:02.0/drm/card0/gt/gt0: > > > >$ cd /sys/devices/pci:00/:00:02.0/drm/card0/gt/gt0 > > > >$ grep . * > >id:0 > >punit_req_freq_mhz:350 > >rc6_enable:1 > >rc6_residency_ms:214934 > >rps_act_freq_mhz:1300 > >rps_boost_freq_mhz:1300 > >rps_cur_freq_mhz:350 > >rps_max_freq_mhz:1300 > >rps_min_freq_mhz:350 > >rps_RP0_freq_mhz:1300 > >rps_RP1_freq_mhz:350 > >rps_RPn_freq_mhz:350 > >throttle_reason_pl1:0 > >throttle_reason_pl2:0 > >throttle_reason_pl4:0 > >throttle_reason_prochot:0 > >throttle_reason_ratl:0 > >throttle_reason_status:0 > >throttle_reason_thermal:0 > >throttle_reason_vr_tdc:0 > >throttle_reason_vr_thermalert:0 > > > >$ sudo dmesg &| grep "CFI failure at" > >[ 214.595903] CFI failure at kobj_attr_show+0x19/0x30 (target: > > id_show+0x0/0x70 [i915]; expected type: 0xc527b809) > >[ 214.596064] CFI failure at kobj_attr_show+0x19/0x30 (target: > > punit_req_freq_mhz_show+0x0/0x40 [i915]; expected type: 0xc527b809) > >[ 214.596407] CFI failure at kobj_attr_show+0x19/0x30 (target: > > rc6_enable_show+0x0/0x40 [i915]; expected type: 0xc527b809) > >[ 214.596528] CFI failure at kobj_attr_show+0x19/0x30 (target: > > rc6_residency_ms_show+0x0/0x270 [i915]; expected type: 0xc527b809) > >[ 214.596682] CFI failure at kobj_attr_show+0x19/0x30 (target: > > act_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > >[ 214.596792] CFI failure at kobj_attr_show+0x19/0x30 (target: > > boost_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > >[ 214.596893] CFI failure at kobj_attr_show+0x19/0x30 (target: > > cur_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > >[ 214.596996] CFI failure at kobj_attr_show+0x19/0x30 (target: > > max_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > >[ 214.597099] CFI failure at kobj_attr_show+0x19/0x30 (target: > > min_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > >[ 214.597198] CFI failure at kobj_attr_show+0x19/0x30 (target: > > RP0_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > >[ 214.597301] CFI failure at kobj_attr_show+0x19/0x30 (target: > > RP1_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > >[ 214.597405] CFI failure at kobj_attr_show+0x19/0x30 (target: > > RPn_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > >[ 214.597538] CFI failure at kobj_attr_show+0x19/0x30 (target: > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > >[ 214.597701] CFI failure at kobj_attr_show+0x19/0x30 (target: > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > >[ 214.597836] CFI failure at kobj_attr_show+0x19/0x30 (target: > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > >[ 214.597952] CFI failure at kobj_attr_show+0x19/0x30 (target: > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > >[ 214.598071] CFI failure at kobj_attr_show+0x19/0x30 (target: > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > >[ 214.598177] CFI failure at kobj_attr_show+0x19/0x30 (target: > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > >[ 214.598307] CFI failure at kobj_attr_show+0x19/0x30 (target: > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > >[ 214.598439] CFI failure at kobj_attr_show+0x19/0x30 (target: > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > >[ 214.598542] CFI failure at kobj_attr_show+0x19/0x30 (target: > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > > > With kCFI, indirect calls are validated against their expected type > > versus actual type and failures occur when the two types do not match. > > Have you tried this tool with drm subsytem, IIRC there are also similar > cases with callbacks expecting ptr to different struct than actually passed. Yes, I have also run a kCFI kernel on an AMD system that I have and I have not seen any failures from them. I only have AMD and Intel systems with graphics so there could
Re: [Intel-gfx] [PATCH] drm/i915: Fix CFI violations in gt_sysfs
Hi Andrzej, On Thu, Sep 29, 2022 at 03:44:40PM -0700, Nathan Chancellor wrote: > On Fri, Sep 30, 2022 at 12:34:41AM +0200, Andrzej Hajda wrote: > > On 22.09.2022 21:51, Nathan Chancellor wrote: > > > When booting with clang's kernel control flow integrity series [1], > > > there are numerous violations when accessing the files under > > > /sys/devices/pci:00/:00:02.0/drm/card0/gt/gt0: > > > > > >$ cd /sys/devices/pci:00/:00:02.0/drm/card0/gt/gt0 > > > > > >$ grep . * > > >id:0 > > >punit_req_freq_mhz:350 > > >rc6_enable:1 > > >rc6_residency_ms:214934 > > >rps_act_freq_mhz:1300 > > >rps_boost_freq_mhz:1300 > > >rps_cur_freq_mhz:350 > > >rps_max_freq_mhz:1300 > > >rps_min_freq_mhz:350 > > >rps_RP0_freq_mhz:1300 > > >rps_RP1_freq_mhz:350 > > >rps_RPn_freq_mhz:350 > > >throttle_reason_pl1:0 > > >throttle_reason_pl2:0 > > >throttle_reason_pl4:0 > > >throttle_reason_prochot:0 > > >throttle_reason_ratl:0 > > >throttle_reason_status:0 > > >throttle_reason_thermal:0 > > >throttle_reason_vr_tdc:0 > > >throttle_reason_vr_thermalert:0 > > > > > >$ sudo dmesg &| grep "CFI failure at" > > >[ 214.595903] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > id_show+0x0/0x70 [i915]; expected type: 0xc527b809) > > >[ 214.596064] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > punit_req_freq_mhz_show+0x0/0x40 [i915]; expected type: 0xc527b809) > > >[ 214.596407] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > rc6_enable_show+0x0/0x40 [i915]; expected type: 0xc527b809) > > >[ 214.596528] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > rc6_residency_ms_show+0x0/0x270 [i915]; expected type: 0xc527b809) > > >[ 214.596682] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > act_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.596792] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > boost_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.596893] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > cur_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.596996] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > max_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.597099] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > min_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.597198] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > RP0_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.597301] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > RP1_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.597405] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > RPn_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) > > >[ 214.597538] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.597701] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.597836] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.597952] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.598071] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.598177] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.598307] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.598439] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > >[ 214.598542] CFI failure at kobj_attr_show+0x19/0x30 (target: > > > throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) > > > > > > With kCFI, indirect calls are va
[Intel-gfx] [PATCH v2] drm/i915: Fix CFI violations in gt_sysfs
When booting with CONFIG_CFI_CLANG, there are numerous violations when accessing the files under /sys/devices/pci:00/:00:02.0/drm/card0/gt/gt0: $ cd /sys/devices/pci:00/:00:02.0/drm/card0/gt/gt0 $ grep . * id:0 punit_req_freq_mhz:350 rc6_enable:1 rc6_residency_ms:214934 rps_act_freq_mhz:1300 rps_boost_freq_mhz:1300 rps_cur_freq_mhz:350 rps_max_freq_mhz:1300 rps_min_freq_mhz:350 rps_RP0_freq_mhz:1300 rps_RP1_freq_mhz:350 rps_RPn_freq_mhz:350 throttle_reason_pl1:0 throttle_reason_pl2:0 throttle_reason_pl4:0 throttle_reason_prochot:0 throttle_reason_ratl:0 throttle_reason_status:0 throttle_reason_thermal:0 throttle_reason_vr_tdc:0 throttle_reason_vr_thermalert:0 $ sudo dmesg &| grep "CFI failure at" [ 214.595903] CFI failure at kobj_attr_show+0x19/0x30 (target: id_show+0x0/0x70 [i915]; expected type: 0xc527b809) [ 214.596064] CFI failure at kobj_attr_show+0x19/0x30 (target: punit_req_freq_mhz_show+0x0/0x40 [i915]; expected type: 0xc527b809) [ 214.596407] CFI failure at kobj_attr_show+0x19/0x30 (target: rc6_enable_show+0x0/0x40 [i915]; expected type: 0xc527b809) [ 214.596528] CFI failure at kobj_attr_show+0x19/0x30 (target: rc6_residency_ms_show+0x0/0x270 [i915]; expected type: 0xc527b809) [ 214.596682] CFI failure at kobj_attr_show+0x19/0x30 (target: act_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) [ 214.596792] CFI failure at kobj_attr_show+0x19/0x30 (target: boost_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) [ 214.596893] CFI failure at kobj_attr_show+0x19/0x30 (target: cur_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) [ 214.596996] CFI failure at kobj_attr_show+0x19/0x30 (target: max_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) [ 214.597099] CFI failure at kobj_attr_show+0x19/0x30 (target: min_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) [ 214.597198] CFI failure at kobj_attr_show+0x19/0x30 (target: RP0_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) [ 214.597301] CFI failure at kobj_attr_show+0x19/0x30 (target: RP1_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) [ 214.597405] CFI failure at kobj_attr_show+0x19/0x30 (target: RPn_freq_mhz_show+0x0/0xe0 [i915]; expected type: 0xc527b809) [ 214.597538] CFI failure at kobj_attr_show+0x19/0x30 (target: throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) [ 214.597701] CFI failure at kobj_attr_show+0x19/0x30 (target: throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) [ 214.597836] CFI failure at kobj_attr_show+0x19/0x30 (target: throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) [ 214.597952] CFI failure at kobj_attr_show+0x19/0x30 (target: throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) [ 214.598071] CFI failure at kobj_attr_show+0x19/0x30 (target: throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) [ 214.598177] CFI failure at kobj_attr_show+0x19/0x30 (target: throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) [ 214.598307] CFI failure at kobj_attr_show+0x19/0x30 (target: throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) [ 214.598439] CFI failure at kobj_attr_show+0x19/0x30 (target: throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) [ 214.598542] CFI failure at kobj_attr_show+0x19/0x30 (target: throttle_reason_bool_show+0x0/0x50 [i915]; expected type: 0xc527b809) With kCFI, indirect calls are validated against their expected type versus actual type and failures occur when the two types do not match. The ultimate issue is that these sysfs functions are expecting to be called via dev_attr_show() but they may also be called via kobj_attr_show(), as certain files are created under two different kobjects that have two different sysfs_ops in intel_gt_sysfs_register(), hence the warnings above. When accessing the gt_ files under /sys/devices/pci:00/:00:02.0/drm/card0, which are using the same sysfs functions, there are no violations, meaning the functions are being called with the proper type. To make everything work properly, adjust certain functions to match the type of the ->show() and ->store() members in 'struct kobj_attribute'. Add a macro to generate functions for that can be called via both dev_attr_{show,store}() or kobj_attr_{show,store}() so that they can be called through both kobject locations without violating kCFI and adjust the attribute groups to account for this. Link: https://github.com/ClangBuiltLinux/linux/issues/1716 Reviewed-by: Andi Shyti Reviewed-by: Andrzej Hajda Reviewed-by: Kees Cook Signed-off-by: Nathan Chancellor --- v2: - Rebase on latest drm-intel-gt-next. - Pick up review tags from Andi, Andrzej, and Kees. - Share more code in INTEL_GT_SYSFS_{SHOW,STORE}() using common functions, as suggested by
Re: [Intel-gfx] [PATCH v4] overflow: Introduce overflows_type() and castable_to_type()
Hi Gwan-gyeong, On Fri, Oct 21, 2022 at 11:33:33AM +0300, Gwan-gyeong Mun wrote: > From: Kees Cook > > Implement a robust overflows_type() macro to test if a variable or > constant value would overflow another variable or type. This can be > used as a constant expression for static_assert() (which requires a > constant expression[1][2]) when used on constant values. This must be > constructed manually, since __builtin_add_overflow() does not produce > a constant expression[3]. > > Additionally adds castable_to_type(), similar to __same_type(), but for > checking if a constant value would overflow if cast to a given type. > > Add unit tests for overflows_type(), __same_type(), and castable_to_type() > to the existing KUnit "overflow" test. > > [1] https://en.cppreference.com/w/c/language/_Static_assert > [2] C11 standard (ISO/IEC 9899:2011): 6.7.10 Static assertions > [3] https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html > 6.56 Built-in Functions to Perform Arithmetic with Overflow Checking > Built-in Function: bool __builtin_add_overflow (type1 a, type2 b, > > Cc: Luc Van Oostenryck > Cc: Nathan Chancellor > Cc: Nick Desaulniers > Cc: Tom Rix > Cc: Daniel Latypov > Cc: Vitor Massaru Iha > Cc: "Gustavo A. R. Silva" > Cc: Jani Nikula > Cc: Mauro Carvalho Chehab > Cc: linux-harden...@vger.kernel.org > Cc: l...@lists.linux.dev > Co-developed-by: Gwan-gyeong Mun > Signed-off-by: Gwan-gyeong Mun > Signed-off-by: Kees Cook > diff --git a/lib/Makefile b/lib/Makefile > index 161d6a724ff7..e061aad90539 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -376,6 +376,10 @@ obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o > obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o > obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o > obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_kunit.o > +# We're expecting to do a lot of "always true" or "always false" tests. > +ifdef CONFIG_CC_IS_CLANG > +CFLAGS_overflow_kunit.o += $(call cc-disable-warning, > tautological-constant-out-of-range-compare) If you are going to wrap this in CONFIG_CC_IS_CLANG (which is good), drop the cc-disable-warning and just disable the warning directly. CFLAGS_overflow_kunit.o += -Wno-tautological-constant-out-of-range-compare All kernel supported clang versions support this warning so there is no point in checking for its existence before disabling it with cc-disable-warning. scripts/Makefile.extrawarn does this as well. > +endif > obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o > CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable) > obj-$(CONFIG_ST&ACKINIT_KUNIT_TEST) += stackinit_kunit.o Cheers, Nathan