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

[sashiko-bot@kernel.org]

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