Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Silence UBSAN uninitialized bool variable warning

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Fri, 12 May 2023 02:33:33 -0700, Andi Shyti wrote:
>
Hi Andi,
>
> On Thu, May 11, 2023 at 10:43:30AM -0700, Dixit, Ashutosh wrote:
> > On Wed, 10 May 2023 11:36:06 -0700, Ashutosh Dixit wrote:
> > >
> > > Loading i915 on UBSAN enabled kernels (CONFIG_UBSAN/CONFIG_UBSAN_BOOL)
> > > causes the following warning:
> > >
> > >   UBSAN: invalid-load in drivers/gpu/drm/i915/gt/uc/intel_uc.c:558:2
> > >   load of value 255 is not a valid value for type '_Bool'
> > >   Call Trace:
> > >    dump_stack_lvl+0x57/0x7d
> > >    ubsan_epilogue+0x5/0x40
> > >    __ubsan_handle_load_invalid_value.cold+0x43/0x48
> > >    __uc_init_hw+0x76a/0x903 [i915]
> > >    ...
> > >    i915_driver_probe+0xfb1/0x1eb0 [i915]
> > >    i915_pci_probe+0xbe/0x2d0 [i915]
> > >
> > > The warning happens because during probe i915_hwmon is still not available
> > > which results in the output boolean variable *old remaining
> > > uninitialized.
> >
> > Note that the variable was uninitialized in this case but it was never used
> > uninitialized (the variable was not needed when it was uninitialized). So
> > there was no bug in the code. UBSAN warning is just complaining about the
> > uninitialized variable being passed into a function (where it is not used).
> >
> > Also the variable can be initialized in the caller (__uc_init_hw) too and
> > it will fix this issue. However in __uc_init_hw the assumption is that the
> > variable will be initialized in the callee (i915_hwmon_power_max_disable),
> > so that is how I have done it in this patch.
> >
> > I thought these clarifications will help with the review.
>
> I think we should not just consider what's now but also what can
> come later. The use of pl1en is not 100% future proof and
> therefore your patch, even though now is not fixing anything,
> might avoid wrong uses in the future.
>
> I'm just wondering, though, why not initializing the variable at
> it's declaration. As you wish.

OK, in v2 I went ahead and did just that (initializing the variable at the
declaration). I was splitting hair too much :/

> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks.
--
Ashutosh


> >
> > > Silence the warning by initializing the variable to an arbitrary value.
> > >
> > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_hwmon.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > > index a3bdd9f68a458..685663861bc0b 100644
> > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > @@ -502,8 +502,11 @@ void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old)
> > >	struct i915_hwmon *hwmon = i915->hwmon;
> > >	u32 r;
> > >
> > > -	if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit))
> > > +	if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) {
> > > +		/* Fix uninitialized bool variable warning */
> > > +		*old = false;
> > >		return;
> > > +	}
> > >
> > >	mutex_lock(&hwmon->hwmon_lock);
> > >
> > > --
> > > 2.38.0
> > >