[PATCH] mfd: wm8350: propagate ADC read timeout instead of returning 0

[PATCH] mfd: wm8350: propagate ADC read timeout instead of returning 0

[Ivy Lopez]

wm8350_read_auxadc() logs an error via dev_err() when the AUX ADC
read times out, but then falls through to return the masked value
of `result`, which was left at its initial value of 0. Callers have
no way to distinguish a genuine 0V reading from a failed read.

Return -ETIMEDOUT on timeout instead, and update the wm8350 hwmon
and power_supply drivers, which call this function directly, to
check for a negative return and propagate the error rather than
using it as a voltage value.

Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
 drivers/hwmon/wm8350-hwmon.c        |  9 ++++--
 drivers/mfd/wm8350-core.c           | 14 ++++++---
 drivers/power/supply/wm8350_power.c | 45 ++++++++++++++++++++++-------
 3 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/drivers/hwmon/wm8350-hwmon.c b/drivers/hwmon/wm8350-hwmon.c
index 10e17bb79895..a455e1c5476c 100644
--- a/drivers/hwmon/wm8350-hwmon.c
+++ b/drivers/hwmon/wm8350-hwmon.c
@@ -23,14 +23,17 @@ static const char * const input_names[] = {
 };
 
 static ssize_t show_voltage(struct device *dev,
-			    struct device_attribute *attr, char *buf)
+			  struct device_attribute *attr, char *buf)
 {
 	struct wm8350 *wm8350 = dev_get_drvdata(dev);
 	int channel = to_sensor_dev_attr(attr)->index;
 	int val;
 
-	val = wm8350_read_auxadc(wm8350, channel, 0, 0) * WM8350_AUX_COEFF;
-	val = DIV_ROUND_CLOSEST(val, 1000);
+	val = wm8350_read_auxadc(wm8350, channel, 0, 0);
+	if (val < 0)
+		return val;
+
+	val = DIV_ROUND_CLOSEST(val * WM8350_AUX_COEFF, 1000);
 
 	return sprintf(buf, "%d\n", val);
 }
diff --git a/drivers/mfd/wm8350-core.c b/drivers/mfd/wm8350-core.c
index fbc77b218215..3120c6b8fa3f 100644
--- a/drivers/mfd/wm8350-core.c
+++ b/drivers/mfd/wm8350-core.c
@@ -188,6 +188,7 @@ EXPORT_SYMBOL_GPL(wm8350_reg_unlock);
 int wm8350_read_auxadc(struct wm8350 *wm8350, int channel, int scale, int vref)
 {
 	u16 reg, result = 0;
+	int ret = 0;
 
 	if (channel < WM8350_AUXADC_AUX1 || channel > WM8350_AUXADC_TEMP)
 		return -EINVAL;
@@ -221,19 +222,24 @@ int wm8350_read_auxadc(struct wm8350 *wm8350, int channel, int scale, int vref)
 	wait_for_completion_timeout(&wm8350->auxadc_done, msecs_to_jiffies(5));
 
 	reg = wm8350_reg_read(wm8350, WM8350_DIGITISER_CONTROL_1);
-	if (reg & WM8350_AUXADC_POLL)
+	if (reg & WM8350_AUXADC_POLL) {
 		dev_err(wm8350->dev, "adc chn %d read timeout\n", channel);
-	else
+		ret = -ETIMEDOUT;
+	} else {
 		result = wm8350_reg_read(wm8350,
-					 WM8350_AUX1_READBACK + channel);
+					WM8350_AUX1_READBACK + channel);
+	}
 
 	/* Turn off the ADC */
 	reg = wm8350_reg_read(wm8350, WM8350_POWER_MGMT_5);
 	wm8350_reg_write(wm8350, WM8350_POWER_MGMT_5,
-			 reg & ~WM8350_AUXADC_ENA);
+			reg & ~WM8350_AUXADC_ENA);
 
 	mutex_unlock(&wm8350->auxadc_mutex);
 
