Re: [PATCH v4 3/6] iio: dac: ad5504: remove legacy platform data support

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

New issues:
- [High] The driver deletes legacy platform data for configuring the output range but fails to implement Device Tree parsing to replace it, leading to incorrect IIO scale reporting.
- [High] Silent failure to probe if the optional 'vcc-supply' regulator is omitted from the device tree.

Pre-existing issues:
- [High] Missing mutex lock protecting shared SPI transfer buffers and device state during concurrent sysfs accesses.
--

--- Patch [3]: [PATCH v4 3/6] iio: dac: ad5504: remove legacy platform data support ---
commit e23f620a135f8935a12128aed6888332ecec66f8
Author: Taha Ed-Dafili <[email protected]>
iio: dac: ad5504: remove legacy platform data support

This commit removes legacy platform data support from the AD5504 driver,
deleting the ad5504_platform_data structure and the fallback configuration
logic in the probe function.

> diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
> index 1793af0781dc7..79db277b776e2 100644
> --- a/drivers/iio/dac/ad5504.c
> +++ b/drivers/iio/dac/ad5504.c
[ ... ]
> @@ -287,16 +285,10 @@ static int ad5504_probe(struct spi_device *spi)
>  	st = iio_priv(indio_dev);
>  
>  	ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
> -	if (ret < 0 && ret != -ENODEV)
> +	if (ret < 0)
>  		return ret;

[Severity: High]
Will this result in a silent probe failure if the optional vcc-supply
regulator is omitted from the device tree?

Since devm_regulator_get_enable_read_voltage() internally uses
devm_regulator_get_optional(), it returns -ENODEV if the regulator is
absent.

Propagating this error directly without logging causes the driver core to
treat it as a silent match rejection, whereas before it gracefully continued.

> -	if (ret == -ENODEV) {
> -		if (pdata->vref_mv)
> -			st->vref_mv = pdata->vref_mv;
> -		else
> -			dev_warn(dev, "reference voltage unspecified\n");
> -	} else {
> -		st->vref_mv = ret / 1000;
> -	}
> +
> +	st->vref_mv = ret / 1000;

[Severity: High]
Does this missing implementation of device tree parsing lead to incorrect IIO
scale reporting?

The code currently hardcodes the reference voltage to the vcc supply
voltage without providing a fallback to parse the newly added
output-range-microvolt property.

If a system has a 60V VCC supply but the R_SEL pin is configured for a
30V output range, the device will now incorrectly report a 60V scale to
userspace since the fallback logic was removed.

[ ... ]

[Severity: High]
This is a pre-existing issue, but is it possible for concurrent sysfs
accesses to corrupt memory or disrupt hardware state?

Looking at ad5504_spi_write() and ad5504_spi_read(), the shared spi buffers
are accessed without locking:

drivers/iio/dac/ad5504.c:ad5504_spi_write() {
    ...
    st->data[0] = cpu_to_be16(AD5504_CMD_WRITE | AD5504_ADDR(addr) |
                              (val & AD5504_RES_MASK));

    return spi_write(st->spi, &st->data[0], 2);
}

Additionally, in ad5504_write_dac_powerdown(), multiple SPI writes are
performed and st->pwr_down_mask is modified without serialization:

drivers/iio/dac/ad5504.c:ad5504_write_dac_powerdown() {
    ...
    if (pwr_down)
        st->pwr_down_mask &= ~(1 << chan->channel);
    else
        st->pwr_down_mask |= (1 << chan->channel);
    
    ret = ad5504_spi_write(st, AD5504_ADDR_CTRL,
                           AD5504_DAC_PWRDWN_MODE(st->pwr_down_mode) |
                           AD5504_DAC_PWR(st->pwr_down_mask));
    
    /* writes to the CTRL register must be followed by a NOOP */
    ad5504_spi_write(st, AD5504_ADDR_NOOP, 0);
    ...
}

Could concurrent reads or writes by userspace processes to sysfs attributes
interleave SPI commands or corrupt the DMA buffer in st->data?

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