[PATCH v2] iio: adc: mt6359: fix unchecked return value in mt6358_read_imp

[PATCH v2] iio: adc: mt6359: fix unchecked return value in mt6358_read_imp

[Salah Triki]

In mt6358_read_imp(), the return value of regmap_read() is currently
ignored. This is problematic because if the bus read fails the variable
val_v remains uninitialized.

The function subsequently assigns this uninitialized stack value to
*vbat, leading to incorrect measurement results being reported to
the IIO subsystem without any error indication.

Update the function to check the return value of regmap_read(). Ensure
that mt6358_stop_imp_conv() is still called to clean up the hardware
state before returning the error code.

Fixes: 3587914bf61 ("iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC")
Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
Changes in v2:
- Added Fixes tag.
- Re-examined the entire driver for unchecked regmap operations.
  While several regmap_write() and regmap_set_bits() calls also ignore
  return values, I focused on this specific regmap_read() in 
  mt6358_read_imp() because it leads to an uninitialized variable usage
  (val_v). This makes this fix critical for reporting correct data 
  to userspace.

 drivers/iio/adc/mt6359-auxadc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
index 6b9ed9b1fde2..f927bff4a26a 100644
--- a/drivers/iio/adc/mt6359-auxadc.c
+++ b/drivers/iio/adc/mt6359-auxadc.c
@@ -497,10 +497,13 @@ static int mt6358_read_imp(struct mt6359_auxadc *adc_dev,
 		return ret;
 
 	/* Read the params before stopping */
-	regmap_read(regmap, reg_adc0 + (cinfo->imp_adc_num << 1), &val_v);
+	ret = regmap_read(regmap, reg_adc0 + (cinfo->imp_adc_num << 1), &val_v);
 
 	mt6358_stop_imp_conv(adc_dev);
 
+	if (ret)
+		return ret;
+
 	if (vbat)
 		*vbat = val_v;
 	if (ibat)
-- 
2.43.0

Re: [PATCH v2] iio: adc: mt6359: fix unchecked return value in mt6358_read_imp

[Jonathan Cameron]

On Mon, 27 Apr 2026 17:57:59 +0100
Salah Triki <salah.triki@gmail.com> wrote:

> In mt6358_read_imp(), the return value of regmap_read() is currently
> ignored. This is problematic because if the bus read fails the variable
> val_v remains uninitialized.
> 
> The function subsequently assigns this uninitialized stack value to
> *vbat, leading to incorrect measurement results being reported to
> the IIO subsystem without any error indication.
> 
> Update the function to check the return value of regmap_read(). Ensure
> that mt6358_stop_imp_conv() is still called to clean up the hardware
> state before returning the error code.
> 
> Fixes: 3587914bf61 ("iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC")
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
> Changes in v2:
> - Added Fixes tag.
> - Re-examined the entire driver for unchecked regmap operations.
>   While several regmap_write() and regmap_set_bits() calls also ignore
>   return values, I focused on this specific regmap_read() in 
>   mt6358_read_imp() because it leads to an uninitialized variable usage
>   (val_v). This makes this fix critical for reporting correct data 
>   to userspace.

I'd be more concerned about what it might otherwise report.. Will expose
something random that was on the stack previously.

Ok on this one having a fixes tag given the uninitialized, though as I mentioned
I'd have been tempted to take the approach of just initialising it.

Even with the code as you have it here a static analyzer may not be able
to see far enough to tell it is initialized and so throw false positive warnings.

Still, let's see what Andy thinks before you change anything.

J
> 
>  drivers/iio/adc/mt6359-auxadc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
> index 6b9ed9b1fde2..f927bff4a26a 100644
> --- a/drivers/iio/adc/mt6359-auxadc.c
> +++ b/drivers/iio/adc/mt6359-auxadc.c
> @@ -497,10 +497,13 @@ static int mt6358_read_imp(struct mt6359_auxadc *adc_dev,
>  		return ret;
>  
>  	/* Read the params before stopping */
> -	regmap_read(regmap, reg_adc0 + (cinfo->imp_adc_num << 1), &val_v);
> +	ret = regmap_read(regmap, reg_adc0 + (cinfo->imp_adc_num << 1), &val_v);
>  
>  	mt6358_stop_imp_conv(adc_dev);
>  
> +	if (ret)
> +		return ret;
> +
>  	if (vbat)
>  		*vbat = val_v;
>  	if (ibat)

Re: [PATCH v2] iio: adc: mt6359: fix unchecked return value in mt6358_read_imp

[Andy Shevchenko]

On Mon, Apr 27, 2026 at 06:18:56PM +0100, Jonathan Cameron wrote:
> On Mon, 27 Apr 2026 17:57:59 +0100
> Salah Triki <salah.triki@gmail.com> wrote:
> 
> > In mt6358_read_imp(), the return value of regmap_read() is currently
> > ignored. This is problematic because if the bus read fails the variable
> > val_v remains uninitialized.
> > 
> > The function subsequently assigns this uninitialized stack value to
> > *vbat, leading to incorrect measurement results being reported to
> > the IIO subsystem without any error indication.
> > 
> > Update the function to check the return value of regmap_read(). Ensure
> > that mt6358_stop_imp_conv() is still called to clean up the hardware
> > state before returning the error code.

> I'd be more concerned about what it might otherwise report.. Will expose
> something random that was on the stack previously.
> 
> Ok on this one having a fixes tag given the uninitialized, though as I mentioned
> I'd have been tempted to take the approach of just initialising it.
> 
> Even with the code as you have it here a static analyzer may not be able
> to see far enough to tell it is initialized and so throw false positive warnings.

True.

> Still, let's see what Andy thinks before you change anything.

...

> >  	/* Read the params before stopping */
> > -	regmap_read(regmap, reg_adc0 + (cinfo->imp_adc_num << 1), &val_v);
> > +	ret = regmap_read(regmap, reg_adc0 + (cinfo->imp_adc_num << 1), &val_v);

> >  	mt6358_stop_imp_conv(adc_dev);

This still uses unchecked IO.

> > +	if (ret)
> > +		return ret;

So, assume we got it right and error code is 0, but failed to stop conversion.
What would be the consequences?

As I said, the whole driver needs to be re-think of. If you going to check this
you need to add checks into _stop_imp_conv() which may lead to adding checks to
the complementary functions and it will go as a rabbit hole.

I agree with Jonathan that quick fix of initialising it explicitly with
a comment will be better approach taking into account the state of affairs
of this driver as a whole.

-- 
With Best Regards,
Andy Shevchenko