+	if (ret)
+		return ret;
+
 	return result & WM8350_AUXADC_DATA1_MASK;
 }
 EXPORT_SYMBOL_GPL(wm8350_read_auxadc);
diff --git a/drivers/power/supply/wm8350_power.c b/drivers/power/supply/wm8350_power.c
index b0eb6e0ce8bc..111373027905 100644
--- a/drivers/power/supply/wm8350_power.c
+++ b/drivers/power/supply/wm8350_power.c
@@ -19,20 +19,32 @@
 
 static int wm8350_read_battery_uvolts(struct wm8350 *wm8350)
 {
-	return wm8350_read_auxadc(wm8350, WM8350_AUXADC_BATT, 0, 0)
-		* WM8350_AUX_COEFF;
+	int val = wm8350_read_auxadc(wm8350, WM8350_AUXADC_BATT, 0, 0);
+
+	if (val < 0)
+		return val;
+
+	return val * WM8350_AUX_COEFF;
 }
 
 static int wm8350_read_line_uvolts(struct wm8350 *wm8350)
 {
-	return wm8350_read_auxadc(wm8350, WM8350_AUXADC_LINE, 0, 0)
-		* WM8350_AUX_COEFF;
+	int val = wm8350_read_auxadc(wm8350, WM8350_AUXADC_LINE, 0, 0);
+
+	if (val < 0)
+		return val;
+
+	return val * WM8350_AUX_COEFF;
 }
 
 static int wm8350_read_usb_uvolts(struct wm8350 *wm8350)
 {
-	return wm8350_read_auxadc(wm8350, WM8350_AUXADC_USB, 0, 0)
-		* WM8350_AUX_COEFF;
+	int val = wm8350_read_auxadc(wm8350, WM8350_AUXADC_USB, 0, 0);
+
+	if (val < 0)
+		return val;
+
+	return val * WM8350_AUX_COEFF;
 }
 
 #define WM8350_BATT_SUPPLY	1
@@ -257,7 +269,10 @@ static int wm8350_ac_get_prop(struct power_supply *psy,
 				 WM8350_LINE_SUPPLY);
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = wm8350_read_line_uvolts(wm8350);
+		ret = wm8350_read_line_uvolts(wm8350);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
 		break;
 	default:
 		ret = -EINVAL;
@@ -287,7 +302,10 @@ static int wm8350_usb_get_prop(struct power_supply *psy,
 				 WM8350_USB_SUPPLY);
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = wm8350_read_usb_uvolts(wm8350);
+		ret = wm8350_read_usb_uvolts(wm8350);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
 		break;
 	default:
 		ret = -EINVAL;
