Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[Guenter Roeck]

Hi,

On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:
> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
> check of the bounds in clamp().
> Gives better error coverage and one less expansion of the arguments.
> 
> Signed-off-by: David Laight <david.laight@aculab.com>

This patch triggers a build error when trying to build parisc:allmodconfig.
See error message and bisect log below.

I don't think there is anything wrong with the patch. The underlying
problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
tries to build the affected file even though CONFIG_DRM_I915 is not
enabled/supported on parisc.

Copying XE maintainers for feedback/advice.

Thanks,
Guenter

---
Building parisc:allmodconfig ... failed
--------------
Error log:
In file included from <command-line>:
drivers/gpu/drm/i915/display/intel_backlight.c: In function 'scale':
include/linux/compiler_types.h:542:45: error: call to '__compiletime_assert_415' declared with attribute error: clamp() low limit source_min greater than high limit source_max
  542 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
      |                                             ^
include/linux/compiler_types.h:523:25: note: in definition of macro '__compiletime_assert'
  523 |                         prefix ## suffix();                             \
      |                         ^~~~~~
include/linux/compiler_types.h:542:9: note: in expansion of macro '_compiletime_assert'
  542 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
      |         ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
   39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
      |                                     ^~~~~~~~~~~~~~~~~~
include/linux/minmax.h:188:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
  188 |         BUILD_BUG_ON_MSG(statically_true(ulo > uhi),                            \
      |         ^~~~~~~~~~~~~~~~
include/linux/minmax.h:195:9: note: in expansion of macro '__clamp_once'
  195 |         __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
      |         ^~~~~~~~~~~~
include/linux/minmax.h:206:28: note: in expansion of macro '__careful_clamp'
  206 | #define clamp(val, lo, hi) __careful_clamp(__auto_type, val, lo, hi)
      |                            ^~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_backlight.c:48:22: note: in expansion of macro 'clamp'
   48 |         source_val = clamp(source_val, source_min, source_max);
      |                      ^~~~~

---
# bad: [0907e7fb35756464aa34c35d6abb02998418164b] Add linux-next specific files for 20250117
# good: [5bc55a333a2f7316b58edc7573e8e893f7acb532] Linux 6.13-rc7
git bisect start 'HEAD' 'v6.13-rc7'
# bad: [195cedf4deacf84167c32b866ceac1cf4a16df15] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect bad 195cedf4deacf84167c32b866ceac1cf4a16df15
# bad: [e8c0711b153b0db806410d8b31ed23b590f4eab4] Merge branch 'xtensa-for-next' of git://github.com/jcmvbkbc/linux-xtensa.git
git bisect bad e8c0711b153b0db806410d8b31ed23b590f4eab4
# bad: [81d45722d699e594c66c150c8f7a0ec2e2bc9be6] Merge branch 'for-next/perf' of git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git
git bisect bad 81d45722d699e594c66c150c8f7a0ec2e2bc9be6
# bad: [7acb844a672defb15cf202a501815ec22c68c800] foo
git bisect bad 7acb844a672defb15cf202a501815ec22c68c800
# good: [fb2368075b135f174264071b851330649d55f9d0] mm/damon/core: add damos_filter->allow field
git bisect good fb2368075b135f174264071b851330649d55f9d0
# bad: [fc83c501e385753c90db7316faf9fd4158caaa96] minmax.h: remove some #defines that are only expanded once
git bisect bad fc83c501e385753c90db7316faf9fd4158caaa96
# good: [b04d305df1171448df5e87802c4d1f1022cc5784] ocfs2: use a folio in ocfs2_map_and_dirty_page()
git bisect good b04d305df1171448df5e87802c4d1f1022cc5784
# good: [7e01619507058f90ab603acec482951f3c452aaa] kthread: correct comments before kthread_queue_work()
git bisect good 7e01619507058f90ab603acec482951f3c452aaa
# good: [21b510a64c223707caa6db6176128779f0806a73] nilfs2: correct return value kernel-doc descriptions for ioctl functions
git bisect good 21b510a64c223707caa6db6176128779f0806a73
# good: [6afb87f23458f2d4e4334ee5a4efb8b0d07af68b] nilfs2: handle errors that nilfs_prepare_chunk() may return
git bisect good 6afb87f23458f2d4e4334ee5a4efb8b0d07af68b
# good: [8f6d46fed0bad163e5146fea1fdff150039235b2] minmax.h: reduce the #define expansion of min(), max() and clamp()
git bisect good 8f6d46fed0bad163e5146fea1fdff150039235b2
# bad: [7a70c678548d71e609b95dbddf2d411a02d13b54] minmax.h: move all the clamp() definitions after the min/max() ones
git bisect bad 7a70c678548d71e609b95dbddf2d411a02d13b54
# bad: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
git bisect bad 37f375aab0c585388b90d1af6968454fc2769cb9
# first bad commit: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[David Laight]

On Sat, 18 Jan 2025 08:13:06 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> Hi,
> 
> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:
> > Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
> > check of the bounds in clamp().
> > Gives better error coverage and one less expansion of the arguments.
> > 
> > Signed-off-by: David Laight <david.laight@aculab.com>  
> 
> This patch triggers a build error when trying to build parisc:allmodconfig.
> See error message and bisect log below.
> 
> I don't think there is anything wrong with the patch. The underlying
> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
> tries to build the affected file even though CONFIG_DRM_I915 is not
> enabled/supported on parisc.

This has appeared before.
Any idea which inlined copy of scale() is causing the problem.
On the face of it they all look ok.

If you can reproduce it maybe try commenting out some of the calls.

	David

> 
> Copying XE maintainers for feedback/advice.
> 
> Thanks,
> Guenter
> 
> ---
> Building parisc:allmodconfig ... failed
> --------------
> Error log:
> In file included from <command-line>:
> drivers/gpu/drm/i915/display/intel_backlight.c: In function 'scale':
> include/linux/compiler_types.h:542:45: error: call to '__compiletime_assert_415' declared with attribute error: clamp() low limit source_min greater than high limit source_max
>   542 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>       |                                             ^
> include/linux/compiler_types.h:523:25: note: in definition of macro '__compiletime_assert'
>   523 |                         prefix ## suffix();                             \
>       |                         ^~~~~~
> include/linux/compiler_types.h:542:9: note: in expansion of macro '_compiletime_assert'
>   542 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>       |         ^~~~~~~~~~~~~~~~~~~
> include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
>    39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>       |                                     ^~~~~~~~~~~~~~~~~~
> include/linux/minmax.h:188:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
>   188 |         BUILD_BUG_ON_MSG(statically_true(ulo > uhi),                            \
>       |         ^~~~~~~~~~~~~~~~
> include/linux/minmax.h:195:9: note: in expansion of macro '__clamp_once'
>   195 |         __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
>       |         ^~~~~~~~~~~~
> include/linux/minmax.h:206:28: note: in expansion of macro '__careful_clamp'
>   206 | #define clamp(val, lo, hi) __careful_clamp(__auto_type, val, lo, hi)
>       |                            ^~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/display/intel_backlight.c:48:22: note: in expansion of macro 'clamp'
>    48 |         source_val = clamp(source_val, source_min, source_max);
>       |                      ^~~~~
> 
> ---
> # bad: [0907e7fb35756464aa34c35d6abb02998418164b] Add linux-next specific files for 20250117
> # good: [5bc55a333a2f7316b58edc7573e8e893f7acb532] Linux 6.13-rc7
> git bisect start 'HEAD' 'v6.13-rc7'
> # bad: [195cedf4deacf84167c32b866ceac1cf4a16df15] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
> git bisect bad 195cedf4deacf84167c32b866ceac1cf4a16df15
> # bad: [e8c0711b153b0db806410d8b31ed23b590f4eab4] Merge branch 'xtensa-for-next' of git://github.com/jcmvbkbc/linux-xtensa.git
> git bisect bad e8c0711b153b0db806410d8b31ed23b590f4eab4
> # bad: [81d45722d699e594c66c150c8f7a0ec2e2bc9be6] Merge branch 'for-next/perf' of git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git
> git bisect bad 81d45722d699e594c66c150c8f7a0ec2e2bc9be6
> # bad: [7acb844a672defb15cf202a501815ec22c68c800] foo
> git bisect bad 7acb844a672defb15cf202a501815ec22c68c800
> # good: [fb2368075b135f174264071b851330649d55f9d0] mm/damon/core: add damos_filter->allow field
> git bisect good fb2368075b135f174264071b851330649d55f9d0
> # bad: [fc83c501e385753c90db7316faf9fd4158caaa96] minmax.h: remove some #defines that are only expanded once
> git bisect bad fc83c501e385753c90db7316faf9fd4158caaa96
> # good: [b04d305df1171448df5e87802c4d1f1022cc5784] ocfs2: use a folio in ocfs2_map_and_dirty_page()
> git bisect good b04d305df1171448df5e87802c4d1f1022cc5784
> # good: [7e01619507058f90ab603acec482951f3c452aaa] kthread: correct comments before kthread_queue_work()
> git bisect good 7e01619507058f90ab603acec482951f3c452aaa
> # good: [21b510a64c223707caa6db6176128779f0806a73] nilfs2: correct return value kernel-doc descriptions for ioctl functions
> git bisect good 21b510a64c223707caa6db6176128779f0806a73
> # good: [6afb87f23458f2d4e4334ee5a4efb8b0d07af68b] nilfs2: handle errors that nilfs_prepare_chunk() may return
> git bisect good 6afb87f23458f2d4e4334ee5a4efb8b0d07af68b
> # good: [8f6d46fed0bad163e5146fea1fdff150039235b2] minmax.h: reduce the #define expansion of min(), max() and clamp()
> git bisect good 8f6d46fed0bad163e5146fea1fdff150039235b2
> # bad: [7a70c678548d71e609b95dbddf2d411a02d13b54] minmax.h: move all the clamp() definitions after the min/max() ones
> git bisect bad 7a70c678548d71e609b95dbddf2d411a02d13b54
> # bad: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
> git bisect bad 37f375aab0c585388b90d1af6968454fc2769cb9
> # first bad commit: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
> 
> 

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[Guenter Roeck]

On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote:
> On Sat, 18 Jan 2025 08:13:06 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > Hi,
> > 
> > On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:
> > > Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
> > > check of the bounds in clamp().
> > > Gives better error coverage and one less expansion of the arguments.
> > > 
> > > Signed-off-by: David Laight <david.laight@aculab.com>  
> > 
> > This patch triggers a build error when trying to build parisc:allmodconfig.
> > See error message and bisect log below.
> > 
> > I don't think there is anything wrong with the patch. The underlying
> > problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
> > tries to build the affected file even though CONFIG_DRM_I915 is not
> > enabled/supported on parisc.
> 
> This has appeared before.
> Any idea which inlined copy of scale() is causing the problem.
> On the face of it they all look ok.
> 
> If you can reproduce it maybe try commenting out some of the calls.
> 

See diff below. All three changes are needed.
No idea why the compiler would know that the values are invalid.

Guenter

---
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
index fc1e517e074a..3b2c8bdfcf8d 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -76,10 +76,14 @@ static u32 clamp_user_to_hw(struct intel_connector *connector,
 static u32 scale_hw_to_user(struct intel_connector *connector,
 			    u32 hw_level, u32 user_max)
 {
+#if 0
 	struct intel_panel *panel = &connector->panel;
 
 	return scale(hw_level, panel->backlight.min, panel->backlight.max,
 		     0, user_max);
+#else
+	return 0;
+#endif
 }
 
 u32 intel_backlight_invert_pwm_level(struct intel_connector *connector, u32 val)
@@ -119,8 +123,10 @@ u32 intel_backlight_level_to_pwm(struct intel_connector *connector, u32 val)
 	drm_WARN_ON_ONCE(&i915->drm,
 			 panel->backlight.max == 0 || panel->backlight.pwm_level_max == 0);
 
+#if 0
 	val = scale(val, panel->backlight.min, panel->backlight.max,
 		    panel->backlight.pwm_level_min, panel->backlight.pwm_level_max);
+#endif
 
 	return intel_backlight_invert_pwm_level(connector, val);
 }
@@ -138,8 +144,12 @@ u32 intel_backlight_level_from_pwm(struct intel_connector *connector, u32 val)
 	     intel_has_quirk(display, QUIRK_INVERT_BRIGHTNESS)))
 		val = panel->backlight.pwm_level_max - (val - panel->backlight.pwm_level_min);
 
