* [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
@ 2018-03-13 16:19 Arnd Bergmann
2018-03-13 16:38 ` Chris Wilson
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Arnd Bergmann @ 2018-03-13 16:19 UTC (permalink / raw)
To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie
Cc: Arnd Bergmann, Tvrtko Ursulin, Chris Wilson, Michal Wajdeczko,
intel-gfx, dri-devel, linux-kernel
The conditional spinlock confuses gcc into thinking the 'flags' value
might contain uninitialized data:
drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
The code is correct, but it's easy to see how the compiler gets confused
here. This avoids the problem by pulling the lock outside of the function
into its only caller.
Fixes: 1fe699e30113 ("drm/i915/pmu: Fix sleep under atomic in RC6 readout")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: removed unused function argument, fixed 'break' statement.
---
drivers/gpu/drm/i915/i915_pmu.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 4bc7aefa9541..d6b9b6b5fb98 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -412,10 +412,9 @@ static u64 __get_rc6(struct drm_i915_private *i915)
return val;
}
-static u64 get_rc6(struct drm_i915_private *i915, bool locked)
+static u64 get_rc6(struct drm_i915_private *i915)
{
#if IS_ENABLED(CONFIG_PM)
- unsigned long flags;
u64 val;
if (intel_runtime_pm_get_if_in_use(i915)) {
@@ -428,18 +427,12 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
* previously.
*/
- if (!locked)
- spin_lock_irqsave(&i915->pmu.lock, flags);
-
if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
} else {
val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
}
-
- if (!locked)
- spin_unlock_irqrestore(&i915->pmu.lock, flags);
} else {
struct pci_dev *pdev = i915->drm.pdev;
struct device *kdev = &pdev->dev;
@@ -452,9 +445,6 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
* on top of the last known real value, as the approximated RC6
* counter value.
*/
- if (!locked)
- spin_lock_irqsave(&i915->pmu.lock, flags);
-
spin_lock_irqsave(&kdev->power.lock, flags2);
if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
@@ -470,9 +460,6 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
val = jiffies_to_nsecs(val);
val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
-
- if (!locked)
- spin_unlock_irqrestore(&i915->pmu.lock, flags);
}
return val;
@@ -519,7 +506,15 @@ static u64 __i915_pmu_event_read(struct perf_event *event, bool locked)
val = count_interrupts(i915);
break;
case I915_PMU_RC6_RESIDENCY:
- val = get_rc6(i915, locked);
+ if (!locked) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&i915->pmu.lock, flags);
+ val = get_rc6(i915);
+ spin_unlock_irqrestore(&i915->pmu.lock, flags);
+ } else {
+ val = get_rc6(i915);
+ }
break;
}
}
--
2.9.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
2018-03-13 16:19 [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning Arnd Bergmann
@ 2018-03-13 16:38 ` Chris Wilson
2018-03-13 16:46 ` ✗ Fi.CI.SPARSE: warning for drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2) Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-03-13 16:38 UTC (permalink / raw)
To: Arnd Bergmann, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie
Cc: Arnd Bergmann, Tvrtko Ursulin, Michal Wajdeczko, intel-gfx,
dri-devel, linux-kernel
Quoting Arnd Bergmann (2018-03-13 16:19:31)
> The conditional spinlock confuses gcc into thinking the 'flags' value
> might contain uninitialized data:
>
> drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
> arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The code is correct, but it's easy to see how the compiler gets confused
> here. This avoids the problem by pulling the lock outside of the function
> into its only caller.
>
> Fixes: 1fe699e30113 ("drm/i915/pmu: Fix sleep under atomic in RC6 readout")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
@ 2018-03-13 16:38 ` Chris Wilson
0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-03-13 16:38 UTC (permalink / raw)
To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie
Cc: Arnd Bergmann, Tvrtko Ursulin, intel-gfx, linux-kernel,
dri-devel, Michal Wajdeczko
Quoting Arnd Bergmann (2018-03-13 16:19:31)
> The conditional spinlock confuses gcc into thinking the 'flags' value
> might contain uninitialized data:
>
> drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
> arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The code is correct, but it's easy to see how the compiler gets confused
> here. This avoids the problem by pulling the lock outside of the function
> into its only caller.
>
> Fixes: 1fe699e30113 ("drm/i915/pmu: Fix sleep under atomic in RC6 readout")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* ✗ Fi.CI.SPARSE: warning for drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2)
2018-03-13 16:19 [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning Arnd Bergmann
2018-03-13 16:38 ` Chris Wilson
@ 2018-03-13 16:46 ` Patchwork
2018-03-13 17:41 ` Tvrtko Ursulin
2018-03-13 17:02 ` ✗ Fi.CI.BAT: " Patchwork
2018-03-13 17:46 ` Tvrtko Ursulin
3 siblings, 1 reply; 13+ messages in thread
From: Patchwork @ 2018-03-13 16:46 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2)
URL : https://patchwork.freedesktop.org/series/39857/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Commit: drm/i915/pmu: avoid -Wmaybe-uninitialized warning
-O:drivers/gpu/drm/i915/i915_pmu.c:442:47: warning: context imbalance in 'get_rc6' - unexpected unlock
+
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2)
2018-03-13 16:19 [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning Arnd Bergmann
2018-03-13 16:38 ` Chris Wilson
2018-03-13 16:46 ` ✗ Fi.CI.SPARSE: warning for drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2) Patchwork
@ 2018-03-13 17:02 ` Patchwork
2018-03-13 17:46 ` Tvrtko Ursulin
3 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-03-13 17:02 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2)
URL : https://patchwork.freedesktop.org/series/39857/
State : warning
== Summary ==
Series 39857v2 drm/i915/pmu: avoid -Wmaybe-uninitialized warning
https://patchwork.freedesktop.org/api/1.0/series/39857/revisions/2/mbox/
---- Possible new issues:
Test kms_busy:
Subgroup basic-flip-a:
skip -> PASS (fi-ivb-3770)
Subgroup basic-flip-b:
skip -> PASS (fi-ivb-3770)
Subgroup basic-flip-c:
skip -> PASS (fi-ivb-3770)
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
skip -> PASS (fi-ivb-3770)
Subgroup basic-busy-flip-before-cursor-legacy:
skip -> PASS (fi-ivb-3770)
Subgroup basic-flip-after-cursor-atomic:
skip -> PASS (fi-ivb-3770)
Subgroup basic-flip-after-cursor-legacy:
skip -> PASS (fi-ivb-3770)
Subgroup basic-flip-after-cursor-varying-size:
skip -> PASS (fi-ivb-3770)
Subgroup basic-flip-before-cursor-atomic:
skip -> PASS (fi-ivb-3770)
Subgroup basic-flip-before-cursor-legacy:
skip -> PASS (fi-ivb-3770)
Subgroup basic-flip-before-cursor-varying-size:
skip -> PASS (fi-ivb-3770)
Test kms_force_connector_basic:
Subgroup prune-stale-modes:
pass -> SKIP (fi-ivb-3770)
Test kms_frontbuffer_tracking:
Subgroup basic:
skip -> PASS (fi-ivb-3770)
---- Known issues:
Test kms_chamelium:
Subgroup dp-crc-fast:
fail -> PASS (fi-kbl-7500u) fdo#103841
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
pass -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test prime_vgem:
Subgroup basic-fence-flip:
skip -> PASS (fi-ivb-3770) fdo#104008
fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:433s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:434s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:383s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:534s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:299s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:511s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:510s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:510s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:496s
fi-cfl-8700k total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:411s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:578s
fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:592s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:431s
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:313s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:533s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:402s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:425s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:474s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:430s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:473s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:466s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:518s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:644s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:438s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:526s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:543s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:507s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:502s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:426s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:437s
fi-snb-2520m total:245 pass:211 dwarn:0 dfail:0 fail:0 skip:33
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:406s
Blacklisted hosts:
fi-cfl-u total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:514s
fi-cnl-drrs total:288 pass:257 dwarn:3 dfail:0 fail:0 skip:28 time:525s
874b86a759851707e26286c22062f6ccc526e46f drm-tip: 2018y-03m-13d-12h-36m-17s UTC integration manifest
dfa0bf3ec528 drm/i915/pmu: avoid -Wmaybe-uninitialized warning
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8331/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ✗ Fi.CI.SPARSE: warning for drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2)
2018-03-13 16:46 ` ✗ Fi.CI.SPARSE: warning for drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2) Patchwork
@ 2018-03-13 17:41 ` Tvrtko Ursulin
2018-03-13 17:45 ` Chris Wilson
0 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2018-03-13 17:41 UTC (permalink / raw)
To: intel-gfx, Patchwork, Arnd Bergmann
On 13/03/2018 16:46, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2)
> URL : https://patchwork.freedesktop.org/series/39857/
> State : warning
>
> == Summary ==
>
> $ dim sparse origin/drm-tip
> Commit: drm/i915/pmu: avoid -Wmaybe-uninitialized warning
> -O:drivers/gpu/drm/i915/i915_pmu.c:442:47: warning: context imbalance in 'get_rc6' - unexpected unlock
> +
Is sparse being run before the patch has been applied?!
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ✗ Fi.CI.SPARSE: warning for drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2)
2018-03-13 17:41 ` Tvrtko Ursulin
@ 2018-03-13 17:45 ` Chris Wilson
0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-03-13 17:45 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx, Patchwork, Arnd Bergmann
Quoting Tvrtko Ursulin (2018-03-13 17:41:59)
>
> On 13/03/2018 16:46, Patchwork wrote:
> > == Series Details ==
> >
> > Series: drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2)
> > URL : https://patchwork.freedesktop.org/series/39857/
> > State : warning
> >
> > == Summary ==
> >
> > $ dim sparse origin/drm-tip
> > Commit: drm/i915/pmu: avoid -Wmaybe-uninitialized warning
> > -O:drivers/gpu/drm/i915/i915_pmu.c:442:47: warning: context imbalance in 'get_rc6' - unexpected unlock
> > +
>
> Is sparse being run before the patch has been applied?!
Sparse is definitely complaining about the apparent imbalance in
drm-tip. So the -+ does look like it means that warning was removed by
the patch.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
2018-03-13 16:19 [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning Arnd Bergmann
@ 2018-03-13 17:46 ` Tvrtko Ursulin
2018-03-13 16:46 ` ✗ Fi.CI.SPARSE: warning for drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2) Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2018-03-13 17:46 UTC (permalink / raw)
To: Arnd Bergmann, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie
Cc: intel-gfx, linux-kernel, dri-devel
On 13/03/2018 16:19, Arnd Bergmann wrote:
> The conditional spinlock confuses gcc into thinking the 'flags' value
> might contain uninitialized data:
>
> drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
> arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
Hm, how does paravirt_types.h comes into the picture?
> The code is correct, but it's easy to see how the compiler gets confused
> here. This avoids the problem by pulling the lock outside of the function
> into its only caller.
Is it specific gcc version, specific options, or specific kernel config
that this happens?
Strange that it hasn't been seen so far.
Regards,
Tvrtko
> Fixes: 1fe699e30113 ("drm/i915/pmu: Fix sleep under atomic in RC6 readout")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: removed unused function argument, fixed 'break' statement.
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 25 ++++++++++---------------
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 4bc7aefa9541..d6b9b6b5fb98 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -412,10 +412,9 @@ static u64 __get_rc6(struct drm_i915_private *i915)
> return val;
> }
>
> -static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> +static u64 get_rc6(struct drm_i915_private *i915)
> {
> #if IS_ENABLED(CONFIG_PM)
> - unsigned long flags;
> u64 val;
>
> if (intel_runtime_pm_get_if_in_use(i915)) {
> @@ -428,18 +427,12 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> * previously.
> */
>
> - if (!locked)
> - spin_lock_irqsave(&i915->pmu.lock, flags);
> -
> if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
> i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
> } else {
> val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> }
> -
> - if (!locked)
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> } else {
> struct pci_dev *pdev = i915->drm.pdev;
> struct device *kdev = &pdev->dev;
> @@ -452,9 +445,6 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> * on top of the last known real value, as the approximated RC6
> * counter value.
> */
> - if (!locked)
> - spin_lock_irqsave(&i915->pmu.lock, flags);
> -
> spin_lock_irqsave(&kdev->power.lock, flags2);
>
> if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> @@ -470,9 +460,6 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> val = jiffies_to_nsecs(val);
> val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> -
> - if (!locked)
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> }
>
> return val;
> @@ -519,7 +506,15 @@ static u64 __i915_pmu_event_read(struct perf_event *event, bool locked)
> val = count_interrupts(i915);
> break;
> case I915_PMU_RC6_RESIDENCY:
> - val = get_rc6(i915, locked);
> + if (!locked) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&i915->pmu.lock, flags);
> + val = get_rc6(i915);
> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + } else {
> + val = get_rc6(i915);
> + }
> break;
> }
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
@ 2018-03-13 17:46 ` Tvrtko Ursulin
0 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2018-03-13 17:46 UTC (permalink / raw)
To: Arnd Bergmann, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie
Cc: intel-gfx, linux-kernel, dri-devel
On 13/03/2018 16:19, Arnd Bergmann wrote:
> The conditional spinlock confuses gcc into thinking the 'flags' value
> might contain uninitialized data:
>
> drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
> arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
Hm, how does paravirt_types.h comes into the picture?
> The code is correct, but it's easy to see how the compiler gets confused
> here. This avoids the problem by pulling the lock outside of the function
> into its only caller.
Is it specific gcc version, specific options, or specific kernel config
that this happens?
Strange that it hasn't been seen so far.
Regards,
Tvrtko
> Fixes: 1fe699e30113 ("drm/i915/pmu: Fix sleep under atomic in RC6 readout")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: removed unused function argument, fixed 'break' statement.
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 25 ++++++++++---------------
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 4bc7aefa9541..d6b9b6b5fb98 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -412,10 +412,9 @@ static u64 __get_rc6(struct drm_i915_private *i915)
> return val;
> }
>
> -static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> +static u64 get_rc6(struct drm_i915_private *i915)
> {
> #if IS_ENABLED(CONFIG_PM)
> - unsigned long flags;
> u64 val;
>
> if (intel_runtime_pm_get_if_in_use(i915)) {
> @@ -428,18 +427,12 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> * previously.
> */
>
> - if (!locked)
> - spin_lock_irqsave(&i915->pmu.lock, flags);
> -
> if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
> i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
> } else {
> val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> }
> -
> - if (!locked)
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> } else {
> struct pci_dev *pdev = i915->drm.pdev;
> struct device *kdev = &pdev->dev;
> @@ -452,9 +445,6 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> * on top of the last known real value, as the approximated RC6
> * counter value.
> */
> - if (!locked)
> - spin_lock_irqsave(&i915->pmu.lock, flags);
> -
> spin_lock_irqsave(&kdev->power.lock, flags2);
>
> if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> @@ -470,9 +460,6 @@ static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> val = jiffies_to_nsecs(val);
> val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> -
> - if (!locked)
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> }
>
> return val;
> @@ -519,7 +506,15 @@ static u64 __i915_pmu_event_read(struct perf_event *event, bool locked)
> val = count_interrupts(i915);
> break;
> case I915_PMU_RC6_RESIDENCY:
> - val = get_rc6(i915, locked);
> + if (!locked) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&i915->pmu.lock, flags);
> + val = get_rc6(i915);
> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + } else {
> + val = get_rc6(i915);
> + }
> break;
> }
> }
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
2018-03-13 17:46 ` Tvrtko Ursulin
@ 2018-03-13 20:10 ` Arnd Bergmann
-1 siblings, 0 replies; 13+ messages in thread
From: Arnd Bergmann @ 2018-03-13 20:10 UTC (permalink / raw)
To: Tvrtko Ursulin
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Intel Graphics, Linux Kernel Mailing List, dri-devel
On Tue, Mar 13, 2018 at 6:46 PM, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
> On 13/03/2018 16:19, Arnd Bergmann wrote:
>>
>> The conditional spinlock confuses gcc into thinking the 'flags' value
>> might contain uninitialized data:
>>
>> drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
>> arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>
>
> Hm, how does paravirt_types.h comes into the picture?
spin_unlock_irqrestore() calls arch_local_irq_restore()
>> The code is correct, but it's easy to see how the compiler gets confused
>> here. This avoids the problem by pulling the lock outside of the function
>> into its only caller.
>
>
> Is it specific gcc version, specific options, or specific kernel config that
> this happens?
Not gcc version specific (same result with gcc-4.9 through 8, didn't test
earlier versions that are currently broken).
> Strange that it hasn't been seen so far.
It seems to be a relatively rare 'randconfig' combination. Looking at
the preprocessed sources, I find:
static u64 get_rc6(struct drm_i915_private *i915, bool locked)
{
unsigned long flags;
u64 val;
if (intel_runtime_pm_get_if_in_use(i915)) {
val = __get_rc6(i915);
intel_runtime_pm_put(i915);
if (!locked)
do { do { ({ unsigned long __dummy; typeof(flags) __dummy2;
(void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
__dummy; typeof(flags) __dummy2; (void)(&__dummy == &__dummy2); 1; });
flags = arch_local_irq_save(); } while (0); trace_hardirqs_off(); }
while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
(void)(spinlock_check(&i915->pmu.lock)); } while (0); } while (0); }
while (0); } while (0); } while (0);
if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
} else {
val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
}
if (!locked)
spin_unlock_irqrestore(&i915->pmu.lock, flags);
} else {
struct pci_dev *pdev = i915->drm.pdev;
struct device *kdev = &pdev->dev;
unsigned long flags2;
# 455 "/git/arm-soc/drivers/gpu/drm/i915/i915_pmu.c"
if (!locked)
do { do { ({ unsigned long __dummy; typeof(flags) __dummy2;
(void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
__dummy; typeof(flags) __dummy2; (void)(&__dummy == &__dummy2); 1; });
flags = arch_local_irq_save(); } while (0); trace_hardirqs_off(); }
while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
(void)(spinlock_check(&i915->pmu.lock)); } while (0); } while (0); }
while (0); } while (0); } while (0);
do { do { ({ unsigned long __dummy; typeof(flags2) __dummy2;
(void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
__dummy; typeof(flags2) __dummy2; (void)(&__dummy == &__dummy2); 1;
}); flags2 = arch_local_irq_save(); } while (0); trace_hardirqs_off();
} while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
(void)(spinlock_check(&kdev->power.lock)); } while (0); } while (0); }
while (0); } while (0); } while (0);
if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
i915->pmu.suspended_jiffies_last =
kdev->power.suspended_jiffies;
val = kdev->power.suspended_jiffies -
i915->pmu.suspended_jiffies_last;
val += jiffies - kdev->power.accounting_timestamp;
spin_unlock_irqrestore(&kdev->power.lock, flags2);
val = jiffies_to_nsecs(val);
val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
if (!locked)
spin_unlock_irqrestore(&i915->pmu.lock, flags);
}
return val;
}
so it seems that the spin_lock_irqsave() is completely inlined through
a macro while the unlock is not, and the lock contains a memory barrier
(among other things) that might tell the compiler that the state of the
'locked' flag could changed underneath it.
It could also be the problem that arch_local_irq_restore() uses
__builtin_expect() in PVOP_TEST_NULL(op) when
CONFIG_PARAVIRT_DEBUG is enabled, see
static inline __attribute__((unused))
__attribute__((no_instrument_function))
__attribute__((no_instrument_function)) void
arch_local_irq_restore(unsigned long f)
{
({ unsigned long __eax = __eax, __edx = __edx, __ecx = __ecx;; do {
if (__builtin_expect(!!(pv_irq_ops.restore_fl.func == ((void *)0)),
0)) do { do { asm volatile("1:\t" ".byte 0x0f, 0x0b" "\n"
".pushsection __bug_table,\"aw\"\n" "2:\t" ".long " "1b" "\t#
bug_entry::bug_addr\n" "\t" ".long " "%c0" "\t# bug_entry::file\n"
"\t.word %c1" "\t# bug_entry::line\n" "\t.word %c2" "\t#
bug_entry::flags\n" "\t.org 2b+%c3\n" ".popsection" : : "i"
("/git/arm-soc/arch/x86/include/asm/paravirt.h"), "i" (783), "i" (0),
"i" (sizeof(struct bug_entry))); } while (0); do { ; asm volatile("");
__builtin_unreachable(); } while (0); } while (0); } while (0); asm
volatile("" "771:\n\t" "999:\n\t" ".pushsection
.discard.retpoline_safe\n\t" " " ".long" " " " 999b\n\t"
".popsection\n\t" "call *%c[paravirt_opptr];" "\n" "772:\n"
".pushsection .parainstructions,\"a\"\n" " " ".balign 4" " " "\n" " "
".long" " " " 771b\n" " .byte " "%c[paravirt_typenum]" "\n" " .byte
772b-771b\n" " .short " "%c[paravirt_clobber]" "\n" ".popsection\n"
"" : "=a" (__eax), "=d" (__edx), "+r" (current_stack_pointer) :
[paravirt_typenum] "i" ((__builtin_offsetof(struct
paravirt_patch_template, pv_irq_ops.restore_fl.func) / sizeof(void
*))), [paravirt_opptr] "i" (&(pv_irq_ops.restore_fl.func)),
[paravirt_clobber] "i" (((1 << 0) | (1 << 2))), "a" ((unsigned
long)(f)) : "memory", "cc" ); });
}
this seems to frequently confuse gcc, and turning off that NULL check
avoids the warning as well.
If you want to analyze it further, see https://pastebin.com/T2yLRqU5
for the .config file, but I'm pretty sure this is a known problem with gcc
that happens to be very hard to fix.
Arnd
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
@ 2018-03-13 20:10 ` Arnd Bergmann
0 siblings, 0 replies; 13+ messages in thread
From: Arnd Bergmann @ 2018-03-13 20:10 UTC (permalink / raw)
To: Tvrtko Ursulin
Cc: David Airlie, Intel Graphics, Linux Kernel Mailing List,
dri-devel, Rodrigo Vivi
On Tue, Mar 13, 2018 at 6:46 PM, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
> On 13/03/2018 16:19, Arnd Bergmann wrote:
>>
>> The conditional spinlock confuses gcc into thinking the 'flags' value
>> might contain uninitialized data:
>>
>> drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
>> arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>
>
> Hm, how does paravirt_types.h comes into the picture?
spin_unlock_irqrestore() calls arch_local_irq_restore()
>> The code is correct, but it's easy to see how the compiler gets confused
>> here. This avoids the problem by pulling the lock outside of the function
>> into its only caller.
>
>
> Is it specific gcc version, specific options, or specific kernel config that
> this happens?
Not gcc version specific (same result with gcc-4.9 through 8, didn't test
earlier versions that are currently broken).
> Strange that it hasn't been seen so far.
It seems to be a relatively rare 'randconfig' combination. Looking at
the preprocessed sources, I find:
static u64 get_rc6(struct drm_i915_private *i915, bool locked)
{
unsigned long flags;
u64 val;
if (intel_runtime_pm_get_if_in_use(i915)) {
val = __get_rc6(i915);
intel_runtime_pm_put(i915);
if (!locked)
do { do { ({ unsigned long __dummy; typeof(flags) __dummy2;
(void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
__dummy; typeof(flags) __dummy2; (void)(&__dummy == &__dummy2); 1; });
flags = arch_local_irq_save(); } while (0); trace_hardirqs_off(); }
while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
(void)(spinlock_check(&i915->pmu.lock)); } while (0); } while (0); }
while (0); } while (0); } while (0);
if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
} else {
val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
}
if (!locked)
spin_unlock_irqrestore(&i915->pmu.lock, flags);
} else {
struct pci_dev *pdev = i915->drm.pdev;
struct device *kdev = &pdev->dev;
unsigned long flags2;
# 455 "/git/arm-soc/drivers/gpu/drm/i915/i915_pmu.c"
if (!locked)
do { do { ({ unsigned long __dummy; typeof(flags) __dummy2;
(void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
__dummy; typeof(flags) __dummy2; (void)(&__dummy == &__dummy2); 1; });
flags = arch_local_irq_save(); } while (0); trace_hardirqs_off(); }
while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
(void)(spinlock_check(&i915->pmu.lock)); } while (0); } while (0); }
while (0); } while (0); } while (0);
do { do { ({ unsigned long __dummy; typeof(flags2) __dummy2;
(void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
__dummy; typeof(flags2) __dummy2; (void)(&__dummy == &__dummy2); 1;
}); flags2 = arch_local_irq_save(); } while (0); trace_hardirqs_off();
} while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
(void)(spinlock_check(&kdev->power.lock)); } while (0); } while (0); }
while (0); } while (0); } while (0);
if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
i915->pmu.suspended_jiffies_last =
kdev->power.suspended_jiffies;
val = kdev->power.suspended_jiffies -
i915->pmu.suspended_jiffies_last;
val += jiffies - kdev->power.accounting_timestamp;
spin_unlock_irqrestore(&kdev->power.lock, flags2);
val = jiffies_to_nsecs(val);
val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
if (!locked)
spin_unlock_irqrestore(&i915->pmu.lock, flags);
}
return val;
}
so it seems that the spin_lock_irqsave() is completely inlined through
a macro while the unlock is not, and the lock contains a memory barrier
(among other things) that might tell the compiler that the state of the
'locked' flag could changed underneath it.
It could also be the problem that arch_local_irq_restore() uses
__builtin_expect() in PVOP_TEST_NULL(op) when
CONFIG_PARAVIRT_DEBUG is enabled, see
static inline __attribute__((unused))
__attribute__((no_instrument_function))
__attribute__((no_instrument_function)) void
arch_local_irq_restore(unsigned long f)
{
({ unsigned long __eax = __eax, __edx = __edx, __ecx = __ecx;; do {
if (__builtin_expect(!!(pv_irq_ops.restore_fl.func == ((void *)0)),
0)) do { do { asm volatile("1:\t" ".byte 0x0f, 0x0b" "\n"
".pushsection __bug_table,\"aw\"\n" "2:\t" ".long " "1b" "\t#
bug_entry::bug_addr\n" "\t" ".long " "%c0" "\t# bug_entry::file\n"
"\t.word %c1" "\t# bug_entry::line\n" "\t.word %c2" "\t#
bug_entry::flags\n" "\t.org 2b+%c3\n" ".popsection" : : "i"
("/git/arm-soc/arch/x86/include/asm/paravirt.h"), "i" (783), "i" (0),
"i" (sizeof(struct bug_entry))); } while (0); do { ; asm volatile("");
__builtin_unreachable(); } while (0); } while (0); } while (0); asm
volatile("" "771:\n\t" "999:\n\t" ".pushsection
.discard.retpoline_safe\n\t" " " ".long" " " " 999b\n\t"
".popsection\n\t" "call *%c[paravirt_opptr];" "\n" "772:\n"
".pushsection .parainstructions,\"a\"\n" " " ".balign 4" " " "\n" " "
".long" " " " 771b\n" " .byte " "%c[paravirt_typenum]" "\n" " .byte
772b-771b\n" " .short " "%c[paravirt_clobber]" "\n" ".popsection\n"
"" : "=a" (__eax), "=d" (__edx), "+r" (current_stack_pointer) :
[paravirt_typenum] "i" ((__builtin_offsetof(struct
paravirt_patch_template, pv_irq_ops.restore_fl.func) / sizeof(void
*))), [paravirt_opptr] "i" (&(pv_irq_ops.restore_fl.func)),
[paravirt_clobber] "i" (((1 << 0) | (1 << 2))), "a" ((unsigned
long)(f)) : "memory", "cc" ); });
}
this seems to frequently confuse gcc, and turning off that NULL check
avoids the warning as well.
If you want to analyze it further, see https://pastebin.com/T2yLRqU5
for the .config file, but I'm pretty sure this is a known problem with gcc
that happens to be very hard to fix.
Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
2018-03-13 20:10 ` Arnd Bergmann
@ 2018-03-14 7:43 ` Tvrtko Ursulin
-1 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2018-03-14 7:43 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Intel Graphics, Linux Kernel Mailing List, dri-devel
On 13/03/2018 20:10, Arnd Bergmann wrote:
> On Tue, Mar 13, 2018 at 6:46 PM, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>> On 13/03/2018 16:19, Arnd Bergmann wrote:
>>>
>>> The conditional spinlock confuses gcc into thinking the 'flags' value
>>> might contain uninitialized data:
>>>
>>> drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
>>> arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>
>>
>> Hm, how does paravirt_types.h comes into the picture?
>
> spin_unlock_irqrestore() calls arch_local_irq_restore()
>
>>> The code is correct, but it's easy to see how the compiler gets confused
>>> here. This avoids the problem by pulling the lock outside of the function
>>> into its only caller.
>>
>>
>> Is it specific gcc version, specific options, or specific kernel config that
>> this happens?
>
> Not gcc version specific (same result with gcc-4.9 through 8, didn't test
> earlier versions that are currently broken).
>
>> Strange that it hasn't been seen so far.
>
> It seems to be a relatively rare 'randconfig' combination. Looking at
> the preprocessed sources, I find:
>
> static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> {
>
> unsigned long flags;
> u64 val;
>
> if (intel_runtime_pm_get_if_in_use(i915)) {
> val = __get_rc6(i915);
> intel_runtime_pm_put(i915);
> if (!locked)
> do { do { ({ unsigned long __dummy; typeof(flags) __dummy2;
> (void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
> __dummy; typeof(flags) __dummy2; (void)(&__dummy == &__dummy2); 1; });
> flags = arch_local_irq_save(); } while (0); trace_hardirqs_off(); }
> while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
> (void)(spinlock_check(&i915->pmu.lock)); } while (0); } while (0); }
> while (0); } while (0); } while (0);
>
> if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
> i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
> } else {
> val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> }
> if (!locked)
> spin_unlock_irqrestore(&i915->pmu.lock, flags);
> } else {
> struct pci_dev *pdev = i915->drm.pdev;
> struct device *kdev = &pdev->dev;
> unsigned long flags2;
> # 455 "/git/arm-soc/drivers/gpu/drm/i915/i915_pmu.c"
> if (!locked)
> do { do { ({ unsigned long __dummy; typeof(flags) __dummy2;
> (void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
> __dummy; typeof(flags) __dummy2; (void)(&__dummy == &__dummy2); 1; });
> flags = arch_local_irq_save(); } while (0); trace_hardirqs_off(); }
> while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
> (void)(spinlock_check(&i915->pmu.lock)); } while (0); } while (0); }
> while (0); } while (0); } while (0);
>
> do { do { ({ unsigned long __dummy; typeof(flags2) __dummy2;
> (void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
> __dummy; typeof(flags2) __dummy2; (void)(&__dummy == &__dummy2); 1;
> }); flags2 = arch_local_irq_save(); } while (0); trace_hardirqs_off();
> } while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
> (void)(spinlock_check(&kdev->power.lock)); } while (0); } while (0); }
> while (0); } while (0); } while (0);
>
> if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> i915->pmu.suspended_jiffies_last =
> kdev->power.suspended_jiffies;
>
> val = kdev->power.suspended_jiffies -
> i915->pmu.suspended_jiffies_last;
> val += jiffies - kdev->power.accounting_timestamp;
>
> spin_unlock_irqrestore(&kdev->power.lock, flags2);
>
> val = jiffies_to_nsecs(val);
> val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
>
> if (!locked)
> spin_unlock_irqrestore(&i915->pmu.lock, flags);
> }
> return val;
> }
>
> so it seems that the spin_lock_irqsave() is completely inlined through
> a macro while the unlock is not, and the lock contains a memory barrier
> (among other things) that might tell the compiler that the state of the
> 'locked' flag could changed underneath it.
Ha, interesting. So it sounds more like us having to workaround a bug in
the paravirt spinlock macros.
I think I would prefer a different solution, where we don't end up doing
MMIO under irqsave spinlock. I'll send a patch.
Regards,
Tvrtko
>
> It could also be the problem that arch_local_irq_restore() uses
> __builtin_expect() in PVOP_TEST_NULL(op) when
> CONFIG_PARAVIRT_DEBUG is enabled, see
>
> static inline __attribute__((unused))
> __attribute__((no_instrument_function))
> __attribute__((no_instrument_function)) void
> arch_local_irq_restore(unsigned long f)
> {
> ({ unsigned long __eax = __eax, __edx = __edx, __ecx = __ecx;; do {
> if (__builtin_expect(!!(pv_irq_ops.restore_fl.func == ((void *)0)),
> 0)) do { do { asm volatile("1:\t" ".byte 0x0f, 0x0b" "\n"
> ".pushsection __bug_table,\"aw\"\n" "2:\t" ".long " "1b" "\t#
> bug_entry::bug_addr\n" "\t" ".long " "%c0" "\t# bug_entry::file\n"
> "\t.word %c1" "\t# bug_entry::line\n" "\t.word %c2" "\t#
> bug_entry::flags\n" "\t.org 2b+%c3\n" ".popsection" : : "i"
> ("/git/arm-soc/arch/x86/include/asm/paravirt.h"), "i" (783), "i" (0),
> "i" (sizeof(struct bug_entry))); } while (0); do { ; asm volatile("");
> __builtin_unreachable(); } while (0); } while (0); } while (0); asm
> volatile("" "771:\n\t" "999:\n\t" ".pushsection
> .discard.retpoline_safe\n\t" " " ".long" " " " 999b\n\t"
> ".popsection\n\t" "call *%c[paravirt_opptr];" "\n" "772:\n"
> ".pushsection .parainstructions,\"a\"\n" " " ".balign 4" " " "\n" ""
> ".long" " " " 771b\n" " .byte " "%c[paravirt_typenum]" "\n" " .byte
> 772b-771b\n" " .short " "%c[paravirt_clobber]" "\n" ".popsection\n"
> "" : "=a" (__eax), "=d" (__edx), "+r" (current_stack_pointer) :
> [paravirt_typenum] "i" ((__builtin_offsetof(struct
> paravirt_patch_template, pv_irq_ops.restore_fl.func) / sizeof(void
> *))), [paravirt_opptr] "i" (&(pv_irq_ops.restore_fl.func)),
> [paravirt_clobber] "i" (((1 << 0) | (1 << 2))), "a" ((unsigned
> long)(f)) : "memory", "cc" ); });
> }
>
> this seems to frequently confuse gcc, and turning off that NULL check
> avoids the warning as well.
>
> If you want to analyze it further, see https://pastebin.com/T2yLRqU5
> for the .config file, but I'm pretty sure this is a known problem with gcc
> that happens to be very hard to fix.
>
> Arnd
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
@ 2018-03-14 7:43 ` Tvrtko Ursulin
0 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2018-03-14 7:43 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Airlie, Intel Graphics, Linux Kernel Mailing List,
dri-devel, Rodrigo Vivi
On 13/03/2018 20:10, Arnd Bergmann wrote:
> On Tue, Mar 13, 2018 at 6:46 PM, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>> On 13/03/2018 16:19, Arnd Bergmann wrote:
>>>
>>> The conditional spinlock confuses gcc into thinking the 'flags' value
>>> might contain uninitialized data:
>>>
>>> drivers/gpu/drm/i915/i915_pmu.c: In function '__i915_pmu_event_read':
>>> arch/x86/include/asm/paravirt_types.h:573:3: error: 'flags' may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>
>>
>> Hm, how does paravirt_types.h comes into the picture?
>
> spin_unlock_irqrestore() calls arch_local_irq_restore()
>
>>> The code is correct, but it's easy to see how the compiler gets confused
>>> here. This avoids the problem by pulling the lock outside of the function
>>> into its only caller.
>>
>>
>> Is it specific gcc version, specific options, or specific kernel config that
>> this happens?
>
> Not gcc version specific (same result with gcc-4.9 through 8, didn't test
> earlier versions that are currently broken).
>
>> Strange that it hasn't been seen so far.
>
> It seems to be a relatively rare 'randconfig' combination. Looking at
> the preprocessed sources, I find:
>
> static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> {
>
> unsigned long flags;
> u64 val;
>
> if (intel_runtime_pm_get_if_in_use(i915)) {
> val = __get_rc6(i915);
> intel_runtime_pm_put(i915);
> if (!locked)
> do { do { ({ unsigned long __dummy; typeof(flags) __dummy2;
> (void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
> __dummy; typeof(flags) __dummy2; (void)(&__dummy == &__dummy2); 1; });
> flags = arch_local_irq_save(); } while (0); trace_hardirqs_off(); }
> while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
> (void)(spinlock_check(&i915->pmu.lock)); } while (0); } while (0); }
> while (0); } while (0); } while (0);
>
> if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
> i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
> } else {
> val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> }
> if (!locked)
> spin_unlock_irqrestore(&i915->pmu.lock, flags);
> } else {
> struct pci_dev *pdev = i915->drm.pdev;
> struct device *kdev = &pdev->dev;
> unsigned long flags2;
> # 455 "/git/arm-soc/drivers/gpu/drm/i915/i915_pmu.c"
> if (!locked)
> do { do { ({ unsigned long __dummy; typeof(flags) __dummy2;
> (void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
> __dummy; typeof(flags) __dummy2; (void)(&__dummy == &__dummy2); 1; });
> flags = arch_local_irq_save(); } while (0); trace_hardirqs_off(); }
> while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
> (void)(spinlock_check(&i915->pmu.lock)); } while (0); } while (0); }
> while (0); } while (0); } while (0);
>
> do { do { ({ unsigned long __dummy; typeof(flags2) __dummy2;
> (void)(&__dummy == &__dummy2); 1; }); do { do { do { ({ unsigned long
> __dummy; typeof(flags2) __dummy2; (void)(&__dummy == &__dummy2); 1;
> }); flags2 = arch_local_irq_save(); } while (0); trace_hardirqs_off();
> } while (0); do { __asm__ __volatile__("": : :"memory"); do { (void)0;
> (void)(spinlock_check(&kdev->power.lock)); } while (0); } while (0); }
> while (0); } while (0); } while (0);
>
> if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> i915->pmu.suspended_jiffies_last =
> kdev->power.suspended_jiffies;
>
> val = kdev->power.suspended_jiffies -
> i915->pmu.suspended_jiffies_last;
> val += jiffies - kdev->power.accounting_timestamp;
>
> spin_unlock_irqrestore(&kdev->power.lock, flags2);
>
> val = jiffies_to_nsecs(val);
> val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
>
> if (!locked)
> spin_unlock_irqrestore(&i915->pmu.lock, flags);
> }
> return val;
> }
>
> so it seems that the spin_lock_irqsave() is completely inlined through
> a macro while the unlock is not, and the lock contains a memory barrier
> (among other things) that might tell the compiler that the state of the
> 'locked' flag could changed underneath it.
Ha, interesting. So it sounds more like us having to workaround a bug in
the paravirt spinlock macros.
I think I would prefer a different solution, where we don't end up doing
MMIO under irqsave spinlock. I'll send a patch.
Regards,
Tvrtko
>
> It could also be the problem that arch_local_irq_restore() uses
> __builtin_expect() in PVOP_TEST_NULL(op) when
> CONFIG_PARAVIRT_DEBUG is enabled, see
>
> static inline __attribute__((unused))
> __attribute__((no_instrument_function))
> __attribute__((no_instrument_function)) void
> arch_local_irq_restore(unsigned long f)
> {
> ({ unsigned long __eax = __eax, __edx = __edx, __ecx = __ecx;; do {
> if (__builtin_expect(!!(pv_irq_ops.restore_fl.func == ((void *)0)),
> 0)) do { do { asm volatile("1:\t" ".byte 0x0f, 0x0b" "\n"
> ".pushsection __bug_table,\"aw\"\n" "2:\t" ".long " "1b" "\t#
> bug_entry::bug_addr\n" "\t" ".long " "%c0" "\t# bug_entry::file\n"
> "\t.word %c1" "\t# bug_entry::line\n" "\t.word %c2" "\t#
> bug_entry::flags\n" "\t.org 2b+%c3\n" ".popsection" : : "i"
> ("/git/arm-soc/arch/x86/include/asm/paravirt.h"), "i" (783), "i" (0),
> "i" (sizeof(struct bug_entry))); } while (0); do { ; asm volatile("");
> __builtin_unreachable(); } while (0); } while (0); } while (0); asm
> volatile("" "771:\n\t" "999:\n\t" ".pushsection
> .discard.retpoline_safe\n\t" " " ".long" " " " 999b\n\t"
> ".popsection\n\t" "call *%c[paravirt_opptr];" "\n" "772:\n"
> ".pushsection .parainstructions,\"a\"\n" " " ".balign 4" " " "\n" ""
> ".long" " " " 771b\n" " .byte " "%c[paravirt_typenum]" "\n" " .byte
> 772b-771b\n" " .short " "%c[paravirt_clobber]" "\n" ".popsection\n"
> "" : "=a" (__eax), "=d" (__edx), "+r" (current_stack_pointer) :
> [paravirt_typenum] "i" ((__builtin_offsetof(struct
> paravirt_patch_template, pv_irq_ops.restore_fl.func) / sizeof(void
> *))), [paravirt_opptr] "i" (&(pv_irq_ops.restore_fl.func)),
> [paravirt_clobber] "i" (((1 << 0) | (1 << 2))), "a" ((unsigned
> long)(f)) : "memory", "cc" ); });
> }
>
> this seems to frequently confuse gcc, and turning off that NULL check
> avoids the warning as well.
>
> If you want to analyze it further, see https://pastebin.com/T2yLRqU5
> for the .config file, but I'm pretty sure this is a known problem with gcc
> that happens to be very hard to fix.
>
> Arnd
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-03-14 7:43 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-13 16:19 [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning Arnd Bergmann
2018-03-13 16:38 ` Chris Wilson
2018-03-13 16:38 ` Chris Wilson
2018-03-13 16:46 ` ✗ Fi.CI.SPARSE: warning for drm/i915/pmu: avoid -Wmaybe-uninitialized warning (rev2) Patchwork
2018-03-13 17:41 ` Tvrtko Ursulin
2018-03-13 17:45 ` Chris Wilson
2018-03-13 17:02 ` ✗ Fi.CI.BAT: " Patchwork
2018-03-13 17:46 ` [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning Tvrtko Ursulin
2018-03-13 17:46 ` Tvrtko Ursulin
2018-03-13 20:10 ` Arnd Bergmann
2018-03-13 20:10 ` Arnd Bergmann
2018-03-14 7:43 ` Tvrtko Ursulin
2018-03-14 7:43 ` Tvrtko Ursulin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.