Re: [PATCH v3 1/3] iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules

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

New issues:
- [High] Renaming the user-visible Kconfig symbol from `MCP47FEB02` to `MCP47FEB02_I2C` breaks existing kernel configurations.

Pre-existing issues:
- [High] Failure to initialize `dac_data` during probe causes `store_eeprom` and system suspend to permanently clobber the DAC hardware output state.
- [High] System suspend and resume clobbers the user-configured channel powerdown state.
- [High] Incorrect bitwise shift in hardware gain parsing causes it to always evaluate to 0.
- [High] The sysfs store function `store_eeprom_store` returns 0 when given a false value, causing an infinite loop in userspace.
- [High] The driver unconditionally fails to probe if a channel node in the Device Tree lacks the optional `label` property.
- [High] Incorrect logic when parsing the hardware powerdown mode desynchronizes the software state from the hardware.
--

--- Patch [1]: [PATCH v3 1/3] iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules ---
commit 899388ec468a06591c301218099420c047b4404b
Author: Ariana Lazar <[email protected]>
Subject: iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules

This commit prepares the driver for bus-specific code by refactoring the
MCP47FEB02 driver into separate files. It splits the driver into
mcp47feb02-core.c and mcp47feb02-i2c.c to support the upcoming SPI
implementation.

> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 657c68e75542e..cc10d43ffd1e8 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -550,8 +550,13 @@ config MCP4728
>  	  will be called mcp4728.
>  
>  config MCP47FEB02
> -	tristate "MCP47F(E/V)B01/02/04/08/11/12/14/18/21/22/24/28 DAC driver"
> +	tristate

[Severity: High]
Will removing the prompt from MCP47FEB02 break existing kernel configurations?

When users upgrade their kernel and run make oldconfig, their previous
selection for this symbol will be discarded because it no longer has a prompt,
and the new MCP47FEB02_I2C will default to n. This could silently remove the
DAC driver from existing configurations. The original symbol should probably
retain the prompt to preserve backward compatibility.

[ ... ]
> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02-core.c
> similarity index 72%
> rename from drivers/iio/dac/mcp47feb02.c
> rename to drivers/iio/dac/mcp47feb02-core.c
[ ... ]
> @@ -471,6 +264,8 @@ static ssize_t store_eeprom_store(struct device *dev, struct device_attribute *a
>  	if (!state)
>  		return 0;

[Severity: High]
This is a pre-existing issue, but does returning 0 here trap userspace in an
infinite loop?

When a sysfs store function returns 0, it signals to the VFS that 0 bytes were
consumed. This causes standard userspace tools like echo to continuously retry
the write in a tight loop, which could lead to a local CPU denial of service.

[Severity: High]
This is also a pre-existing issue, but does this function corrupt the DAC
hardware output state? 

During driver probe in mcp47feb02_init_ctrl_regs(), the hardware DAC output
registers are not read into the driver's software cache, leaving 
data->chdata[i].dac_data as 0. If a user writes to store_eeprom (or if the
system suspends), this function will unconditionally write this uninitialized 0
to both the EEPROM and volatile DAC registers, permanently overwriting
legitimate defaults.

>  
> +	guard(mutex)(&data->lock);
> +
>  	/*
>  	 * Verify DAC Wiper and DAC Configuration are unlocked. If both are disabled,
>  	 * writing to EEPROM is available.

[ ... ]
> @@ -577,18 +372,17 @@ static int mcp47feb02_resume(struct device *dev)
>  	guard(mutex)(&data->lock);
>  
>  	for_each_set_bit(ch, &data->active_channels_mask, data->phys_channels) {
> -		u8 pd_mode;
>  		int ret;
>  
>  		data->chdata[ch].powerdown = false;

[Severity: High]
This is a pre-existing issue, but does system resume clobber the user-configured
powerdown state?

By forcing powerdown to false here and writing normal operation mode to the
hardware, it seems to forcefully wake up all channels. This would silently
override the user's previously configured sysfs powerdown state for channels
that were meant to remain off.

>  
>  		ret = regmap_write(data->regmap, REG_ADDR(ch), data->chdata[ch].dac_data);

[ ... ]
> @@ -1022,7 +816,7 @@ static int mcp47feb02_init_ctrl_regs(struct mcp47feb02_data *data)
>  		struct device *dev = regmap_get_device(data->regmap);
>  		unsigned int pd_tmp;
>  
> -		data->chdata[i].ref_mode = (vref_ch >> (2 * i)) & MCP47FEB02_DAC_CTRL_MASK;
> +		data->chdata[i].ref_mode = (vref_ch >> (2 * i)) & MCP47FEB02_CMD_MASK;
>  		data->chdata[i].use_2x_gain = (gain_ch >> i)  & MCP47FEB02_GAIN_BIT_MASK;

[Severity: High]
This is a pre-existing issue, but is the bitwise shift for use_2x_gain
evaluating the correct bits?

Earlier in this function, gain_ch is masked to clear the lower bits, but this
evaluates it by shifting by i (0 to 7) instead of 8 + i. Since the LSB is
evaluated from the explicitly cleared lower byte, won't this cause use_2x_gain
to always incorrectly initialize to 0 for all channels?

[ ... ]
> @@ -1066,7 +860,7 @@ static int mcp47feb02_init_ctrl_regs(struct mcp47feb02_data *data)
>  			break;
>  		}
>  
> -		pd_tmp = (pd_ch >> (2 * i)) & MCP47FEB02_DAC_CTRL_MASK;
> +		pd_tmp = (pd_ch >> (2 * i)) & MCP47FEB02_CMD_MASK;
>  		data->chdata[i].powerdown_mode = pd_tmp ? (pd_tmp - 1) : pd_tmp;
>  		data->chdata[i].powerdown = !!(data->chdata[i].powerdown_mode);

[Severity: High]
This is a pre-existing issue, but does this boolean cast incorrectly assume
the device is not in powerdown?

When parsing the hardware state, if the DAC is in 1kOhm powerdown mode (where
pd_tmp is 1), powerdown_mode computes to 0. The driver then sets powerdown to
!!0, evaluating to false. Won't this incorrectly initialize the channel's state
as powered on when it is actually powered down?

>  	}

[ ... ]
> @@ -1091,10 +885,10 @@ static int mcp47feb02_init_ch_scales(struct mcp47feb02_data *data, int vdd_uV,
>  	return 0;
>  }
>  
> -static int mcp47feb02_probe(struct i2c_client *client)
> +int mcp47feb02_common_probe(const struct mcp47feb02_features *chip_features,
> +			    struct regmap *regmap)
>  {

[Severity: High]
This is a pre-existing issue, but does the driver unconditionally fail to probe
if a channel node in the device tree lacks a label property?

In the probe call chain:

mcp47feb02_common_probe()
  mcp47feb02_parse_fw()
    fwnode_property_read_string(child, "label", ...)

The driver queries the label property and aborts probe if it is missing. Since
label is canonically an optional property in IIO device tree bindings, enforcing
its presence breaks compatibility and prevents the driver from loading on valid
device trees.

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