@@ -308,8 +326,12 @@ static enum power_supply_property wm8350_usb_props[] = {
 static int wm8350_bat_check_health(struct wm8350 *wm8350)
 {
 	u16 reg;
+	int uvolts = wm8350_read_battery_uvolts(wm8350);
+
+	if (uvolts < 0)
+		return POWER_SUPPLY_HEALTH_UNKNOWN;
 
-	if (wm8350_read_battery_uvolts(wm8350) < 2850000)
+	if (uvolts < 2850000)
 		return POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
 
 	reg = wm8350_reg_read(wm8350, WM8350_CHARGER_OVERRIDES);
@@ -356,7 +378,10 @@ static int wm8350_bat_get_property(struct power_supply *psy,
 				 WM8350_BATT_SUPPLY);
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = wm8350_read_battery_uvolts(wm8350);
+		ret = wm8350_read_battery_uvolts(wm8350);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
 		break;
 	case POWER_SUPPLY_PROP_HEALTH:
 		val->intval = wm8350_bat_check_health(wm8350);
-- 
2.55.0

Re: [PATCH] mfd: wm8350: propagate ADC read timeout instead of returning 0

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] power_supply get_property callbacks return a positive voltage instead of 0 on success.
- [Low] The commit message description starts with a lowercase letter, violating MFD subsystem guidelines.

Pre-existing issues:
- [High] wm8350_reg_read() returns uninitialized stack memory on I/O error.
- [Medium] The MFD probe function fails to unwind registered devices or return an error if child registration fails.
--

--- Patch [1]: [PATCH] mfd: wm8350: propagate ADC read timeout instead of returning 0 ---
commit 85cc18ec8aa2aed410635585d8d7ebb26d16479c
Author: Ivy Lopez <skunkolee@gmail.com>

mfd: wm8350: propagate ADC read timeout instead of returning 0
    
This commit updates wm8350_read_auxadc() to return -ETIMEDOUT when the AUX
ADC read times out, instead of returning a masked value of 0. It also
modifies the hwmon and power_supply drivers to propagate this error.

> mfd: wm8350: propagate ADC read timeout instead of returning 0

[Severity: Low]
This isn't a bug, but the commit message description starts with a lowercase
letter. Should this be capitalized according to MFD subsystem guidelines?

> diff --git a/drivers/mfd/wm8350-core.c b/drivers/mfd/wm8350-core.c
> index fbc77b218215c..3120c6b8fa3fb 100644
> --- a/drivers/mfd/wm8350-core.c
> +++ b/drivers/mfd/wm8350-core.c

[ ... ]

> @@ -221,19 +222,24 @@ int wm8350_read_auxadc(struct wm8350 *wm8350, int channel, int scale, int vref)
>  	wait_for_completion_timeout(&wm8350->auxadc_done, msecs_to_jiffies(5));
>  
>  	reg = wm8350_reg_read(wm8350, WM8350_DIGITISER_CONTROL_1);

[Severity: High]
This is a pre-existing issue, but does wm8350_reg_read() return uninitialized
stack memory on I/O errors?

Looking at wm8350_reg_read():

u16 wm8350_reg_read(struct wm8350 *wm8350, int reg)
{
    unsigned int data;
    int err;

    err = regmap_read(wm8350->regmap, reg, &data);
    if (err)
        dev_err(wm8350->dev, "read from reg R%d failed\n", reg);

    return data;
}

If regmap_read() fails, the uninitialized data variable is returned. Callers
then blindly use this garbage value to write back to critical hardware
registers, which can cause power domains to randomly turn on or off.

> -	if (reg & WM8350_AUXADC_POLL)
> +	if (reg & WM8350_AUXADC_POLL) {
>  		dev_err(wm8350->dev, "adc chn %d read timeout\n", channel);
> -	else
> +		ret = -ETIMEDOUT;
> +	} else {
>  		result = wm8350_reg_read(wm8350,
> -					 WM8350_AUX1_READBACK + channel);
> +					WM8350_AUX1_READBACK + channel);
> +	}
>  
>  	/* Turn off the ADC */
>  	reg = wm8350_reg_read(wm8350, WM8350_POWER_MGMT_5);
>  	wm8350_reg_write(wm8350, WM8350_POWER_MGMT_5,
> -			 reg & ~WM8350_AUXADC_ENA);
> +			reg & ~WM8350_AUXADC_ENA);
>  
>  	mutex_unlock(&wm8350->auxadc_mutex);
>  
> +	if (ret)
> +		return ret;
> +
>  	return result & WM8350_AUXADC_DATA1_MASK;
>  }
>  EXPORT_SYMBOL_GPL(wm8350_read_auxadc);

[Severity: Medium]
This is a pre-existing issue, but does the MFD probe function fail to unwind
registered devices if a child registration fails?

Looking at wm8350_client_dev_register():

ret = platform_device_add(*pdev);
if (ret != 0) {
    dev_err(wm8350->dev, "Failed to register %s: %d\n", name, ret);
    platform_device_put(*pdev);
    *pdev = NULL;
}

This custom helper drops the error, causing wm8350_device_init() to proceed
without unwinding previously registered children. This violates the MFD
subsystem guideline that requires unwinding if a child device fails to
register.

