On 26/05/17 07:20PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s)
> to consider:
> - [High] Incorrect usage of `device_property_read_u32_array` causes
> single-value property `output-range-microamp` to be ignored.
> - [High] VCO_SEL field is incorrectly set to an out-of-bounds value (7) when
> the PLL is bypassed.
> - [High] Stack out-of-bounds read in `ad9910_profile_set()` triggered by
> unvalidated GPIO descriptor count.
> - [High] Missing resource teardown: device reset line is never asserted on
> driver removal.
> - [Medium] PLL multiplier clamping logic violates the hardware's minimum VCO
> operating frequency.
> - [Medium] Missing hardware initialization delay after reset deassertion can
> cause SPI communications to fail.
> --
>
> commit fb6e130c7d866653f35ab965acf927500a84756c
> Author: Rodrigo Alencar <[email protected]>
>
> iio: frequency: ad9910: initial driver implementation
>
> Add the core AD9910 DDS driver infrastructure with single tone mode
> support. This includes SPI register access, profile management via GPIO
> pins, PLL/DAC configuration from firmware properties, and single tone
> frequency/phase/amplitude control through IIO attributes.
>
> > diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> > new file mode 100644
> > index 0000000000000..c7b1e474c92df
> > --- /dev/null
> > +++ b/drivers/iio/frequency/ad9910.c
> [ ... ]
> > +static int ad9910_set_sysclk_freq(struct ad9910_state *st, u32 freq_hz,
> > + bool update)
> > +{
> [ ... ]
> > + tmp32 = clamp(tmp32, DIV_ROUND_UP(AD9910_PLL_OUT_MIN_FREQ_HZ,
> > refclk_freq_hz),
> > + AD9910_PLL_OUT_MAX_FREQ_HZ / refclk_freq_hz);
> > + tmp32 = clamp(tmp32, AD9910_PLL_MIN_N, AD9910_PLL_MAX_N);
> > + sysclk_freq_hz = refclk_freq_hz * tmp32;
>
> Is it possible for this second clamp to drop the multiplier below the
> minimum frequency constraint? If the input reference clock is 3.2 MHz,
> the first clamp would raise the multiplier to 132 to reach 420 MHz, but
> the second clamp limits it to 127. This results in a VCO frequency of
> 406.4 MHz, violating the hardware minimum. Should the driver return
> -ERANGE instead if the bounds cannot be satisfied?
I suppose that would not be problematic as the PLL does accept that input
frequency. Even though the VCO might not be designed for frequencies as low as
406.4 MHz, we are doing the best effort here to get a proper configuration.
> [ ... ]
> > + } else {
> > + tmp32 = DIV_ROUND_CLOSEST(refclk_freq_hz, freq_hz);
> > + tmp32 = clamp(tmp32, 1U, 2U);
> > + sysclk_freq_hz = refclk_freq_hz / tmp32;
> > + tmp32 = AD9910_CFR3_VCO_SEL_MSK |
> > + FIELD_PREP(AD9910_CFR3_REFCLK_DIV_BYPASS_MSK, tmp32 %
> > 2);
> > + ret = ad9910_reg32_update(st, AD9910_REG_CFR3,
> > + AD9910_CFR3_VCO_SEL_MSK |
> > + AD9910_CFR3_REFCLK_DIV_BYPASS_MSK,
> > + tmp32, update);
>
> Does setting tmp32 with AD9910_CFR3_VCO_SEL_MSK cause the update function
> to write an out-of-bounds value here? Because tmp32 contains the entire
> mask, the underlying field will be set to 7, but the valid range for the
> vco sel field is 0 to 5.
No, 6 and 7 means PLL bypassed (Table 8 in the datasheet). Also, 7 is the
default value of that bitfield.
> [ ... ]
> > +static int ad9910_profile_set(struct ad9910_state *st, u8 profile)
> > +{
> > + DECLARE_BITMAP(values, BITS_PER_TYPE(profile));
> > +
> > + st->profile = profile;
> > + values[0] = profile;
> > + gpiod_multi_set_value_cansleep(st->gpio_profile, values);
> > +
> > + return 0;
> > +}
>
> Could this cause an out-of-bounds read on the stack? The values bitmap is
> sized using BITS_PER_TYPE(profile), which allocates space for 1 unsigned
> long. If the device tree provides more profile gpios than fit in an unsigned
> long, gpiod_multi_set_value_cansleep() might read past the end of the bitmap
> when iterating over the descriptors.
well, profile-gpios are at most 3 pins. I'll see to constraint that in the
probe function.
> [ ... ]
> > +static int ad9910_parse_fw(struct ad9910_state *st)
> > +{
> [ ... ]
> > + tmp[1] = AD9910_DAC_IOUT_DEFAULT_uA;
> > + device_property_read_u32_array(dev, "output-range-microamp", tmp,
> > + ARRAY_SIZE(tmp));
>
> Will this properly read a single-value property? Because ARRAY_SIZE(tmp)
> is passed as the expected element count, device_property_read_u32_array()
> might return -EOVERFLOW and leave the tmp array unmodified if only one
> value is present in the device tree, causing the property to be silently
> ignored.
Ignoring those misconfigurations might be ok too. One can refer to the
bindings to have a proper reference.
>
> [ ... ]
> > +static int ad9910_setup(struct device *dev, struct ad9910_state *st,
> > + struct reset_control *dev_rst)
> > +{
> > + int ret;
> > +
> > + ret = reset_control_deassert(dev_rst);
> > + if (ret)
> > + return ret;
> > +
>
> Is a teardown action missing for the device reset here? Without adding a
> managed action to assert the reset line, the hardware might remain active
> after the driver is unloaded or if a subsequent setup step fails.
I'd say it can't remain active with the power down pin being handled already.
> Might this also require a hardware delay before the first spi write? The
> datasheet specifies waiting for the internal state machine to become ready
> after a low-to-high transition on the reset line. On fast hosts, the
> immediate spi access might be ignored.
Could not find that info in the datasheet. What is says is that the reset
clears memory elements, e.g. registers are set to default values.
> > + ret = ad9910_reg32_write(st, AD9910_REG_CFR1,
> > + (st->spi->mode & SPI_3WIRE ? 0 :
> > + AD9910_CFR1_SDIO_INPUT_ONLY_MSK), false);
> > + if (ret)
> > + return ret;
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=6
--
Kind regards,
Rodrigo Alencar
