Re: [PATCH v4 2/6] iio: dac: ad5504: Align headers with IWYU principle

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

New issues:
- [Low] API Contract Violation (IWYU) and commit message mismatch: improper removal of `<linux/device.h>`.

Pre-existing issues:
- [High] Potential NULL pointer dereference of `pdata` during probe when the `vcc` regulator is missing.
- [High] Missing locking leads to race conditions and SPI DMA buffer corruption across concurrent sysfs accesses.
--

--- Patch [2]: [PATCH v4 2/6] iio: dac: ad5504: Align headers with IWYU principle ---
commit 90816be5685af9f86bca164af4343c7bef759b65
Author: Taha Ed-Dafili <[email protected]>

iio: dac: ad5504: Align headers with IWYU principle

This commit updates header inclusions in the ad5504 driver to align with
the Include What You Use (IWYU) principle. It removes several unused
headers, replaces others with more specific ones, and sorts them
alphabetically.

>     - Add <linux/array_size.h> for ARRAY_SIZE().
>     - Add <linux/kstrtox.h> for string to integer conversions.
>     - Replaced <linux/device.h> with <linux/dev_printk.h>

[Severity: Low]
This isn't a bug, but this description might be inaccurate since
<linux/device.h> is still required for dev_get_platdata().

> 
>     Signed-off-by: Taha Ed-Dafili <[email protected]>
> 
> ---
>  drivers/iio/dac/ad5504.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
> index 5e586185d8579..1793af0781dc7 100644
> --- a/drivers/iio/dac/ad5504.c
> +++ b/drivers/iio/dac/ad5504.c
> @@ -5,22 +5,26 @@
>   * Copyright 2011 Analog Devices Inc.
>   */
>  
> -#include <linux/bitops.h>
> -#include <linux/device.h>

[Severity: Low]
Does this removal violate the IWYU principle? The file directly calls
dev_get_platdata() and uses struct device in ad5504_probe(), which both
rely on <linux/device.h> being included.

[ ... ]

[Severity: High]
This is a pre-existing issue, but could this oops if the device is
instantiated via DeviceTree without a "vcc" regulator?

If dev_get_platdata() returns NULL and the regulator is missing,
devm_regulator_get_enable_read_voltage() returns -ENODEV, leading to:

drivers/iio/dac/ad5504.c:ad5504_probe() {
...
	if (ret == -ENODEV) {
		if (pdata->vref_mv)
			st->vref_mv = pdata->vref_mv;
...
}

Should we verify pdata is not NULL before dereferencing it here?

[Severity: High]
This is a pre-existing issue, but could concurrent sysfs accesses corrupt
the SPI DMA buffer or the powerdown mask?

If two threads concurrently access sysfs endpoints (like
out_voltageX_powerdown or in_voltageX_raw), they can enter
ad5504_spi_write() without any locking. Since st->data is shared among
all SPI transactions to satisfy DMA alignment, could concurrent writes
overwrite st->data and corrupt the SPI payload?

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

Similarly, st->pwr_down_mask appears to be subject to an unprotected
read-modify-write:

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

Does this state need a mutex to prevent race conditions during concurrent
accesses?

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