Re: [PATCH] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
On Tue, Mar 13, 2018 at 1:31 PM, Chris Wilsonwrote: > Quoting Arnd Bergmann (2018-03-13 12:08:29) >> 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 >> --- >> 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..2c8c705d1f18 100644 >> --- a/drivers/gpu/drm/i915/i915_pmu.c >> +++ b/drivers/gpu/drm/i915/i915_pmu.c >> @@ -415,7 +415,6 @@ static u64 __get_rc6(struct drm_i915_private *i915) >> static u64 get_rc6(struct drm_i915_private *i915, bool locked) > > bool locked is now unused, right? Right, removing it now. >> - val = get_rc6(i915, locked); >> - break; >> + if (!locked) { >> + unsigned long flags; >> + >> + spin_lock_irqsave(>pmu.lock, flags); >> + val = get_rc6(i915, locked); >> + spin_unlock_irqrestore(>pmu.lock, >> flags); >> + break; >> + } else { >> + val = get_rc6(i915, locked); >> + } > > break is now on one path only; should be here. Fixed as well. I'll wait a bit for more comments before resending. Thanks for the review! Arnd
Re: [PATCH] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
On Tue, Mar 13, 2018 at 1:31 PM, Chris Wilson wrote: > Quoting Arnd Bergmann (2018-03-13 12:08:29) >> 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 >> --- >> 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..2c8c705d1f18 100644 >> --- a/drivers/gpu/drm/i915/i915_pmu.c >> +++ b/drivers/gpu/drm/i915/i915_pmu.c >> @@ -415,7 +415,6 @@ static u64 __get_rc6(struct drm_i915_private *i915) >> static u64 get_rc6(struct drm_i915_private *i915, bool locked) > > bool locked is now unused, right? Right, removing it now. >> - val = get_rc6(i915, locked); >> - break; >> + if (!locked) { >> + unsigned long flags; >> + >> + spin_lock_irqsave(>pmu.lock, flags); >> + val = get_rc6(i915, locked); >> + spin_unlock_irqrestore(>pmu.lock, >> flags); >> + break; >> + } else { >> + val = get_rc6(i915, locked); >> + } > > break is now on one path only; should be here. Fixed as well. I'll wait a bit for more comments before resending. Thanks for the review! Arnd
Re: [PATCH] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
Quoting Arnd Bergmann (2018-03-13 12:08:29) > 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> --- > 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..2c8c705d1f18 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -415,7 +415,6 @@ static u64 __get_rc6(struct drm_i915_private *i915) > static u64 get_rc6(struct drm_i915_private *i915, bool locked) bool locked is now unused, right? > { > #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,8 +506,16 @@ 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); > - break; > + if (!locked) { > + unsigned long flags; > + > + spin_lock_irqsave(>pmu.lock, flags); > + val = get_rc6(i915, locked); > + spin_unlock_irqrestore(>pmu.lock, > flags); > + break; > + } else { > + val = get_rc6(i915, locked); > + } break is now on one path only; should be here. -Chris
Re: [PATCH] drm/i915/pmu: avoid -Wmaybe-uninitialized warning
Quoting Arnd Bergmann (2018-03-13 12:08:29) > 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 > --- > 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..2c8c705d1f18 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -415,7 +415,6 @@ static u64 __get_rc6(struct drm_i915_private *i915) > static u64 get_rc6(struct drm_i915_private *i915, bool locked) bool locked is now unused, right? > { > #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,8 +506,16 @@ 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); > - break; > + if (!locked) { > + unsigned long flags; > + > + spin_lock_irqsave(>pmu.lock, flags); > + val = get_rc6(i915, locked); > + spin_unlock_irqrestore(>pmu.lock, > flags); > + break; > + } else { > + val = get_rc6(i915, locked); > + } break is now on one path only; should be here. -Chris