+#if 0
 	return scale(val, panel->backlight.pwm_level_min, panel->backlight.pwm_level_max,
 		     panel->backlight.min, panel->backlight.max);
+#else
+	return 0;
+#endif
 }
 
 static u32 lpt_get_backlight(struct intel_connector *connector, enum pipe unused)

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[David Laight]

On Sat, 18 Jan 2025 09:49:21 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote:
> > On Sat, 18 Jan 2025 08:13:06 -0800
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >   
> > > Hi,
> > > 
> > > On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:  
> > > > Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
> > > > check of the bounds in clamp().
> > > > Gives better error coverage and one less expansion of the arguments.
> > > > 
> > > > Signed-off-by: David Laight <david.laight@aculab.com>    
> > > 
> > > This patch triggers a build error when trying to build parisc:allmodconfig.
> > > See error message and bisect log below.
> > > 
> > > I don't think there is anything wrong with the patch. The underlying
> > > problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
> > > tries to build the affected file even though CONFIG_DRM_I915 is not
> > > enabled/supported on parisc.  
> > 
> > This has appeared before.
> > Any idea which inlined copy of scale() is causing the problem.
> > On the face of it they all look ok.
> > 
> > If you can reproduce it maybe try commenting out some of the calls.
> >   
> 
> See diff below. All three changes are needed.
> No idea why the compiler would know that the values are invalid.

Maybe it isn't even an inlining issue.
Perhaps that compiler just doesn't like the function ?
What happens without the 'static' (and an extra prototype)?

	David

> 
> Guenter
> 
> ---
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index fc1e517e074a..3b2c8bdfcf8d 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -76,10 +76,14 @@ static u32 clamp_user_to_hw(struct intel_connector *connector,
>  static u32 scale_hw_to_user(struct intel_connector *connector,
>  			    u32 hw_level, u32 user_max)
>  {
> +#if 0
>  	struct intel_panel *panel = &connector->panel;
>  
>  	return scale(hw_level, panel->backlight.min, panel->backlight.max,
>  		     0, user_max);
> +#else
> +	return 0;
> +#endif
>  }
>  
>  u32 intel_backlight_invert_pwm_level(struct intel_connector *connector, u32 val)
> @@ -119,8 +123,10 @@ u32 intel_backlight_level_to_pwm(struct intel_connector *connector, u32 val)
>  	drm_WARN_ON_ONCE(&i915->drm,
>  			 panel->backlight.max == 0 || panel->backlight.pwm_level_max == 0);
>  
> +#if 0
>  	val = scale(val, panel->backlight.min, panel->backlight.max,
>  		    panel->backlight.pwm_level_min, panel->backlight.pwm_level_max);
> +#endif
>  
>  	return intel_backlight_invert_pwm_level(connector, val);
>  }
> @@ -138,8 +144,12 @@ u32 intel_backlight_level_from_pwm(struct intel_connector *connector, u32 val)
>  	     intel_has_quirk(display, QUIRK_INVERT_BRIGHTNESS)))
>  		val = panel->backlight.pwm_level_max - (val - panel->backlight.pwm_level_min);
>  
> +#if 0
>  	return scale(val, panel->backlight.pwm_level_min, panel->backlight.pwm_level_max,
>  		     panel->backlight.min, panel->backlight.max);
> +#else
> +	return 0;
> +#endif
>  }
>  
>  static u32 lpt_get_backlight(struct intel_connector *connector, enum pipe unused)

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/18/25 10:09, David Laight wrote:
> On Sat, 18 Jan 2025 09:49:21 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote:
>>> On Sat, 18 Jan 2025 08:13:06 -0800
>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>    
>>>> Hi,
>>>>
>>>> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:
>>>>> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
>>>>> check of the bounds in clamp().
>>>>> Gives better error coverage and one less expansion of the arguments.
>>>>>
>>>>> Signed-off-by: David Laight <david.laight@aculab.com>
>>>>
>>>> This patch triggers a build error when trying to build parisc:allmodconfig.
>>>> See error message and bisect log below.
>>>>
>>>> I don't think there is anything wrong with the patch. The underlying
>>>> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
>>>> tries to build the affected file even though CONFIG_DRM_I915 is not
>>>> enabled/supported on parisc.
>>>
>>> This has appeared before.
>>> Any idea which inlined copy of scale() is causing the problem.
>>> On the face of it they all look ok.
>>>
>>> If you can reproduce it maybe try commenting out some of the calls.
>>>    
>>
>> See diff below. All three changes are needed.
>> No idea why the compiler would know that the values are invalid.
> 
> Maybe it isn't even an inlining issue.
> Perhaps that compiler just doesn't like the function ?
> What happens without the 'static' (and an extra prototype)?
> 


