| Message ID | 1614696235-24088-1-git-send-email-fabrice.gasnier@foss.st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | counter: stm32-timer-cnt: fix ceiling write max value | expand |
On Tue, Mar 02, 2021 at 03:43:55PM +0100, Fabrice Gasnier wrote: > The ceiling value isn't checked before writing it into registers. The user > could write a value higher than the counter resolution (e.g. 16 or 32 bits > indicated by max_arr). This makes most significant bits to be truncated. > Fix it by checking the max_arr to report a range error [1] to the user. > > Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder") > > [1] https://lkml.org/lkml/2021/2/12/358 > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com> Side question: if priv->ceiling is tracking the current ceiling configuration, would it make sense to change stm32_count_ceiling_read() to print the value of priv->ceiling instead of doing a regmap_read() call? > --- > drivers/counter/stm32-timer-cnt.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c > index ef2a974..2cf0c05 100644 > --- a/drivers/counter/stm32-timer-cnt.c > +++ b/drivers/counter/stm32-timer-cnt.c > @@ -32,6 +32,7 @@ struct stm32_timer_cnt { > struct regmap *regmap; > struct clk *clk; > u32 ceiling; > + u32 max_arr; > bool enabled; > struct stm32_timer_regs bak; > }; > @@ -185,6 +186,9 @@ static ssize_t stm32_count_ceiling_write(struct counter_device *counter, > if (ret) > return ret; > > + if (ceiling > priv->max_arr) > + return -ERANGE; > + > /* TIMx_ARR register shouldn't be buffered (ARPE=0) */ > regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0); > regmap_write(priv->regmap, TIM_ARR, ceiling); > @@ -360,6 +364,7 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) > priv->regmap = ddata->regmap; > priv->clk = ddata->clk; > priv->ceiling = ddata->max_arr; > + priv->max_arr = ddata->max_arr; > > priv->counter.name = dev_name(dev); > priv->counter.parent = dev; > -- > 2.7.4 >
On 3/3/21 12:42 AM, William Breathitt Gray wrote: > On Tue, Mar 02, 2021 at 06:03:25PM +0100, Fabrice Gasnier wrote: >> On 3/2/21 3:56 PM, William Breathitt Gray wrote: >>> Side question: if priv->ceiling is tracking the current ceiling >>> configuration, would it make sense to change stm32_count_ceiling_read() >>> to print the value of priv->ceiling instead of doing a regmap_read() >>> call? >> >> Hi William, >> >> Thanks for reviewing. >> >> I'd be fine either way. So no objection to move to the priv->ceiling >> (cached) value. It could also here here. >> By looking at this, I figured out there's probably another thing to fix >> here, for initial conditions. >> >> At probe time priv->ceiling is initialized to max value (ex 65535 for a >> 16 bits counter). But the register content is 0 (clear by mfd driver at >> probe time). >> >> - So, reading ceiling from sysfs currently reports 0 (regmap_read()) >> after booting and probing. >> >> I see two cases at this point: >> - In case the counter gets enabled without any prior configuration, it >> won't count: ceiling value (e.g. 65535) should be written to register >> before it is enabled, so the counter will actually count. So there's >> room for a fix here. >> >> - In case function gets set (ex: quadrature x4), priv->ceiling (e.g. >> 65535) gets written to the register (although it's been read earlier as >> 0 from sysfs). >> This could be fixed by reading the priv->ceiling in >> stm32_count_ceiling_read() as you're asking (provided 1st case has been >> fixed as well) >> >> I'll probably prepare one or two patches for the above cases, if you agree ? >> >> Best Regards, >> Fabrice > > Looking through the driver, it doesn't seem like priv->ceiling is used > in any performance critical code, just the callbacks for count_write() > and function_set(). It might make more sense to remove priv->ceiling and > replace it with the regmap_read() calls where necessary so that we > always get the most current ceiling value from the device when needed. Hi William, Ok, I'll look into this. > > As for the default ceiling value for the device at probe time, this > should probably be set to the max value because that is what a normal > user would expect when loading a Counter driver (a ceiling value of 0 at > startup is somewhat unintuitive). Yes, I agree. In fact, the default (reset) value is the maximum. The 0 value is forced in ARR register by the mfd driver [1], after reading the max_arr value. I see no rationale for this. I think the fix should probably be done there: I'd prefer to backup and restore ARR value in the mfd, instead of unconditionally clearing it. [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/mfd/stm32-timers.c?h=v5.11#n167 > > If you prepare those two patches, then that should resolve this. I'll prepare this. Thanks for your advices, Best Regards, Fabrice > > William Breathitt Gray >
On Tue, 2 Mar 2021 23:56:46 +0900 William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > On Tue, Mar 02, 2021 at 03:43:55PM +0100, Fabrice Gasnier wrote: > > The ceiling value isn't checked before writing it into registers. The user > > could write a value higher than the counter resolution (e.g. 16 or 32 bits > > indicated by max_arr). This makes most significant bits to be truncated. > > Fix it by checking the max_arr to report a range error [1] to the user. > > > > Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder") > > > > [1] https://lkml.org/lkml/2021/2/12/358 > > > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> > > Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com> Applied to the fixes-togreg branch of iio.git and marked for stable. thanks Jonathan > > Side question: if priv->ceiling is tracking the current ceiling > configuration, would it make sense to change stm32_count_ceiling_read() > to print the value of priv->ceiling instead of doing a regmap_read() > call? > > > --- > > drivers/counter/stm32-timer-cnt.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c > > index ef2a974..2cf0c05 100644 > > --- a/drivers/counter/stm32-timer-cnt.c > > +++ b/drivers/counter/stm32-timer-cnt.c > > @@ -32,6 +32,7 @@ struct stm32_timer_cnt { > > struct regmap *regmap; > > struct clk *clk; > > u32 ceiling; > > + u32 max_arr; > > bool enabled; > > struct stm32_timer_regs bak; > > }; > > @@ -185,6 +186,9 @@ static ssize_t stm32_count_ceiling_write(struct counter_device *counter, > > if (ret) > > return ret; > > > > + if (ceiling > priv->max_arr) > > + return -ERANGE; > > + > > /* TIMx_ARR register shouldn't be buffered (ARPE=0) */ > > regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0); > > regmap_write(priv->regmap, TIM_ARR, ceiling); > > @@ -360,6 +364,7 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) > > priv->regmap = ddata->regmap; > > priv->clk = ddata->clk; > > priv->ceiling = ddata->max_arr; > > + priv->max_arr = ddata->max_arr; > > > > priv->counter.name = dev_name(dev); > > priv->counter.parent = dev; > > -- > > 2.7.4 > >
diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c index ef2a974..2cf0c05 100644 --- a/drivers/counter/stm32-timer-cnt.c +++ b/drivers/counter/stm32-timer-cnt.c @@ -32,6 +32,7 @@ struct stm32_timer_cnt { struct regmap *regmap; struct clk *clk; u32 ceiling; + u32 max_arr; bool enabled; struct stm32_timer_regs bak; }; @@ -185,6 +186,9 @@ static ssize_t stm32_count_ceiling_write(struct counter_device *counter, if (ret) return ret; + if (ceiling > priv->max_arr) + return -ERANGE; + /* TIMx_ARR register shouldn't be buffered (ARPE=0) */ regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0); regmap_write(priv->regmap, TIM_ARR, ceiling); @@ -360,6 +364,7 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev) priv->regmap = ddata->regmap; priv->clk = ddata->clk; priv->ceiling = ddata->max_arr; + priv->max_arr = ddata->max_arr; priv->counter.name = dev_name(dev); priv->counter.parent = dev;
The ceiling value isn't checked before writing it into registers. The user could write a value higher than the counter resolution (e.g. 16 or 32 bits indicated by max_arr). This makes most significant bits to be truncated. Fix it by checking the max_arr to report a range error [1] to the user. Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder") [1] https://lkml.org/lkml/2021/2/12/358 Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> --- drivers/counter/stm32-timer-cnt.c | 5 +++++ 1 file changed, 5 insertions(+)