From: Fabrice Gasnier <fabrice.gasnier@foss.st.com> To: William Breathitt Gray <vilhelm.gray@gmail.com> Cc: <jic23@kernel.org>, <david@lechnology.com>, <alexandre.torgue@foss.st.com>, <mcoquelin.stm32@gmail.com>, <olivier.moysan@foss.st.com>, <linux-iio@vger.kernel.org>, <linux-stm32@st-md-mailman.stormreply.com>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] counter: stm32-timer-cnt: fix ceiling write max value Date: Tue, 2 Mar 2021 18:03:25 +0100 [thread overview] Message-ID: <e54d1446-b583-9625-1ab3-09e54d6a7456@foss.st.com> (raw) In-Reply-To: <YD5SLrdttn+95M7N@shinobu> On 3/2/21 3:56 PM, William Breathitt Gray 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> > > 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 > >> --- >> 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 >>
WARNING: multiple messages have this Message-ID (diff)
From: Fabrice Gasnier <fabrice.gasnier@foss.st.com> To: William Breathitt Gray <vilhelm.gray@gmail.com> Cc: <jic23@kernel.org>, <david@lechnology.com>, <alexandre.torgue@foss.st.com>, <mcoquelin.stm32@gmail.com>, <olivier.moysan@foss.st.com>, <linux-iio@vger.kernel.org>, <linux-stm32@st-md-mailman.stormreply.com>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] counter: stm32-timer-cnt: fix ceiling write max value Date: Tue, 2 Mar 2021 18:03:25 +0100 [thread overview] Message-ID: <e54d1446-b583-9625-1ab3-09e54d6a7456@foss.st.com> (raw) In-Reply-To: <YD5SLrdttn+95M7N@shinobu> On 3/2/21 3:56 PM, William Breathitt Gray 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> > > 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 > >> --- >> 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 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-03-02 20:58 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-02 14:43 [PATCH] counter: stm32-timer-cnt: fix ceiling write max value Fabrice Gasnier 2021-03-02 14:43 ` Fabrice Gasnier 2021-03-02 14:56 ` William Breathitt Gray 2021-03-02 14:56 ` William Breathitt Gray 2021-03-02 17:03 ` Fabrice Gasnier [this message] 2021-03-02 17:03 ` Fabrice Gasnier 2021-03-02 23:42 ` William Breathitt Gray 2021-03-02 23:42 ` William Breathitt Gray 2021-03-03 16:11 ` Fabrice Gasnier 2021-03-03 16:11 ` Fabrice Gasnier 2021-03-06 16:47 ` Jonathan Cameron 2021-03-06 16:47 ` Jonathan Cameron
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=e54d1446-b583-9625-1ab3-09e54d6a7456@foss.st.com \ --to=fabrice.gasnier@foss.st.com \ --cc=alexandre.torgue@foss.st.com \ --cc=david@lechnology.com \ --cc=jic23@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-iio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=mcoquelin.stm32@gmail.com \ --cc=olivier.moysan@foss.st.com \ --cc=vilhelm.gray@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.