You mean like that ?

-static u32 scale(u32 source_val,
+
+u32 scale(u32 source_val,
+                u32 source_min, u32 source_max,
+                u32 target_min, u32 target_max);
+
+u32 scale(u32 source_val,
                  u32 source_min, u32 source_max,
                  u32 target_min, u32 target_max)

It doesn't help. Worse, after that change the error is still reported
even with the #if 0 elsewhere.

Guenter

Re: Buiild error in i915/xe

[David Laight]

On Sat, 18 Jan 2025 10:36:11 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On 1/18/25 10:09, David Laight wrote:
> > On Sat, 18 Jan 2025 09:49:21 -0800
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >   
> >> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote:  
> >>> On Sat, 18 Jan 2025 08:13:06 -0800
> >>> Guenter Roeck <linux@roeck-us.net> wrote:
> >>>      
> >>>> Hi,
> >>>>
> >>>> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:  
> >>>>> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
> >>>>> check of the bounds in clamp().
> >>>>> Gives better error coverage and one less expansion of the arguments.
> >>>>>
> >>>>> Signed-off-by: David Laight <david.laight@aculab.com>  
> >>>>
> >>>> This patch triggers a build error when trying to build parisc:allmodconfig.
> >>>> See error message and bisect log below.
> >>>>
> >>>> I don't think there is anything wrong with the patch. The underlying
> >>>> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
> >>>> tries to build the affected file even though CONFIG_DRM_I915 is not
> >>>> enabled/supported on parisc.  
> >>>
> >>> This has appeared before.
> >>> Any idea which inlined copy of scale() is causing the problem.
> >>> On the face of it they all look ok.
> >>>
> >>> If you can reproduce it maybe try commenting out some of the calls.
> >>>      
> >>
> >> See diff below. All three changes are needed.
> >> No idea why the compiler would know that the values are invalid.  
> > 
> > Maybe it isn't even an inlining issue.
> > Perhaps that compiler just doesn't like the function ?
> > What happens without the 'static' (and an extra prototype)?
> >   
> 
> 
> You mean like that ?
> 
> -static u32 scale(u32 source_val,
> +
> +u32 scale(u32 source_val,
> +                u32 source_min, u32 source_max,
> +                u32 target_min, u32 target_max);
> +
> +u32 scale(u32 source_val,
>                   u32 source_min, u32 source_max,
>                   u32 target_min, u32 target_max)
> 
> It doesn't help. Worse, after that change the error is still reported
> even with the #if 0 elsewhere.

Yes - that means the compiler is 'objecting' to the scale() function itself.
(Without any regard for its callers.)
Which should make it easy to reproduce outside the kernel build.

I think Mat had a successful build with a different (older?) version of gcc for
parisc.

There must be something odd causing the problem - there will be other clamp()
calls in the build that don't generate the error.

Remember that lack of the error messages requires the compiler optimise away
some code - so if the optimisation is skipped the call could be generated
and the warning output (even if the call is optimised away later).

Perhaps there is some obscure interaction with the WARN() statements?

I don't have the required compiler (neither does godbolt).

	David 

> 
> Guenter
> 

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/18/25 13:18, David Laight wrote:
> On Sat, 18 Jan 2025 10:36:11 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 1/18/25 10:09, David Laight wrote:
>>> On Sat, 18 Jan 2025 09:49:21 -0800
>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>    
>>>> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote:
>>>>> On Sat, 18 Jan 2025 08:13:06 -0800
>>>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>       
>>>>>> Hi,
>>>>>>
>>>>>> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:
>>>>>>> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
>>>>>>> check of the bounds in clamp().
>>>>>>> Gives better error coverage and one less expansion of the arguments.
>>>>>>>
>>>>>>> Signed-off-by: David Laight <david.laight@aculab.com>
>>>>>>
>>>>>> This patch triggers a build error when trying to build parisc:allmodconfig.
>>>>>> See error message and bisect log below.
>>>>>>
>>>>>> I don't think there is anything wrong with the patch. The underlying
>>>>>> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
>>>>>> tries to build the affected file even though CONFIG_DRM_I915 is not
>>>>>> enabled/supported on parisc.
>>>>>
>>>>> This has appeared before.
>>>>> Any idea which inlined copy of scale() is causing the problem.
>>>>> On the face of it they all look ok.
>>>>>
>>>>> If you can reproduce it maybe try commenting out some of the calls.
>>>>>       
>>>>
>>>> See diff below. All three changes are needed.
>>>> No idea why the compiler would know that the values are invalid.
>>>
>>> Maybe it isn't even an inlining issue.
>>> Perhaps that compiler just doesn't like the function ?
>>> What happens without the 'static' (and an extra prototype)?
>>>    
>>
>>
>> You mean like that ?
>>
>> -static u32 scale(u32 source_val,
>> +
>> +u32 scale(u32 source_val,
>> +                u32 source_min, u32 source_max,
>> +                u32 target_min, u32 target_max);
>> +
>> +u32 scale(u32 source_val,
>>                    u32 source_min, u32 source_max,
>>                    u32 target_min, u32 target_max)
>>
>> It doesn't help. Worse, after that change the error is still reported
>> even with the #if 0 elsewhere.
> 
> Yes - that means the compiler is 'objecting' to the scale() function itself.
> (Without any regard for its callers.)
> Which should make it easy to reproduce outside the kernel build.
> 
> I think Mat had a successful build with a different (older?) version of gcc for
> parisc.
> 
> There must be something odd causing the problem - there will be other clamp()
> calls in the build that don't generate the error.
> 
> Remember that lack of the error messages requires the compiler optimise away
> some code - so if the optimisation is skipped the call could be generated
> and the warning output (even if the call is optimised away later).
> 
> Perhaps there is some obscure interaction with the WARN() statements?
> 
> I don't have the required compiler (neither does godbolt).
> 

Oh man - that was a good hint. Turns out I can only reproduce the problem with
gcc 13.2 and 13.3. gcc 10.3, 11.4, and 12.4 do not generate the error.

And, yes, I can "fix" the problem with

-       WARN_ON(source_min > source_max);
+       // WARN_ON(source_min > source_max);

Any idea what to do ? Should I just scrap builds with gcc 13.x ?

Thanks,
Guenter

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[Linus Torvalds]

On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
>
> No idea why the compiler would know that the values are invalid.

It's not that the compiler knows tat they are invalid, but I bet what
happens is in scale() (and possibly other places that do similar
checks), which does this:

        WARN_ON(source_min > source_max);
        ...
        source_val = clamp(source_val, source_min, source_max);

and the compiler notices that the ordering comparison in the first
WARN_ON() is the same as the one in clamp(), so it basically converts
the logic to

        if (source_min > source_max) {
                WARN(..);
                /* Do the clamp() knowing that source_min > source_max */
                source_val = clamp(source_val, source_min, source_max);
        } else {
                /* Do the clamp knowing that source_min <= source_max */
                source_val = clamp(source_val, source_min, source_max);
        }

(obviously I dropped the other WARN_ON in the conversion, it wasn't
relevant for this case).

And now that first clamp() case is done with source_min > source_max,
and it triggers that build error because that's invalid.

So the condition is not statically true in the *source* code, but in
the "I have moved code around to combine tests" case it now *is*
statically true as far as the compiler is concerned.

              Linus

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/18/25 13:21, Linus Torvalds wrote:
> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> No idea why the compiler would know that the values are invalid.
> 
> It's not that the compiler knows tat they are invalid, but I bet what
> happens is in scale() (and possibly other places that do similar
> checks), which does this:
> 
>          WARN_ON(source_min > source_max);
>          ...
>          source_val = clamp(source_val, source_min, source_max);
> 
> and the compiler notices that the ordering comparison in the first
> WARN_ON() is the same as the one in clamp(), so it basically converts
> the logic to
> 
>          if (source_min > source_max) {
>                  WARN(..);
>                  /* Do the clamp() knowing that source_min > source_max */
>                  source_val = clamp(source_val, source_min, source_max);
>          } else {
>                  /* Do the clamp knowing that source_min <= source_max */
>                  source_val = clamp(source_val, source_min, source_max);
>          }
> 
> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> relevant for this case).
> 
> And now that first clamp() case is done with source_min > source_max,
> and it triggers that build error because that's invalid.
> 
> So the condition is not statically true in the *source* code, but in
> the "I have moved code around to combine tests" case it now *is*
> statically true as far as the compiler is concerned.
> 

Yes, turns out I can reproduce the problem by adding WARN_ON() ahead
of similar clamp() calls (see below). However, I can only reproduce it
with gcc 13.3 for parisc. I don't see the problem with other cross compilers
(I tried arm, powerpc, and loongarch64). Compilers are weird :-(.

I am not sure what to do here. That kind of problem seems difficult
to avoid, and I am sure we will hit it again elsewhere. Should I declare
gcc 13.x off limits for parisc builds ?

Guenter

---
diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
index 505c562a5daa..71c0da31a9d2 100644
--- a/drivers/input/mousedev.c
+++ b/drivers/input/mousedev.c
@@ -179,6 +179,7 @@ static void mousedev_abs_event(struct input_dev *dev, struct mousedev *mousedev,
                 if (size == 0)
                         size = xres ? : 1;

+               WARN_ON(min > max);
                 value = clamp(value, min, max);

                 mousedev->packet.x = ((value - min) * xres) / size;

Re: Buiild error in i915/xe

[Linus Torvalds]

On Sat, 18 Jan 2025 at 13:59, Guenter Roeck <linux@roeck-us.net> wrote:
>
> I am not sure what to do here. That kind of problem seems difficult
> to avoid, and I am sure we will hit it again elsewhere. Should I declare
> gcc 13.x off limits for parisc builds ?

No, I'm sure it can happen on other architectures too.

I think the only thing that makes parisc trigger this is that its
WARN_ON() is slightly different from others, and uses the
"__ret_warn_on" a few more times, and that just happens to make the
compiler decide to simplify all those tests.

Or something like that.

           Linus

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[David Laight]

On Sat, 18 Jan 2025 13:21:39 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > No idea why the compiler would know that the values are invalid.  
> 
> It's not that the compiler knows tat they are invalid, but I bet what
> happens is in scale() (and possibly other places that do similar
> checks), which does this:
> 
>         WARN_ON(source_min > source_max);
>         ...
>         source_val = clamp(source_val, source_min, source_max);
> 
> and the compiler notices that the ordering comparison in the first
> WARN_ON() is the same as the one in clamp(), so it basically converts
> the logic to
> 
>         if (source_min > source_max) {
>                 WARN(..);
>                 /* Do the clamp() knowing that source_min > source_max */
>                 source_val = clamp(source_val, source_min, source_max);
>         } else {
>                 /* Do the clamp knowing that source_min <= source_max */
>                 source_val = clamp(source_val, source_min, source_max);
>         }
> 
> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> relevant for this case).
> 
> And now that first clamp() case is done with source_min > source_max,
> and it triggers that build error because that's invalid.
> 
> So the condition is not statically true in the *source* code, but in
> the "I have moved code around to combine tests" case it now *is*
> statically true as far as the compiler is concerned.

Well spotted :-)

One option would be to move the WARN_ON() below the clamp() and
add an OPTIMISER_HIDE_VAR(source_max) between them.

Or do something more sensible than the WARN().
Perhaps return target_min on any such errors?

	David

> 
>               Linus
> 

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/18/25 14:11, David Laight wrote:
> On Sat, 18 Jan 2025 13:21:39 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> No idea why the compiler would know that the values are invalid.
>>
>> It's not that the compiler knows tat they are invalid, but I bet what
>> happens is in scale() (and possibly other places that do similar
>> checks), which does this:
>>
>>          WARN_ON(source_min > source_max);
>>          ...
>>          source_val = clamp(source_val, source_min, source_max);
>>
>> and the compiler notices that the ordering comparison in the first
>> WARN_ON() is the same as the one in clamp(), so it basically converts
>> the logic to
>>
>>          if (source_min > source_max) {
>>                  WARN(..);
>>                  /* Do the clamp() knowing that source_min > source_max */
>>                  source_val = clamp(source_val, source_min, source_max);
>>          } else {
>>                  /* Do the clamp knowing that source_min <= source_max */
>>                  source_val = clamp(source_val, source_min, source_max);
>>          }
>>
>> (obviously I dropped the other WARN_ON in the conversion, it wasn't
>> relevant for this case).
>>
>> And now that first clamp() case is done with source_min > source_max,
>> and it triggers that build error because that's invalid.
>>
>> So the condition is not statically true in the *source* code, but in
>> the "I have moved code around to combine tests" case it now *is*
>> statically true as far as the compiler is concerned.
> 
> Well spotted :-)
> 
> One option would be to move the WARN_ON() below the clamp() and
> add an OPTIMISER_HIDE_VAR(source_max) between them.
> 
> Or do something more sensible than the WARN().
> Perhaps return target_min on any such errors?
> 

This helps:

-       WARN_ON(source_min > source_max);
-       WARN_ON(target_min > target_max);
-
         /* defensive */
         source_val = clamp(source_val, source_min, source_max);

+       WARN_ON(source_min > source_max);
+       WARN_ON(target_min > target_max);

Guenter

Re: Buiild error in i915/xe

[David Laight]

On Sat, 18 Jan 2025 14:58:48 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On 1/18/25 14:11, David Laight wrote:
> > On Sat, 18 Jan 2025 13:21:39 -0800
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >   
> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:  
> >>>
> >>> No idea why the compiler would know that the values are invalid.  
> >>
> >> It's not that the compiler knows tat they are invalid, but I bet what
> >> happens is in scale() (and possibly other places that do similar
> >> checks), which does this:
> >>
> >>          WARN_ON(source_min > source_max);
> >>          ...
> >>          source_val = clamp(source_val, source_min, source_max);
> >>
> >> and the compiler notices that the ordering comparison in the first
> >> WARN_ON() is the same as the one in clamp(), so it basically converts
> >> the logic to
> >>
> >>          if (source_min > source_max) {
> >>                  WARN(..);
> >>                  /* Do the clamp() knowing that source_min > source_max */
> >>                  source_val = clamp(source_val, source_min, source_max);
> >>          } else {
> >>                  /* Do the clamp knowing that source_min <= source_max */
> >>                  source_val = clamp(source_val, source_min, source_max);
> >>          }
> >>
> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> >> relevant for this case).
> >>
> >> And now that first clamp() case is done with source_min > source_max,
> >> and it triggers that build error because that's invalid.
> >>
> >> So the condition is not statically true in the *source* code, but in
> >> the "I have moved code around to combine tests" case it now *is*
> >> statically true as far as the compiler is concerned.  
> > 
> > Well spotted :-)
> > 
> > One option would be to move the WARN_ON() below the clamp() and
> > add an OPTIMISER_HIDE_VAR(source_max) between them.
> > 
> > Or do something more sensible than the WARN().
> > Perhaps return target_min on any such errors?
> >   
> 
> This helps:
> 
> -       WARN_ON(source_min > source_max);
> -       WARN_ON(target_min > target_max);
> -
>          /* defensive */
>          source_val = clamp(source_val, source_min, source_max);
> 
> +       WARN_ON(source_min > source_max);
> +       WARN_ON(target_min > target_max);

