subject:"\[PATCH\] \[v2\] drm\/i915\/pmu\: avoid \-Wmaybe\-uninitialized warning"

Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning

2018-03-14 Thread Tvrtko Ursulin



On 13/03/2018 20:10, Arnd Bergmann wrote:

On Tue, Mar 13, 2018 at 6:46 PM, Tvrtko Ursulin
 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(>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(>pmu.lock, flags);
  } else {
   struct pci_dev *pdev = i915->drm.pdev;
   struct device *kdev = >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(>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(>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(>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(>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"

Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning

2018-03-13 Thread Arnd Bergmann

On Tue, Mar 13, 2018 at 6:46 PM, Tvrtko Ursulin
 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(>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(>pmu.lock, flags);
 } else {
  struct pci_dev *pdev = i915->drm.pdev;
  struct device *kdev = >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(>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(>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(>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(>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" " "

Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning

2018-03-13 Thread Tvrtko Ursulin



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 
---
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(>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(>pmu.lock, flags);
} else {
struct pci_dev *pdev = i915->drm.pdev;
struct device *kdev = >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(>pmu.lock, flags);
-
spin_lock_irqsave(>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(>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(>pmu.lock, flags);
+   val = get_rc6(i915);
+   spin_unlock_irqrestore(>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

Re: [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning

2018-03-13 Thread Chris Wilson

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 
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning

2018-03-13 Thread Arnd Bergmann

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 
---
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(>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(>pmu.lock, flags);
} else {
struct pci_dev *pdev = i915->drm.pdev;
struct device *kdev = >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(>pmu.lock, flags);
-
spin_lock_irqsave(>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(>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(>pmu.lock, flags);
+   val = get_rc6(i915);
+   spin_unlock_irqrestore(>pmu.lock, flags);
+   } else {
+   val = get_rc6(i915);
+   }
break;
}
}
-- 
2.9.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning

Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning

Re: [Intel-gfx] [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning

Re: [PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning

[PATCH] [v2] drm/i915/pmu: avoid -Wmaybe-uninitialized warning

5 matches

Site Navigation

Mail list logo

Footer information