On 09/06/26 10:37, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
> - [Medium] ABI Violation: Exposing a writable sysfs attribute to fix a static
> hardware configuration.
> - [Medium] State Inconsistency: `st->double_scale` is not rolled back if
> hardware synchronization fails.
> - [Low] Data Race: Lockless read of `st->double_scale` can race with
> concurrent writes.
> --
>
> commit 7fefd2fd08838a6ee5a81a3a819607e02af9d85d
> Author: Rodrigo Alencar <[email protected]>
>
> iio: dac: ad5686: add gain control support
>
> This commit adds support for a 2x gain multiplier applied to the output
> voltage. This provides a gain control through a bit field in the control
> register or a dedicated GAIN pin, exposing the scale property as configurable.
>
> > diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> > index d9022c6a32016..d49390af71f90 100644
> > --- a/drivers/iio/dac/ad5686.c
> > +++ b/drivers/iio/dac/ad5686.c
>
> [ ... ]
>
> > @@ -192,9 +196,14 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
> > GENMASK(chan->scan_type.realbits - 1, 0);
> > return IIO_VAL_INT;
> > case IIO_CHAN_INFO_SCALE:
> > - *val = st->vref_mv;
> > - *val2 = chan->scan_type.realbits;
> > - return IIO_VAL_FRACTIONAL_LOG2;
> > + if (st->double_scale) {
> > + *val = st->scale_avail[2];
> > + *val2 = st->scale_avail[3];
> > + } else {
> > + *val = st->scale_avail[0];
> > + *val2 = st->scale_avail[1];
> > + }
> > + return IIO_VAL_INT_PLUS_NANO;
>
> [Severity: Low]
> Is it possible for a data race to occur here?
>
> In ad5686_read_raw(), the mutable software state st->double_scale is read
> to determine the returned scale values without holding the st->lock mutex.
>
> Because st->double_scale can be concurrently modified by ad5686_write_raw()
> (which properly acquires st->lock), this can lead to reading a prematurely
> updated state before the hardware configuration has actually completed.
Valid concern, will add guarded mutex lock in read_raw to the previous patch.
> > }
> > return -EINVAL;
> > }
>
> [ ... ]
>
> > @@ -216,6 +225,63 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
> >
> > return ad5686_write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
> > chan->address, val <<
> > chan->scan_type.shift);
> > + case IIO_CHAN_INFO_SCALE:
> > + if (val == st->scale_avail[0] && val2 == st->scale_avail[1])
> > + st->double_scale = false;
> > + else if (val == st->scale_avail[2] && val2 ==
> > st->scale_avail[3])
> > + st->double_scale = true;
> > + else
> > + return -EINVAL;
> > +
> > + switch (st->chip_info->regmap_type) {
> > + case AD5310_REGMAP:
> > + return ad5310_control_sync(st);
> > + case AD5683_REGMAP:
> > + return ad5683_control_sync(st);
>
> [Severity: Medium]
> Should st->double_scale be restored to its previous value if the hardware
> synchronization fails?
>
> In ad5686_write_raw(), st->double_scale is updated before the driver attempts
> to push the change to the DAC via ad5310_control_sync(st) or
> ad5683_control_sync(st).
>
> If the underlying SPI/I2C write fails, the function immediately returns the
> error code without rolling back st->double_scale.
>
> This could cause the driver's cached state to diverge from the actual
> hardware configuration, causing future reads of the scale to return the
> incorrect updated value.
Will ignore that and assume that SPI write error is a platform problem.
At another attempt the cached value can take effect.
>
> > + case AD5686_REGMAP:
> > + /*
> > + * Even if the gain pin is hardwired on the board, the
> > + * user is able to control the scale such that it
> > + * matches the actual gain setting.
> > + */
> > + gpiod_set_value_cansleep(st->gain_gpio,
> > + st->double_scale ? 1 : 0);
> > + return 0;
>
> [Severity: Medium]
> Does this expose a writable sysfs attribute to fix a static hardware
> configuration?
>
> The commit introduces a writable scale sysfs attribute to allow users to
> manually
> match the software scale to a hardwired gain pin. Static hardware properties
> should typically be described via firmware properties (e.g., Device Tree) so
> the driver can expose the correct, immutable scale at initialization.
>
> If the GAIN GPIO is absent, writing to the scale attribute returns success but
> does not change the physical hardware state, leading to a silent
> desynchronization
> between the reported scale and the actual physical DAC output.
>
> Could this mislead generic IIO clients by presenting multiple available scales
> that they believe they can dynamically toggle?
No sure if this is a big concern (assuming that the user has knowledge of the
hardware).
An option is to add another dt property to indicate that the gain is doubled in
hardware.
Such property cannot be defined alongside the ldac-gpios property.
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=12
--
Kind regards,
Rodrigo Alencar