That is a 'quick fix' ...

Much better would be to replace the WARN() with (say):
	if (target_min >= target_max)
		return target_min;
	if (source_min >= source_max)
		return target_min + (target_max - target_min)/2;
So that the return values are actually in range (in as much as one is defined).
Note that the >= cpmparisons also remove a divide by zero.

	David

> 
> Guenter
> 

Re: Buiild error in i915/xe

[Jani Nikula]

On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
> On Sat, 18 Jan 2025 14:58:48 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
>
>> On 1/18/25 14:11, David Laight wrote:
>> > On Sat, 18 Jan 2025 13:21:39 -0800
>> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> >   
>> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:  
>> >>>
>> >>> No idea why the compiler would know that the values are invalid.  
>> >>
>> >> It's not that the compiler knows tat they are invalid, but I bet what
>> >> happens is in scale() (and possibly other places that do similar
>> >> checks), which does this:
>> >>
>> >>          WARN_ON(source_min > source_max);
>> >>          ...
>> >>          source_val = clamp(source_val, source_min, source_max);
>> >>
>> >> and the compiler notices that the ordering comparison in the first
>> >> WARN_ON() is the same as the one in clamp(), so it basically converts
>> >> the logic to
>> >>
>> >>          if (source_min > source_max) {
>> >>                  WARN(..);
>> >>                  /* Do the clamp() knowing that source_min > source_max */
>> >>                  source_val = clamp(source_val, source_min, source_max);
>> >>          } else {
>> >>                  /* Do the clamp knowing that source_min <= source_max */
>> >>                  source_val = clamp(source_val, source_min, source_max);
>> >>          }
>> >>
>> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't
>> >> relevant for this case).
>> >>
>> >> And now that first clamp() case is done with source_min > source_max,
>> >> and it triggers that build error because that's invalid.
>> >>
>> >> So the condition is not statically true in the *source* code, but in
>> >> the "I have moved code around to combine tests" case it now *is*
>> >> statically true as far as the compiler is concerned.  
>> > 
>> > Well spotted :-)
>> > 
>> > One option would be to move the WARN_ON() below the clamp() and
>> > add an OPTIMISER_HIDE_VAR(source_max) between them.
>> > 
>> > Or do something more sensible than the WARN().
>> > Perhaps return target_min on any such errors?
>> >   
>> 
>> This helps:
>> 
>> -       WARN_ON(source_min > source_max);
>> -       WARN_ON(target_min > target_max);
>> -
>>          /* defensive */
>>          source_val = clamp(source_val, source_min, source_max);
>> 
>> +       WARN_ON(source_min > source_max);
>> +       WARN_ON(target_min > target_max);
>
> That is a 'quick fix' ...
>
> Much better would be to replace the WARN() with (say):
> 	if (target_min >= target_max)
> 		return target_min;
> 	if (source_min >= source_max)
> 		return target_min + (target_max - target_min)/2;
> So that the return values are actually in range (in as much as one is defined).
> Note that the >= cpmparisons also remove a divide by zero.

