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

PATCH In review Ivy Lopez 3 participants 4 messages Aug 5 – Aug 6

Subsystems (4)

Hardware monitoring Multifunction devices (MFD) Wolfson microelectronics drivers Power supply

lore

Share & tools

lore

Ivy Lopez

author

2026-08-05 01:42 UTC #0 lore raw

PATCH 1/1 mfd: wm8350: propagate ADC read timeout instead of returning 0

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 <[email protected]>

---
 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(-)

drivers/hwmon/wm8350-hwmon.c +6 -3

index 10e17bb79895..a455e1c5476c 100644
@@ -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);
 }

drivers/mfd/wm8350-core.c +10 -4

index fbc77b218215..3120c6b8fa3f 100644
@@ -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);

drivers/power/supply/wm8350_power.c +35 ~~-10~~

84 lines not on this page · raw message

Signature · 1 line

2.55.0

[email protected]

AI review

2026-08-05 01:54 UTC #1 lore raw

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 <[email protected]>

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.

Quoted 1 line

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?

Quoted 4 lines

drivers/mfd/wm8350-core.c +0 -0
index fbc77b218215c..3120c6b8fa3fb 100644

[ ... ]

drivers/mfd/wm8350-core.c int wm8350_read_auxadc(struct wm8350 *wm8350, int channel, int scale, int vref) · line 224

Quoted 4 lines

drivers/mfd/wm8350-core.c +0 -0

@@ -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.

Quoted 15 lines

drivers/mfd/wm8350-core.c +5 -4

-	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);

drivers/mfd/wm8350-core.c int wm8350_read_auxadc(struct wm8350 *wm8350, int channel, int scale, int vref) · line 245

Quoted 10 lines

drivers/mfd/wm8350-core.c +4 -0

+			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.

Quoted 4 lines

drivers/power/supply/wm8350_power.c +0 -0
index b0eb6e0ce8bcf..1113730279053 100644

[ ... ]

drivers/power/supply/wm8350_power.c static int wm8350_ac_get_prop(struct power_supply *psy, · line 278

Quoted 12 lines

drivers/power/supply/wm8350_power.c +4 -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;

[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.

[ ... ]

drivers/power/supply/wm8350_power.c static int wm8350_usb_get_prop(struct power_supply *psy, · line 309

Quoted 10 lines

drivers/power/supply/wm8350_power.c +4 -1

@@ -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?

[ ... ]

drivers/power/supply/wm8350_power.c static int wm8350_bat_get_property(struct power_supply *psy, · line 385

Quoted 10 lines

drivers/power/supply/wm8350_power.c +4 -1

@@ -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?

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Guenter Roeck

maintainer

2026-08-05 03:00 UTC #2 lore raw

In reply to Ivy Lopez

Quoted 10 lines · 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.

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

Ivy Lopez

author

2026-08-06 01:20 UTC #3 lore raw

Quoted 7 lines · Guenter Roeck

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