| Message ID | 20190930204451.28614-1-yzhai003@ucr.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: trigger: stm32-timer: fix the usage of uninitialized variables | expand |
On Mon, 30 Sep 2019 13:44:49 -0700 Yizhuo <yzhai003@ucr.edu> 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@ucr.edu> 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; > }
On 10/1/19 10:43 AM, Jonathan Cameron wrote: > On Mon, 30 Sep 2019 13:44:49 -0700 > Yizhuo <yzhai003@ucr.edu> 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@ucr.edu> > 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 Hi Yizhuo, The only reason that could lead a regmap_read to failed is an issue on the clock but is this case we will be blocked in regmap_read() itself so the error will never be displayed. For me testing all regmap_read() return values only add code and not bring additional informations so I NACK this patch. Benjamin >> --- >> 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; >> }
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; }
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@ucr.edu> --- drivers/iio/trigger/stm32-timer-trigger.c | 98 ++++++++++++++++++++--- 1 file changed, 85 insertions(+), 13 deletions(-)