> diff --git a/drivers/power/supply/wm8350_power.c b/drivers/power/supply/wm8350_power.c
> index b0eb6e0ce8bcf..1113730279053 100644
> --- a/drivers/power/supply/wm8350_power.c
> +++ b/drivers/power/supply/wm8350_power.c

[ ... ]

> @@ -257,7 +269,10 @@ static int wm8350_ac_get_prop(struct power_supply *psy,
>  				 WM8350_LINE_SUPPLY);
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> -		val->intval = wm8350_read_line_uvolts(wm8350);
> +		ret = wm8350_read_line_uvolts(wm8350);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;
>  		break;
>  	default:
>  		ret = -EINVAL;

[Severity: High]
Does this inadvertently return the positive voltage value on success?

By assigning the result of wm8350_read_line_uvolts() to ret in
wm8350_ac_get_prop(), the power_supply_get_property API will now return a
large positive voltage (e.g., 5000000) instead of 0 on success.

Many callers within the kernel check if (power_supply_get_property(...))
and will incorrectly treat the positive return value as a failure, leading
to broken functionality.

[ ... ]

> @@ -287,7 +302,10 @@ static int wm8350_usb_get_prop(struct power_supply *psy,
>  				 WM8350_USB_SUPPLY);
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> -		val->intval = wm8350_read_usb_uvolts(wm8350);
> +		ret = wm8350_read_usb_uvolts(wm8350);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;
>  		break;

[Severity: High]
Could this also leak the positive voltage into the return value of
wm8350_usb_get_prop() instead of returning 0 on success?

[ ... ]

> @@ -356,7 +378,10 @@ static int wm8350_bat_get_property(struct power_supply *psy,
>  				 WM8350_BATT_SUPPLY);
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> -		val->intval = wm8350_read_battery_uvolts(wm8350);
> +		ret = wm8350_read_battery_uvolts(wm8350);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;
>  		break;

[Severity: High]
Is the same regression present here in wm8350_bat_get_property(), where the
function will return the positive battery voltage rather than 0 on success?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805014221.16412-1-skunkolee@gmail.com?part=1

Re: [PATCH] mfd: wm8350: propagate ADC read timeout instead of returning 0

[Guenter Roeck]

On 8/4/26 18:42, Ivy Lopez wrote:
> wm8350_read_auxadc() logs an error via dev_err() when the AUX ADC
> read times out, but then falls through to return the masked value
> of `result`, which was left at its initial value of 0. Callers have
> no way to distinguish a genuine 0V reading from a failed read.
> 
> Return -ETIMEDOUT on timeout instead, and update the wm8350 hwmon
> and power_supply drivers, which call this function directly, to
> check for a negative return and propagate the error rather than
> using it as a voltage value.
> 

I am not sure if this warrants fixing in a 17+ year old driver,
unless it is observed to be a real problem. It appears to me that the
error handling was likely not implemented on purpose.

On top of that, as is seen a lot lately, this fixes a rare issue
that is likely never going to be seen in the real world while not
addressing the many real problems in the driver (as reported by Sashiko).

Guenter

Re: [PATCH] mfd: wm8350: propagate ADC read timeout instead of returning 0

[Ivy Lopez]

On 8/5/26, Guenter Roeck wrote:
> I am not sure if this warrants fixing in a 17+ year old driver,
> unless it is observed to be a real problem. It appears to me that the
> error handling was likely not implemented on purpose.
>
> On top of that, as is seen a lot lately, this fixes a rare issue
> that is likely never going to be seen in the real world while not
> addressing the many real problems in the driver (as reported by Sashiko).

That's fair, sure. The automated review also caught a real regression
in this patch itself: reusing `ret` for the voltage value in the
power_supply get_property callbacks means a successful read returns
a nonzero value instead of 0, which breaks the normal
if (power_supply_get_property(...)) error-checking convention for
callers which is on my end.. I'll drop this patch rather than resubmit a v2 for an issue
you've said isn't worth fixing here.

Thanks,
ivy