[PATCH] iio: adc: stm32-adc: fix check on internal channel availability

[PATCH] iio: adc: stm32-adc: fix check on internal channel availability

[Fabrice Gasnier]

If an unsupported internal channel like vddgpu is requested, the driver
prints a warning but falls through and assigns it a valid int_ch below.

This causes a problem later during setup:
stm32_adc_int_ch_enable() {
    ...
    case STM32_ADC_INT_CH_VDDGPU:
        stm32_adc_set_bits(adc, adc->cfg->regs->or_vddgpu.reg,
                           adc->cfg->regs->or_vddgpu.mask);
    ...
}

Because the register offset is uninitialized (0), this performs a
read-modify-write on offset 0, which corresponds to the ISR register.

Fix this by returning before a valid int_ch is assigned.
Choice is made to just warn about the channel name as it could
be confusing, rather than making the probe fail.

Fixes: cf0fb80ae167 ("iio: adc: stm32-adc: add stm32mp13 support")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/all/20260911162602.D323F1F000FF@smtp.kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/iio/adc/stm32-adc.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 5c6c06b269be..87fafdd4698b 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -2259,7 +2259,7 @@ static int stm32_adc_populate_int_ch(struct iio_dev *indio_dev, const char *ch_n
 {
 	struct stm32_adc *adc = iio_priv(indio_dev);
 	u16 vrefint;
-	int i, ret;
+	int i, ret = 0;
 
 	for (i = 0; i < STM32_ADC_INT_CH_NB; i++) {
 		if (!strncmp(stm32_adc_ic[i].name, ch_name, STM32_ADC_CH_SZ)) {
@@ -2267,31 +2267,36 @@ static int stm32_adc_populate_int_ch(struct iio_dev *indio_dev, const char *ch_n
 			switch (i) {
 			case STM32_ADC_INT_CH_VDDCORE:
 				if (!adc->cfg->regs->or_vddcore.reg)
-					dev_warn(&indio_dev->dev,
-						 "%s channel not available\n", ch_name);
+					ret = -ENOENT;
 				break;
 			case STM32_ADC_INT_CH_VDDCPU:
 				if (!adc->cfg->regs->or_vddcpu.reg)
-					dev_warn(&indio_dev->dev,
-						 "%s channel not available\n", ch_name);
+					ret = -ENOENT;
 				break;
 			case STM32_ADC_INT_CH_VDDQ_DDR:
 				if (!adc->cfg->regs->or_vddq_ddr.reg)
-					dev_warn(&indio_dev->dev,
-						 "%s channel not available\n", ch_name);
+					ret = -ENOENT;
 				break;
 			case STM32_ADC_INT_CH_VREFINT:
 				if (!adc->cfg->regs->ccr_vref.reg)
-					dev_warn(&indio_dev->dev,
-						 "%s channel not available\n", ch_name);
+					ret = -ENOENT;
 				break;
 			case STM32_ADC_INT_CH_VBAT:
 				if (!adc->cfg->regs->ccr_vbat.reg)
-					dev_warn(&indio_dev->dev,
-						 "%s channel not available\n", ch_name);
+					ret = -ENOENT;
 				break;
 			}
 
+			if (ret) {
+				/*
+				 * Confusing channel label matches an internal STM32 ADC channel.
+				 * Just warn about it, as there's normally no restriction on the
+				 * name but that's not among supported internal channels.
+				 */
+				dev_warn(&indio_dev->dev, "no %s internal channel\n", ch_name);
+				return 0;
+			}
+
 			if (stm32_adc_ic[i].idx != STM32_ADC_INT_CH_VREFINT) {
 				adc->int_ch[i] = chan;
 				break;

---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260915-adc-fix-intchan-v1-3deda8cecb50

Best regards,
--  
Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Re: [PATCH] iio: adc: stm32-adc: fix check on internal channel availability

[Andy Shevchenko]

On Tue, Sep 15, 2026 at 06:15:49PM +0200, Fabrice Gasnier wrote:
> If an unsupported internal channel like vddgpu is requested, the driver
> prints a warning but falls through and assigns it a valid int_ch below.
> 
> This causes a problem later during setup:
> stm32_adc_int_ch_enable() {
>     ...
>     case STM32_ADC_INT_CH_VDDGPU:
>         stm32_adc_set_bits(adc, adc->cfg->regs->or_vddgpu.reg,
>                            adc->cfg->regs->or_vddgpu.mask);
>     ...
> }
> 
> Because the register offset is uninitialized (0), this performs a
> read-modify-write on offset 0, which corresponds to the ISR register.
> 
> Fix this by returning before a valid int_ch is assigned.
> Choice is made to just warn about the channel name as it could
> be confusing, rather than making the probe fail.

Are this and the other patch made with AI assistance?

...

>  	struct stm32_adc *adc = iio_priv(indio_dev);
>  	u16 vrefint;
> -	int i, ret;
> +	int i, ret = 0;

No, either assign closer to its first user, or do even better.

>  	for (i = 0; i < STM32_ADC_INT_CH_NB; i++) {
>  		if (!strncmp(stm32_adc_ic[i].name, ch_name, STM32_ADC_CH_SZ)) {
> @@ -2267,31 +2267,36 @@ static int stm32_adc_populate_int_ch(struct iio_dev *indio_dev, const char *ch_n
>  			switch (i) {
>  			case STM32_ADC_INT_CH_VDDCORE:
>  				if (!adc->cfg->regs->or_vddcore.reg)
> -					dev_warn(&indio_dev->dev,
> -						 "%s channel not available\n", ch_name);
> +					ret = -ENOENT;

This is a repetition of the same value. Instead add a boolean flag and do here

	bool na;
	...
			na = false; // or can be dropped with 'default' case
			switch (i) {
			case STM32_ADC_INT_CH_VDDCORE:
				na = !adc->cfg->regs->or_vddcore.reg;

>  				break;
>  			case STM32_ADC_INT_CH_VDDCPU:
>  				if (!adc->cfg->regs->or_vddcpu.reg)
> -					dev_warn(&indio_dev->dev,
> -						 "%s channel not available\n", ch_name);
> +					ret = -ENOENT;
>  				break;
>  			case STM32_ADC_INT_CH_VDDQ_DDR:
>  				if (!adc->cfg->regs->or_vddq_ddr.reg)
> -					dev_warn(&indio_dev->dev,
> -						 "%s channel not available\n", ch_name);
> +					ret = -ENOENT;
>  				break;
>  			case STM32_ADC_INT_CH_VREFINT:
>  				if (!adc->cfg->regs->ccr_vref.reg)
> -					dev_warn(&indio_dev->dev,
> -						 "%s channel not available\n", ch_name);
> +					ret = -ENOENT;
>  				break;
>  			case STM32_ADC_INT_CH_VBAT:
>  				if (!adc->cfg->regs->ccr_vbat.reg)
> -					dev_warn(&indio_dev->dev,
> -						 "%s channel not available\n", ch_name);
> +					ret = -ENOENT;
>  				break;

Don't you also need a default?

			default:
				return -EINVAL; // for example...

>  			}

			if (na) {
				...
				return 0;
			}

>  
> +			if (ret) {
> +				/*
> +				 * Confusing channel label matches an internal STM32 ADC channel.
> +				 * Just warn about it, as there's normally no restriction on the
> +				 * name but that's not among supported internal channels.
> +				 */
> +				dev_warn(&indio_dev->dev, "no %s internal channel\n", ch_name);
> +				return 0;

My gosh, the ret value is even ignored!

> +			}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: stm32-adc: fix check on internal channel availability

[Fabrice Gasnier]

On 9/16/26 09:45, Andy Shevchenko wrote:
> On Tue, Sep 15, 2026 at 06:15:49PM +0200, Fabrice Gasnier wrote:
>> If an unsupported internal channel like vddgpu is requested, the driver
>> prints a warning but falls through and assigns it a valid int_ch below.
>>
>> This causes a problem later during setup:
>> stm32_adc_int_ch_enable() {
>>     ...
>>     case STM32_ADC_INT_CH_VDDGPU:
>>         stm32_adc_set_bits(adc, adc->cfg->regs->or_vddgpu.reg,
>>                            adc->cfg->regs->or_vddgpu.mask);
>>     ...
>> }
>>
>> Because the register offset is uninitialized (0), this performs a
>> read-modify-write on offset 0, which corresponds to the ISR register.
>>
>> Fix this by returning before a valid int_ch is assigned.
>> Choice is made to just warn about the channel name as it could
>> be confusing, rather than making the probe fail.
> 
> Are this and the other patch made with AI assistance?

Hi Andy,

Not the solution (patch content) to fix the issue.

But most of the commit message is copied from Sashiko, as I find it
clear, see:
Link:
https://lore.kernel.org/all/20260911162602.D323F1F000FF@smtp.kernel.org/

I've added Reported-by tag. Do you think I should add more tags ?

The code bellow isn't assisted-by anything.

> 
> ...
> 
>>  	struct stm32_adc *adc = iio_priv(indio_dev);
>>  	u16 vrefint;
>> -	int i, ret;
>> +	int i, ret = 0;
> 
> No, either assign closer to its first user, or do even better.
> 
>>  	for (i = 0; i < STM32_ADC_INT_CH_NB; i++) {
>>  		if (!strncmp(stm32_adc_ic[i].name, ch_name, STM32_ADC_CH_SZ)) {
>> @@ -2267,31 +2267,36 @@ static int stm32_adc_populate_int_ch(struct iio_dev *indio_dev, const char *ch_n
>>  			switch (i) {
>>  			case STM32_ADC_INT_CH_VDDCORE:
>>  				if (!adc->cfg->regs->or_vddcore.reg)
>> -					dev_warn(&indio_dev->dev,
>> -						 "%s channel not available\n", ch_name);
>> +					ret = -ENOENT;
> 
> This is a repetition of the same value. Instead add a boolean flag and do here
> 
> 	bool na;
> 	...
> 			na = false; // or can be dropped with 'default' case
> 			switch (i) {
> 			case STM32_ADC_INT_CH_VDDCORE:
> 				na = !adc->cfg->regs->or_vddcore.reg;

Ack, thanks for suggesting! I will update in v2.

> 
>>  				break;
>>  			case STM32_ADC_INT_CH_VDDCPU:
>>  				if (!adc->cfg->regs->or_vddcpu.reg)
>> -					dev_warn(&indio_dev->dev,
>> -						 "%s channel not available\n", ch_name);
>> +					ret = -ENOENT;
>>  				break;
>>  			case STM32_ADC_INT_CH_VDDQ_DDR:
>>  				if (!adc->cfg->regs->or_vddq_ddr.reg)
>> -					dev_warn(&indio_dev->dev,
>> -						 "%s channel not available\n", ch_name);
>> +					ret = -ENOENT;
>>  				break;
>>  			case STM32_ADC_INT_CH_VREFINT:
>>  				if (!adc->cfg->regs->ccr_vref.reg)
>> -					dev_warn(&indio_dev->dev,
>> -						 "%s channel not available\n", ch_name);
>> +					ret = -ENOENT;
>>  				break;
>>  			case STM32_ADC_INT_CH_VBAT:
>>  				if (!adc->cfg->regs->ccr_vbat.reg)
>> -					dev_warn(&indio_dev->dev,
>> -						 "%s channel not available\n", ch_name);
>> +					ret = -ENOENT;
>>  				break;
> 
> Don't you also need a default?

Ack, I was wondering too. I will add a default in v2.

> 
> 			default:
> 				return -EINVAL; // for example...
> 
>>  			}
> 
> 			if (na) {
> 				...
> 				return 0;
> 			}
> 
>>  
>> +			if (ret) {
>> +				/*
>> +				 * Confusing channel label matches an internal STM32 ADC channel.
>> +				 * Just warn about it, as there's normally no restriction on the
>> +				 * name but that's not among supported internal channels.
>> +				 */
>> +				dev_warn(&indio_dev->dev, "no %s internal channel\n", ch_name);
>> +				return 0;
> 
> My gosh, the ret value is even ignored!

Yes, That's what I try to explain in the comment, e.g. Just warn (as it
is doing currently).

The purpose of the fix is not to change current driver behavior but to
address the undesired subsequent int_ch assignment which ends-up in
writing bits in stm32_adc_int_ch_enable() in an uncontrolled way.

Semantically, this ret value introduced in v1, will be turned into a
bool as you suggest. Hope you agree with this approach ?

BR,
Fabrice

> 
>> +			}
>