On Tue, 9 Jun 2026 12:10:02 +0100 Rodrigo Alencar <[email protected]> wrote:
> 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. I don't mind the locking being added as it makes it easier to reason about but other than risk of a write tear or other silliness, I don't think the report is valid. The upshot of this race is that the read might report a value just ahead of it being valid. Given there is no roll back on error (maybe there should be?) in write_raw() we are dealing with same visible result as the we will get with the lock. One or other of read_raw / write_raw gets the lock and that dictates what is read. Chances are for the read we aren't going to use the scale immediately so it might be stale anyway if we really have multiple threads messing around in sysfs for a device. Perhaps I'm missing something? > > > > } > > > 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. It's fine given you don't gate on the double_scale when repeating (so that answers the above question I had on whether to roll back). > > > > > > + 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. hmm. I'm not bothered by fixed values on that gpio (we solve that with binding changes on the odd occasion that actually turns up on real hardware). If you want to support that then great - there are a few examples doing that in tree. However, if the thing is optional (or that new property you mention is set) and we don't have it we should not be exposing an interface that tries to set the value of the gpio. So no gpio means: - scale_avail should be based on assumption that pin is in some default (document that ideally in dt binding). - write_raw() should return an error if a value other than that default is requested. Adjust those if the property lets us know which way the pin is wired (high or low). Jonathan > > > -- > > Sashiko AI review · > > https://sashiko.dev/#/patchset/[email protected]?part=12 > > >
