Re: [PATCH v4 3/9] iio: dac: mcp47feb02: correct false state case in store_eeprom_store

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but mcp47feb02_resume incorrectly writes the powerdown mode to the Voltage Reference (VREF) register, corrupting the channel's reference voltage setting.
- [High] This problem wasn't introduced by this patch, but mcp47feb02_suspend and mcp47feb02_resume overwrite the software powerdown state, causing all channels to be forcefully activated on resume regardless of user configuration.
- [High] This problem wasn't introduced by this patch, but mcp47feb02_init_ctrl_regs fails to identify the powerdown state when the hardware boots in '1kohm_to_gnd' mode.
- [High] This problem wasn't introduced by this patch, but store_eeprom_store performs a sequence of volatile register reads and EEPROM writes without holding the state mutex, leading to a TOCTOU race condition.
- [High] This problem wasn't introduced by this patch, but mcp47feb02_write_raw does not validate the incoming user value against bounds for IIO_CHAN_INFO_RAW.
--

--- Patch [3]: [PATCH v4 3/9] iio: dac: mcp47feb02: correct false state case in store_eeprom_store ---
commit dc6f30f8cc576f94a887f7f0e174caf833ebb683
Author: Ariana Lazar <ariana.lazar@microchip.com>

iio: dac: mcp47feb02: correct false state case in store_eeprom_store
    
Return len also when the state is false instead of 0.

> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02.c
> index bf78618ac2c89..2ee981393aa1a 100644
> --- a/drivers/iio/dac/mcp47feb02.c
> +++ b/drivers/iio/dac/mcp47feb02.c
> @@ -469,7 +469,7 @@ static ssize_t store_eeprom_store(struct device *dev, struct device_attribute *a
>  		return ret;
>  
>  	if (!state)
> -		return 0;
> +		return len;
>  
>  	/*
>  	 * Verify DAC Wiper and DAC Configuration are unlocked. If both are disabled,

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but store_eeprom_store performs
a sequence of volatile register reads and EEPROM writes without holding the
state mutex, leading to a race condition with concurrent state modifications.

Can an inconsistent combination of driver states be written to the NV EEPROM
because the data->lock is not held across the snapshot sequence?

drivers/iio/dac/mcp47feb02.c:store_eeprom_store() {
    ...
    ret = mcp47feb02_write_to_eeprom(data, NV_REG_ADDR(i),
                                     data->chdata[i].dac_data);
    ...
}

[Severity: High]
This is a pre-existing issue, but mcp47feb02_resume incorrectly writes the
powerdown mode to the Voltage Reference (VREF) register, corrupting the
channel's reference voltage setting.

During system resume, does this corrupt the voltage reference configuration
because it targets MCP47FEB02_VREF_REG_ADDR instead of the powerdown register?

drivers/iio/dac/mcp47feb02.c:mcp47feb02_resume() {
    ...
    ret = regmap_update_bits(data->regmap, MCP47FEB02_VREF_REG_ADDR,
                             DAC_CTRL_MASK(ch), DAC_CTRL_VAL(ch, pd_mode));
    ...
}

[Severity: High]
This is a pre-existing issue, but mcp47feb02_suspend and mcp47feb02_resume
overwrite the software powerdown state, causing all channels to be forcefully
activated on resume regardless of user configuration.

Does this unconditionally wake the channel and destroy the user's previously
configured powerdown preference?

drivers/iio/dac/mcp47feb02.c:mcp47feb02_suspend() {
    ...
    data->chdata[ch].powerdown = true;
    ...
}

drivers/iio/dac/mcp47feb02.c:mcp47feb02_resume() {
    ...
    data->chdata[ch].powerdown = false;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but mcp47feb02_init_ctrl_regs
fails to identify the powerdown state when the hardware boots in 1kohm_to_gnd
mode.

When pd_tmp is 1, powerdown_mode becomes 0. Does this cause the powerdown
check to evaluate to false even though the hardware is in a powerdown state?

drivers/iio/dac/mcp47feb02.c:mcp47feb02_init_ctrl_regs() {
    ...
    data->chdata[i].powerdown = !!(data->chdata[i].powerdown_mode);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but mcp47feb02_write_raw does not
validate the incoming user value against bounds for IIO_CHAN_INFO_RAW.

Can out-of-bounds values be written directly to the DAC data registers since
the user-supplied value is not validated?

drivers/iio/dac/mcp47feb02.c:mcp47feb02_write_raw() {
    ...
    ret = regmap_write(data->regmap, REG_ADDR(ch->address), val);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-mcp47feb02_refactor-v4-0-beb2aa3bfd09@microchip.com?part=3