I want the loud and early warnings for clear bugs instead of
"gracefully" silencing the errors only to be found through debugging
user reports.

BR,
Jani.



-- 
Jani Nikula, Intel

Re: Buiild error in i915/xe

[David Laight]

On Mon, 20 Jan 2025 12:48:11 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
> > On Sat, 18 Jan 2025 14:58:48 -0800
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >  
> >> On 1/18/25 14:11, David Laight wrote:  
> >> > On Sat, 18 Jan 2025 13:21:39 -0800
> >> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >> >     
> >> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:    
> >> >>>
> >> >>> No idea why the compiler would know that the values are invalid.    
> >> >>
> >> >> It's not that the compiler knows tat they are invalid, but I bet what
> >> >> happens is in scale() (and possibly other places that do similar
> >> >> checks), which does this:
> >> >>
> >> >>          WARN_ON(source_min > source_max);
> >> >>          ...
> >> >>          source_val = clamp(source_val, source_min, source_max);
> >> >>
> >> >> and the compiler notices that the ordering comparison in the first
> >> >> WARN_ON() is the same as the one in clamp(), so it basically converts
> >> >> the logic to
> >> >>
> >> >>          if (source_min > source_max) {
> >> >>                  WARN(..);
> >> >>                  /* Do the clamp() knowing that source_min > source_max */
> >> >>                  source_val = clamp(source_val, source_min, source_max);
> >> >>          } else {
> >> >>                  /* Do the clamp knowing that source_min <= source_max */
> >> >>                  source_val = clamp(source_val, source_min, source_max);
> >> >>          }
> >> >>
> >> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> >> >> relevant for this case).
> >> >>
> >> >> And now that first clamp() case is done with source_min > source_max,
> >> >> and it triggers that build error because that's invalid.
> >> >>
> >> >> So the condition is not statically true in the *source* code, but in
> >> >> the "I have moved code around to combine tests" case it now *is*
> >> >> statically true as far as the compiler is concerned.    
> >> > 
> >> > Well spotted :-)
> >> > 
> >> > One option would be to move the WARN_ON() below the clamp() and
> >> > add an OPTIMISER_HIDE_VAR(source_max) between them.
> >> > 
> >> > Or do something more sensible than the WARN().
> >> > Perhaps return target_min on any such errors?
> >> >     
> >> 
> >> This helps:
> >> 
> >> -       WARN_ON(source_min > source_max);
> >> -       WARN_ON(target_min > target_max);
> >> -
> >>          /* defensive */
> >>          source_val = clamp(source_val, source_min, source_max);
> >> 
> >> +       WARN_ON(source_min > source_max);
> >> +       WARN_ON(target_min > target_max);  
> >
> > That is a 'quick fix' ...
> >
> > Much better would be to replace the WARN() with (say):
> > 	if (target_min >= target_max)
> > 		return target_min;
> > 	if (source_min >= source_max)
> > 		return target_min + (target_max - target_min)/2;
> > So that the return values are actually in range (in as much as one is defined).
> > Note that the >= cpmparisons also remove a divide by zero.  
> 
> I want the loud and early warnings for clear bugs instead of
> "gracefully" silencing the errors only to be found through debugging
> user reports.

A user isn't going to notice a WARN() - not until you tell them to look for it.
In any case even if you output a message you really want to return a 'sane'
value, who knows what effect a very out of range value is going to have.

	David

Re: Buiild error in i915/xe

[Jani Nikula]

On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
> On Mon, 20 Jan 2025 12:48:11 +0200
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
>> > On Sat, 18 Jan 2025 14:58:48 -0800
>> > Guenter Roeck <linux@roeck-us.net> wrote:
>> >  
>> >> On 1/18/25 14:11, David Laight wrote:  
>> >> > On Sat, 18 Jan 2025 13:21:39 -0800
>> >> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> >> >     
>> >> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:    
>> >> >>>
>> >> >>> No idea why the compiler would know that the values are invalid.    
>> >> >>
>> >> >> It's not that the compiler knows tat they are invalid, but I bet what
>> >> >> happens is in scale() (and possibly other places that do similar
>> >> >> checks), which does this:
>> >> >>
>> >> >>          WARN_ON(source_min > source_max);
>> >> >>          ...
>> >> >>          source_val = clamp(source_val, source_min, source_max);
>> >> >>
>> >> >> and the compiler notices that the ordering comparison in the first
>> >> >> WARN_ON() is the same as the one in clamp(), so it basically converts
>> >> >> the logic to
>> >> >>
>> >> >>          if (source_min > source_max) {
>> >> >>                  WARN(..);
>> >> >>                  /* Do the clamp() knowing that source_min > source_max */
>> >> >>                  source_val = clamp(source_val, source_min, source_max);
>> >> >>          } else {
>> >> >>                  /* Do the clamp knowing that source_min <= source_max */
>> >> >>                  source_val = clamp(source_val, source_min, source_max);
>> >> >>          }
>> >> >>
>> >> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't
>> >> >> relevant for this case).
>> >> >>
>> >> >> And now that first clamp() case is done with source_min > source_max,
>> >> >> and it triggers that build error because that's invalid.
>> >> >>
>> >> >> So the condition is not statically true in the *source* code, but in
>> >> >> the "I have moved code around to combine tests" case it now *is*
>> >> >> statically true as far as the compiler is concerned.    
>> >> > 
>> >> > Well spotted :-)
>> >> > 
>> >> > One option would be to move the WARN_ON() below the clamp() and
>> >> > add an OPTIMISER_HIDE_VAR(source_max) between them.
>> >> > 
>> >> > Or do something more sensible than the WARN().
>> >> > Perhaps return target_min on any such errors?
>> >> >     
>> >> 
>> >> This helps:
>> >> 
>> >> -       WARN_ON(source_min > source_max);
>> >> -       WARN_ON(target_min > target_max);
>> >> -
>> >>          /* defensive */
>> >>          source_val = clamp(source_val, source_min, source_max);
>> >> 
>> >> +       WARN_ON(source_min > source_max);
>> >> +       WARN_ON(target_min > target_max);  
>> >
>> > That is a 'quick fix' ...
>> >
>> > Much better would be to replace the WARN() with (say):
>> > 	if (target_min >= target_max)
>> > 		return target_min;
>> > 	if (source_min >= source_max)
>> > 		return target_min + (target_max - target_min)/2;
>> > So that the return values are actually in range (in as much as one is defined).
>> > Note that the >= cpmparisons also remove a divide by zero.  
>> 
>> I want the loud and early warnings for clear bugs instead of
>> "gracefully" silencing the errors only to be found through debugging
>> user reports.
>
> A user isn't going to notice a WARN() - not until you tell them to look for it.
> In any case even if you output a message you really want to return a 'sane'
> value, who knows what effect a very out of range value is going to have.

The point is, we'll catch the WARN in CI before it goes out to users.

BR,
Jani.

>
> 	David
>
>

