On Mon, 30 Sep 2019 13:44:49 -0700 Yizhuo <yzhai003@xxxxxxx> wrote: > Several functions in this file are trying to use regmap_read() to > initialize the specific variable, however, if regmap_read() fails, > the variable could be uninitialized but used directly, which is > potentially unsafe. The return value of regmap_read() should be > checked and handled. This patch fixes most of the uninitialized > variables, but those in function stm32_tt_read_frequency() are > hard to handle and need extra effot. > > Signed-off-by: Yizhuo <yzhai003@xxxxxxx> Hi Yizhuo, This is a slightly interesting case. >From an 'obviously' correct point of view, it makes sense to always handle the errors and avoid the potentially uninitialised cases. >From the point of view of a 'fix' we need to show if these can actually happen. These calls are regmap-mmio so the error path is in clock enable / disable if there is a clock provided. Looking at mfd/stm-timers.c there is a clock called "int" provided. Now it's possible that clock enable can never fail, but that is getting too hard to chase down. As such, I think this fix is worthwhile, but I'm not going to mark it for stable without a report of it actually being possible to trigger it. I'll leave this on the list for a little longer though as I'd definitely like an ack from Benjamin or someone else more familiar with the driver than I am. Thanks, Jonathan > --- > drivers/iio/trigger/stm32-timer-trigger.c | 98 ++++++++++++++++++++--- > 1 file changed, 85 insertions(+), 13 deletions(-) > > diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c > index a5dfe65cd9b9..f8ea7bcbb739 100644 > --- a/drivers/iio/trigger/stm32-timer-trigger.c > +++ b/drivers/iio/trigger/stm32-timer-trigger.c > @@ -107,6 +107,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv, > unsigned long long prd, div; > int prescaler = 0; > u32 ccer, cr1; > + int ret; > > /* Period and prescaler values depends of clock rate */ > div = (unsigned long long)clk_get_rate(priv->clk); > @@ -132,11 +133,21 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv, > } > > /* Check if nobody else use the timer */ > - regmap_read(priv->regmap, TIM_CCER, &ccer); > + ret = regmap_read(priv->regmap, TIM_CCER, &ccer); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_CCER.\n"); > + return ret; > + } > + > if (ccer & TIM_CCER_CCXE) > return -EBUSY; > > - regmap_read(priv->regmap, TIM_CR1, &cr1); > + ret = regmap_read(priv->regmap, TIM_CR1, &cr1); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_CR1.\n"); > + return ret; > + } > + > if (!(cr1 & TIM_CR1_CEN)) > clk_enable(priv->clk); > > @@ -164,12 +175,23 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv, > static void stm32_timer_stop(struct stm32_timer_trigger *priv) > { > u32 ccer, cr1; > + int ret; > + > + ret = regmap_read(priv->regmap, TIM_CCER, &ccer); > + if (ret) { > + dev_err(priv->dev, "Fail to read TIM_CCER.\n"); > + return; > + } > > - regmap_read(priv->regmap, TIM_CCER, &ccer); > if (ccer & TIM_CCER_CCXE) > return; > > - regmap_read(priv->regmap, TIM_CR1, &cr1); > + ret = regmap_read(priv->regmap, TIM_CR1, &cr1); > + if (ret) { > + dev_err(priv->dev, "Fail to read TIM_CR1.\n"); > + return; > + } > + > if (cr1 & TIM_CR1_CEN) > clk_disable(priv->clk); > > @@ -403,20 +425,36 @@ static int stm32_counter_read_raw(struct iio_dev *indio_dev, > { > struct stm32_timer_trigger *priv = iio_priv(indio_dev); > u32 dat; > + int ret; > > switch (mask) { > case IIO_CHAN_INFO_RAW: > - regmap_read(priv->regmap, TIM_CNT, &dat); > + ret = regmap_read(priv->regmap, TIM_CNT, &dat); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_CNT.\n"); > + return ret; > + } > + > *val = dat; > return IIO_VAL_INT; > > case IIO_CHAN_INFO_ENABLE: > - regmap_read(priv->regmap, TIM_CR1, &dat); > + ret = regmap_read(priv->regmap, TIM_CR1, &dat); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_CR1.\n"); > + return ret; > + } > + > *val = (dat & TIM_CR1_CEN) ? 1 : 0; > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SCALE: > - regmap_read(priv->regmap, TIM_SMCR, &dat); > + ret = regmap_read(priv->regmap, TIM_SMCR, &dat); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_SMCR.\n"); > + return ret; > + } > + > dat &= TIM_SMCR_SMS; > > *val = 1; > @@ -438,6 +476,7 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, > { > struct stm32_timer_trigger *priv = iio_priv(indio_dev); > u32 dat; > + int ret; > > switch (mask) { > case IIO_CHAN_INFO_RAW: > @@ -449,13 +488,23 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev, > > case IIO_CHAN_INFO_ENABLE: > if (val) { > - regmap_read(priv->regmap, TIM_CR1, &dat); > + ret = regmap_read(priv->regmap, TIM_CR1, &dat); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_CR1.\n"); > + return ret; > + } > + > if (!(dat & TIM_CR1_CEN)) > clk_enable(priv->clk); > regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, > TIM_CR1_CEN); > } else { > - regmap_read(priv->regmap, TIM_CR1, &dat); > + ret = regmap_read(priv->regmap, TIM_CR1, &dat); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_CR1.\n"); > + return ret; > + } > + > regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, > 0); > if (dat & TIM_CR1_CEN) > @@ -517,8 +566,13 @@ static int stm32_get_trigger_mode(struct iio_dev *indio_dev, > { > struct stm32_timer_trigger *priv = iio_priv(indio_dev); > u32 smcr; > + int ret; > > - regmap_read(priv->regmap, TIM_SMCR, &smcr); > + ret = regmap_read(priv->regmap, TIM_SMCR, &smcr); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_SMCR.\n"); > + return ret; > + } > > return (smcr & TIM_SMCR_SMS) == TIM_SMCR_SMS ? 0 : -EINVAL; > } > @@ -557,6 +611,7 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev, > struct stm32_timer_trigger *priv = iio_priv(indio_dev); > int sms = stm32_enable_mode2sms(mode); > u32 val; > + int ret; > > if (sms < 0) > return sms; > @@ -565,7 +620,12 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev, > * enable counter clock, so it can use it. Keeps it in sync with CEN. > */ > if (sms == 6) { > - regmap_read(priv->regmap, TIM_CR1, &val); > + ret = regmap_read(priv->regmap, TIM_CR1, &val); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_CR1.\n"); > + return ret; > + } > + > if (!(val & TIM_CR1_CEN)) > clk_enable(priv->clk); > } > @@ -594,8 +654,14 @@ static int stm32_get_enable_mode(struct iio_dev *indio_dev, > { > struct stm32_timer_trigger *priv = iio_priv(indio_dev); > u32 smcr; > + int ret; > + > + ret = regmap_read(priv->regmap, TIM_SMCR, &smcr); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_SMCR.\n"); > + return ret; > + } > > - regmap_read(priv->regmap, TIM_SMCR, &smcr); > smcr &= TIM_SMCR_SMS; > > return stm32_sms2enable_mode(smcr); > @@ -706,13 +772,19 @@ EXPORT_SYMBOL(is_stm32_timer_trigger); > static void stm32_timer_detect_trgo2(struct stm32_timer_trigger *priv) > { > u32 val; > + int ret; > > /* > * Master mode selection 2 bits can only be written and read back when > * timer supports it. > */ > regmap_update_bits(priv->regmap, TIM_CR2, TIM_CR2_MMS2, TIM_CR2_MMS2); > - regmap_read(priv->regmap, TIM_CR2, &val); > + ret = regmap_read(priv->regmap, TIM_CR2, &val); > + if (ret) { > + dev_err(priv->dev, "fail to read TIM_CR2.\n"); > + return; > + } > + > regmap_update_bits(priv->regmap, TIM_CR2, TIM_CR2_MMS2, 0); > priv->has_trgo2 = !!val; > }