| Message ID | 20230510183606.2480777-1-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/hwmon: Silence UBSAN uninitialized bool variable warning | expand |
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. 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 >
Hi Ashutosh, 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. Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Andi > 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 > >
On 10.05.2023 20:36, 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. Silence the warning by initializing the variable to an > arbitrary value. > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Regards Andrzej > --- > 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); >
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 > > >
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);
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. 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(-)