-- 
Jani Nikula, Intel

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/20/25 03:21, Jani Nikula wrote:
> On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
>> On Mon, 20 Jan 2025 12:48:11 +0200
>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
>>>> On Sat, 18 Jan 2025 14:58:48 -0800
>>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>>   
>>>>> On 1/18/25 14:11, David Laight wrote:
>>>>>> On Sat, 18 Jan 2025 13:21:39 -0800
>>>>>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>>>>      
>>>>>>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>>>
>>>>>>>> No idea why the compiler would know that the values are invalid.
>>>>>>>
>>>>>>> It's not that the compiler knows tat they are invalid, but I bet what
>>>>>>> happens is in scale() (and possibly other places that do similar
>>>>>>> checks), which does this:
>>>>>>>
>>>>>>>           WARN_ON(source_min > source_max);
>>>>>>>           ...
>>>>>>>           source_val = clamp(source_val, source_min, source_max);
>>>>>>>
>>>>>>> and the compiler notices that the ordering comparison in the first
>>>>>>> WARN_ON() is the same as the one in clamp(), so it basically converts
>>>>>>> the logic to
>>>>>>>
>>>>>>>           if (source_min > source_max) {
>>>>>>>                   WARN(..);
>>>>>>>                   /* Do the clamp() knowing that source_min > source_max */
>>>>>>>                   source_val = clamp(source_val, source_min, source_max);
>>>>>>>           } else {
>>>>>>>                   /* Do the clamp knowing that source_min <= source_max */
>>>>>>>                   source_val = clamp(source_val, source_min, source_max);
>>>>>>>           }
>>>>>>>
>>>>>>> (obviously I dropped the other WARN_ON in the conversion, it wasn't
>>>>>>> relevant for this case).
>>>>>>>
>>>>>>> And now that first clamp() case is done with source_min > source_max,
>>>>>>> and it triggers that build error because that's invalid.
>>>>>>>
>>>>>>> So the condition is not statically true in the *source* code, but in
>>>>>>> the "I have moved code around to combine tests" case it now *is*
>>>>>>> statically true as far as the compiler is concerned.
>>>>>>
>>>>>> Well spotted :-)
>>>>>>
>>>>>> One option would be to move the WARN_ON() below the clamp() and
>>>>>> add an OPTIMISER_HIDE_VAR(source_max) between them.
>>>>>>
>>>>>> Or do something more sensible than the WARN().
>>>>>> Perhaps return target_min on any such errors?
>>>>>>      
>>>>>
>>>>> This helps:
>>>>>
>>>>> -       WARN_ON(source_min > source_max);
>>>>> -       WARN_ON(target_min > target_max);
>>>>> -
>>>>>           /* defensive */
>>>>>           source_val = clamp(source_val, source_min, source_max);
>>>>>
>>>>> +       WARN_ON(source_min > source_max);
>>>>> +       WARN_ON(target_min > target_max);
>>>>
>>>> That is a 'quick fix' ...
>>>>
>>>> Much better would be to replace the WARN() with (say):
>>>> 	if (target_min >= target_max)
>>>> 		return target_min;
>>>> 	if (source_min >= source_max)
>>>> 		return target_min + (target_max - target_min)/2;
>>>> So that the return values are actually in range (in as much as one is defined).
>>>> Note that the >= cpmparisons also remove a divide by zero.
>>>
>>> I want the loud and early warnings for clear bugs instead of
>>> "gracefully" silencing the errors only to be found through debugging
>>> user reports.
>>
>> A user isn't going to notice a WARN() - not until you tell them to look for it.
>> In any case even if you output a message you really want to return a 'sane'
>> value, who knows what effect a very out of range value is going to have.
> 
> The point is, we'll catch the WARN in CI before it goes out to users.
> 

It isn't going to catch the divide by 0 error, and it obviously doesn't
catch the build problem on parisc with gcc 13.x because the CI isn't
testing it.

How about disabling DRM_XE on architectures where it isn't supported,
matching DRM_I915 ?

Thanks,
Guenter

Re: Buiild error in i915/xe

[David Laight]

On Mon, 20 Jan 2025 06:15:30 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On 1/20/25 03:21, Jani Nikula wrote:
> > On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:  
> >> On Mon, 20 Jan 2025 12:48:11 +0200
> >> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>  
> >>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:  
> >>>> On Sat, 18 Jan 2025 14:58:48 -0800
> >>>> Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>     
> >>>>> On 1/18/25 14:11, David Laight wrote:  
> >>>>>> On Sat, 18 Jan 2025 13:21:39 -0800
> >>>>>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >>>>>>        
> >>>>>>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:  
> >>>>>>>>
> >>>>>>>> No idea why the compiler would know that the values are invalid.  
> >>>>>>>
> >>>>>>> It's not that the compiler knows tat they are invalid, but I bet what
> >>>>>>> happens is in scale() (and possibly other places that do similar
> >>>>>>> checks), which does this:
> >>>>>>>
> >>>>>>>           WARN_ON(source_min > source_max);
> >>>>>>>           ...
> >>>>>>>           source_val = clamp(source_val, source_min, source_max);
> >>>>>>>
> >>>>>>> and the compiler notices that the ordering comparison in the first
> >>>>>>> WARN_ON() is the same as the one in clamp(), so it basically converts
> >>>>>>> the logic to
> >>>>>>>
> >>>>>>>           if (source_min > source_max) {
> >>>>>>>                   WARN(..);
> >>>>>>>                   /* Do the clamp() knowing that source_min > source_max */
> >>>>>>>                   source_val = clamp(source_val, source_min, source_max);
> >>>>>>>           } else {
> >>>>>>>                   /* Do the clamp knowing that source_min <= source_max */
> >>>>>>>                   source_val = clamp(source_val, source_min, source_max);
> >>>>>>>           }
> >>>>>>>
> >>>>>>> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> >>>>>>> relevant for this case).
> >>>>>>>
> >>>>>>> And now that first clamp() case is done with source_min > source_max,
> >>>>>>> and it triggers that build error because that's invalid.
> >>>>>>>
> >>>>>>> So the condition is not statically true in the *source* code, but in
> >>>>>>> the "I have moved code around to combine tests" case it now *is*
> >>>>>>> statically true as far as the compiler is concerned.  
> >>>>>>
> >>>>>> Well spotted :-)
> >>>>>>
> >>>>>> One option would be to move the WARN_ON() below the clamp() and
> >>>>>> add an OPTIMISER_HIDE_VAR(source_max) between them.
> >>>>>>
> >>>>>> Or do something more sensible than the WARN().
> >>>>>> Perhaps return target_min on any such errors?
> >>>>>>        
> >>>>>
> >>>>> This helps:
> >>>>>
> >>>>> -       WARN_ON(source_min > source_max);
> >>>>> -       WARN_ON(target_min > target_max);
> >>>>> -
> >>>>>           /* defensive */
> >>>>>           source_val = clamp(source_val, source_min, source_max);
> >>>>>
> >>>>> +       WARN_ON(source_min > source_max);
> >>>>> +       WARN_ON(target_min > target_max);  
> >>>>
> >>>> That is a 'quick fix' ...
> >>>>
> >>>> Much better would be to replace the WARN() with (say):
> >>>> 	if (target_min >= target_max)
> >>>> 		return target_min;
> >>>> 	if (source_min >= source_max)
> >>>> 		return target_min + (target_max - target_min)/2;
> >>>> So that the return values are actually in range (in as much as one is defined).
> >>>> Note that the >= cpmparisons also remove a divide by zero.  
> >>>
> >>> I want the loud and early warnings for clear bugs instead of
> >>> "gracefully" silencing the errors only to be found through debugging
> >>> user reports.  
> >>
> >> A user isn't going to notice a WARN() - not until you tell them to look for it.
> >> In any case even if you output a message you really want to return a 'sane'
> >> value, who knows what effect a very out of range value is going to have.  
> > 
> > The point is, we'll catch the WARN in CI before it goes out to users.
> >   
> 
> It isn't going to catch the divide by 0 error, and it obviously doesn't
> catch the build problem on parisc with gcc 13.x because the CI isn't
> testing it.
> 
> How about disabling DRM_XE on architectures where it isn't supported,
> matching DRM_I915 ?

That'll just bite back later.
As Linus spotted the compiler is just 'optimising' some code paths.
It could happen for any architecture including x64.
The repeated tests are basically slightly odd, although you might only
expect them to be present in debug builds.

An alternative would be to replace the clamp() with:
	if (source_val <= source_min)
		return target_min;
	if (source_val >= source_max)
		return target_max;

	David

> 
> Thanks,
> Guenter
> 

Re: Buiild error in i915/xe

[Andy Shevchenko]

On Mon, Jan 20, 2025 at 06:41:43PM +0000, David Laight wrote:
> On Mon, 20 Jan 2025 06:15:30 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> > On 1/20/25 03:21, Jani Nikula wrote:
> > > On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:  
> > >> On Mon, 20 Jan 2025 12:48:11 +0200
> > >> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > >>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:  
> > >>>> On Sat, 18 Jan 2025 14:58:48 -0800
> > >>>> Guenter Roeck <linux@roeck-us.net> wrote:
> > >>>>> On 1/18/25 14:11, David Laight wrote:  
> > >>>>>> On Sat, 18 Jan 2025 13:21:39 -0800
> > >>>>>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > >>>>>>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:  
> > >>>>>>>>
> > >>>>>>>> No idea why the compiler would know that the values are invalid.  
> > >>>>>>>
> > >>>>>>> It's not that the compiler knows tat they are invalid, but I bet what
> > >>>>>>> happens is in scale() (and possibly other places that do similar
> > >>>>>>> checks), which does this:
> > >>>>>>>
> > >>>>>>>           WARN_ON(source_min > source_max);
> > >>>>>>>           ...
> > >>>>>>>           source_val = clamp(source_val, source_min, source_max);
> > >>>>>>>
> > >>>>>>> and the compiler notices that the ordering comparison in the first
> > >>>>>>> WARN_ON() is the same as the one in clamp(), so it basically converts
> > >>>>>>> the logic to
> > >>>>>>>
> > >>>>>>>           if (source_min > source_max) {
> > >>>>>>>                   WARN(..);
> > >>>>>>>                   /* Do the clamp() knowing that source_min > source_max */
> > >>>>>>>                   source_val = clamp(source_val, source_min, source_max);
> > >>>>>>>           } else {
> > >>>>>>>                   /* Do the clamp knowing that source_min <= source_max */
> > >>>>>>>                   source_val = clamp(source_val, source_min, source_max);
> > >>>>>>>           }
> > >>>>>>>
> > >>>>>>> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> > >>>>>>> relevant for this case).
> > >>>>>>>
> > >>>>>>> And now that first clamp() case is done with source_min > source_max,
> > >>>>>>> and it triggers that build error because that's invalid.
> > >>>>>>>
> > >>>>>>> So the condition is not statically true in the *source* code, but in
> > >>>>>>> the "I have moved code around to combine tests" case it now *is*
> > >>>>>>> statically true as far as the compiler is concerned.  
> > >>>>>>
> > >>>>>> Well spotted :-)
> > >>>>>>
> > >>>>>> One option would be to move the WARN_ON() below the clamp() and
> > >>>>>> add an OPTIMISER_HIDE_VAR(source_max) between them.
> > >>>>>>
> > >>>>>> Or do something more sensible than the WARN().
> > >>>>>> Perhaps return target_min on any such errors?
> > >>>>>>        
> > >>>>>
> > >>>>> This helps:
> > >>>>>
> > >>>>> -       WARN_ON(source_min > source_max);
> > >>>>> -       WARN_ON(target_min > target_max);
> > >>>>> -
> > >>>>>           /* defensive */
> > >>>>>           source_val = clamp(source_val, source_min, source_max);
> > >>>>>
> > >>>>> +       WARN_ON(source_min > source_max);
> > >>>>> +       WARN_ON(target_min > target_max);  
> > >>>>
> > >>>> That is a 'quick fix' ...
> > >>>>
> > >>>> Much better would be to replace the WARN() with (say):
> > >>>> 	if (target_min >= target_max)
> > >>>> 		return target_min;
> > >>>> 	if (source_min >= source_max)
> > >>>> 		return target_min + (target_max - target_min)/2;
> > >>>> So that the return values are actually in range (in as much as one is defined).
> > >>>> Note that the >= cpmparisons also remove a divide by zero.  
> > >>>
> > >>> I want the loud and early warnings for clear bugs instead of
> > >>> "gracefully" silencing the errors only to be found through debugging
> > >>> user reports.  
> > >>
> > >> A user isn't going to notice a WARN() - not until you tell them to look for it.
> > >> In any case even if you output a message you really want to return a 'sane'
> > >> value, who knows what effect a very out of range value is going to have.  
> > > 
> > > The point is, we'll catch the WARN in CI before it goes out to users.
> > 
> > It isn't going to catch the divide by 0 error, and it obviously doesn't
> > catch the build problem on parisc with gcc 13.x because the CI isn't
> > testing it.
> > 
> > How about disabling DRM_XE on architectures where it isn't supported,
> > matching DRM_I915 ?
> 
> That'll just bite back later.
> As Linus spotted the compiler is just 'optimising' some code paths.
> It could happen for any architecture including x64.
> The repeated tests are basically slightly odd, although you might only
> expect them to be present in debug builds.
> 
> An alternative would be to replace the clamp() with:
> 	if (source_val <= source_min)
> 		return target_min;
> 	if (source_val >= source_max)
> 		return target_max;

Excuse me if I am missing something, but clamp() has a warning inside it, correct?
Why do wee need an additional warning on top of that?

P.S. However, I agree that ideally clamp() should work independently on the
caller to use WARN*() or other similar stuff.

-- 
With Best Regards,
Andy Shevchenko

Re: Buiild error in i915/xe

[Linus Torvalds]

On Mon, 20 Jan 2025 at 10:55, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Excuse me if I am missing something, but clamp() has a warning inside it, correct?
> Why do we need an additional warning on top of that?

Note: the warning in clamp() only finds compile-time obvious wrong uses.

It's really meant to notice the trivial case where you clam with
constants and just got the order wrong, so you do something silly like

    res = clamp(in, 15, 1);

but it does also end up catching slightly more complex things where
the compiler can figure out the range of the clamping.

The build problem then comes from the compiler doing various *other*
code movem,ent and optimization too, and - like in this case - finds
an error path where the clamping is done "wrong".

I think the real issue in the i915 driver is that it does that
WARN_ON(), but then it just happily continues anyway.

So if the i915 driver instead did

        if (WARN_ON(..)) return invalid value;

none of this would ever have happened.

                Linus

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/20/25 11:14, Linus Torvalds wrote:
> On Mon, 20 Jan 2025 at 10:55, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> Excuse me if I am missing something, but clamp() has a warning inside it, correct?
>> Why do we need an additional warning on top of that?
> 
> Note: the warning in clamp() only finds compile-time obvious wrong uses.
> 
> It's really meant to notice the trivial case where you clam with
> constants and just got the order wrong, so you do something silly like
> 
>      res = clamp(in, 15, 1);
> 
> but it does also end up catching slightly more complex things where
> the compiler can figure out the range of the clamping.
> 
> The build problem then comes from the compiler doing various *other*
> code movem,ent and optimization too, and - like in this case - finds
> an error path where the clamping is done "wrong".
> 
> I think the real issue in the i915 driver is that it does that
> WARN_ON(), but then it just happily continues anyway.
> 
> So if the i915 driver instead did
> 
>          if (WARN_ON(..)) return invalid value;
> 
> none of this would ever have happened.
> 

I'll take a stab and send a patch combining your and David's suggestions.

Guenter

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[Pedro Falcato]

On Sat, Jan 18, 2025 at 10:11 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Sat, 18 Jan 2025 13:21:39 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > No idea why the compiler would know that the values are invalid.
> >
> > It's not that the compiler knows tat they are invalid, but I bet what
> > happens is in scale() (and possibly other places that do similar
> > checks), which does this:
> >
> >         WARN_ON(source_min > source_max);
> >         ...
> >         source_val = clamp(source_val, source_min, source_max);
> >
> > and the compiler notices that the ordering comparison in the first
> > WARN_ON() is the same as the one in clamp(), so it basically converts
> > the logic to
> >
> >         if (source_min > source_max) {
> >                 WARN(..);
> >                 /* Do the clamp() knowing that source_min > source_max */
> >                 source_val = clamp(source_val, source_min, source_max);
> >         } else {
> >                 /* Do the clamp knowing that source_min <= source_max */
> >                 source_val = clamp(source_val, source_min, source_max);
> >         }
> >
> > (obviously I dropped the other WARN_ON in the conversion, it wasn't
> > relevant for this case).
> >
> > And now that first clamp() case is done with source_min > source_max,
> > and it triggers that build error because that's invalid.
> >
> > So the condition is not statically true in the *source* code, but in
> > the "I have moved code around to combine tests" case it now *is*
> > statically true as far as the compiler is concerned.
>
> Well spotted :-)
>
> One option would be to move the WARN_ON() below the clamp() and
> add an OPTIMISER_HIDE_VAR(source_max) between them.
>
> Or do something more sensible than the WARN().

Given the awful problems we've found with these macros (clamp, min,
max, etc), maybe the best option is to not play these awful games in
general?

These macros seem footgunned to hell just as a way to try to
compensate for C's lack of language features.

-- 
Pedro

✗ Fi.CI.CHECKPATCH: warning for Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[Patchwork]

== Series Details ==

Series: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())
URL   : https://patchwork.freedesktop.org/series/143703/
State : warning

== Summary ==

Error: dim checkpatch failed
9955d8377768 Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())
-:20: WARNING:COMMIT_LOG_LONG_LINE: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#20: 
> > This patch triggers a build error when trying to build parisc:allmodconfig.

-:48: WARNING:IF_0: Consider removing the code enclosed by this #if 0 and its #endif
#48: FILE: drivers/gpu/drm/i915/display/intel_backlight.c:79:
+#if 0

-:63: WARNING:IF_0: Consider removing the code enclosed by this #if 0 and its #endif
#63: FILE: drivers/gpu/drm/i915/display/intel_backlight.c:126:
+#if 0

-:74: WARNING:IF_0: Consider removing the code enclosed by this #if 0 and its #endif
#74: FILE: drivers/gpu/drm/i915/display/intel_backlight.c:147:
+#if 0

-:82: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)

total: 1 errors, 4 warnings, 0 checks, 36 lines checked

✗ i915.CI.BAT: failure for Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 9530 bytes --]

== Series Details ==

Series: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())
URL   : https://patchwork.freedesktop.org/series/143703/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_15979 -> Patchwork_143703v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_143703v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_143703v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) 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_143703v1/index.html

Participating hosts (44 -> 41)
------------------------------

  Missing    (3): bat-apl-1 fi-kbl-guc fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_143703v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@hangcheck:
    - bat-arlh-3:         NOTRUN -> [DMESG-WARN][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-arlh-3/igt@i915_selftest@live@hangcheck.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-adlp-6:         [PASS][2] -> [FAIL][3] +1 other test fail
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-adlp-6/igt@kms_pm_backlight@basic-brightness.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-adlp-6/igt@kms_pm_backlight@basic-brightness.html
    - bat-jsl-3:          [PASS][4] -> [FAIL][5] +1 other test fail
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-jsl-3/igt@kms_pm_backlight@basic-brightness.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-jsl-3/igt@kms_pm_backlight@basic-brightness.html
    - bat-twl-2:          [PASS][6] -> [FAIL][7] +1 other test fail
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-twl-2/igt@kms_pm_backlight@basic-brightness.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-twl-2/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_pm_backlight@basic-brightness@edp-1:
    - bat-jsl-1:          [PASS][8] -> [FAIL][9] +1 other test fail
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-jsl-1/igt@kms_pm_backlight@basic-brightness@edp-1.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-jsl-1/igt@kms_pm_backlight@basic-brightness@edp-1.html
    - bat-mtlp-8:         [PASS][10] -> [FAIL][11] +1 other test fail
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-mtlp-8/igt@kms_pm_backlight@basic-brightness@edp-1.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-mtlp-8/igt@kms_pm_backlight@basic-brightness@edp-1.html
    - bat-rplp-1:         [PASS][12] -> [FAIL][13] +1 other test fail
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-rplp-1/igt@kms_pm_backlight@basic-brightness@edp-1.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-rplp-1/igt@kms_pm_backlight@basic-brightness@edp-1.html
    - bat-arlh-3:         [PASS][14] -> [FAIL][15] +1 other test fail
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-arlh-3/igt@kms_pm_backlight@basic-brightness@edp-1.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-arlh-3/igt@kms_pm_backlight@basic-brightness@edp-1.html
    - bat-twl-1:          [PASS][16] -> [FAIL][17] +1 other test fail
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-twl-1/igt@kms_pm_backlight@basic-brightness@edp-1.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-twl-1/igt@kms_pm_backlight@basic-brightness@edp-1.html

  
#### Warnings ####

  * igt@i915_selftest@live:
    - bat-arlh-3:         [INCOMPLETE][18] ([i915#12061] / [i915#12445]) -> [DMESG-WARN][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-arlh-3/igt@i915_selftest@live.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-arlh-3/igt@i915_selftest@live.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@guc:
    - {bat-mtlp-9}:       [PASS][20] -> [INCOMPLETE][21] +1 other test incomplete
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-mtlp-9/igt@i915_selftest@live@guc.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-mtlp-9/igt@i915_selftest@live@guc.html

  * igt@kms_pm_backlight@basic-brightness@edp-1:
    - {bat-mtlp-9}:       [PASS][22] -> [FAIL][23] +1 other test fail
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-mtlp-9/igt@kms_pm_backlight@basic-brightness@edp-1.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-mtlp-9/igt@kms_pm_backlight@basic-brightness@edp-1.html

  
Known issues
------------

  Here are the changes found in Patchwork_143703v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@module-reload:
    - bat-dg1-7:          [PASS][24] -> [FAIL][25] ([i915#13401])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-dg1-7/igt@i915_pm_rpm@module-reload.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-dg1-7/igt@i915_pm_rpm@module-reload.html
    - bat-rpls-4:         [PASS][26] -> [FAIL][27] ([i915#13401])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-rpls-4/igt@i915_pm_rpm@module-reload.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-rpls-4/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live:
    - bat-mtlp-8:         [PASS][28] -> [DMESG-FAIL][29] ([i915#12061]) +1 other test dmesg-fail
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-mtlp-8/igt@i915_selftest@live.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-mtlp-8/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-arls-5:         [PASS][30] -> [DMESG-FAIL][31] ([i915#12061]) +1 other test dmesg-fail
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-arls-5/igt@i915_selftest@live@workarounds.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-arls-5/igt@i915_selftest@live@workarounds.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         [PASS][32] -> [SKIP][33] ([i915#9197]) +1 other test skip
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  
#### Possible fixes ####

  * igt@i915_selftest@live:
    - bat-jsl-3:          [INCOMPLETE][34] ([i915#12445] / [i915#13241]) -> [PASS][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-jsl-3/igt@i915_selftest@live.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-jsl-3/igt@i915_selftest@live.html

  * igt@i915_selftest@live@gem_contexts:
    - bat-arlh-3:         [INCOMPLETE][36] -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-arlh-3/igt@i915_selftest@live@gem_contexts.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-arlh-3/igt@i915_selftest@live@gem_contexts.html
    - bat-jsl-3:          [INCOMPLETE][38] ([i915#13241]) -> [PASS][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-jsl-3/igt@i915_selftest@live@gem_contexts.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-jsl-3/igt@i915_selftest@live@gem_contexts.html

  * igt@i915_selftest@live@workarounds:
    - bat-arlh-3:         [DMESG-FAIL][40] ([i915#12061]) -> [PASS][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15979/bat-arlh-3/igt@i915_selftest@live@workarounds.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/bat-arlh-3/igt@i915_selftest@live@workarounds.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [i915#12445]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12445
  [i915#13241]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13241
  [i915#13401]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13401
  [i915#13494]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13494
  [i915#9197]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9197


Build changes
-------------

  * Linux: CI_DRM_15979 -> Patchwork_143703v1

  CI-20190529: 20190529
  CI_DRM_15979: 4fc988de9f5e17d19edb5fc0a0fbc15a8fc837f4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8198: 1d1ae626601119d249b530547b9e226c6a684144 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_143703v1: 4fc988de9f5e17d19edb5fc0a0fbc15a8fc837f4 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143703v1/index.html

[-- Attachment #2: Type: text/html, Size: 10908 bytes --]

✗ Fi.CI.BUILD: failure for Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()) (rev2)

[Patchwork]

== Series Details ==

Series: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()) (rev2)
URL   : https://patchwork.freedesktop.org/series/143703/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/143703/revisions/2/mbox/ not applied
Applying: Buiild error in i915/xe
error: patch failed: drivers/input/mousedev.c:179
error: drivers/input/mousedev.c: patch does not apply
error: Did you hand edit your patch?
It does not apply to blobs recorded in its index.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Using index info to reconstruct a base tree...
Patch failed at 0001 Buiild error in i915/xe
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced