Fix the issue of max_timeout being calculated larger than actual value.
The calculation result of freq / (S3C2410_WTCON_PRESCALE_MAX + 1) /
S3C2410_WTCON_MAXDIV is smaller than the actual value because the remainder
is discarded during the calculation process. This leads to a larger
calculated value for max_timeout compared to the actual settable value.
A ceiling operation is applied in the calculation process to resolve this.
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Sangwook Shin <sw617.shin@samsung.com>
---
drivers/watchdog/s3c2410_wdt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 95f7207e390a..31f7e1ec779e 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -411,8 +411,8 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
{
const unsigned long freq = s3c2410wdt_get_freq(wdt);
- return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
- / S3C2410_WTCON_MAXDIV);
+ return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq,
+ (S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV);
}
static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
--
2.25.1On Thu, Jul 24, 2025 at 3:13 AM Sangwook Shin <sw617.shin@samsung.com> wrote:
>
> Fix the issue of max_timeout being calculated larger than actual value.
> The calculation result of freq / (S3C2410_WTCON_PRESCALE_MAX + 1) /
> S3C2410_WTCON_MAXDIV is smaller than the actual value because the remainder
> is discarded during the calculation process. This leads to a larger
> calculated value for max_timeout compared to the actual settable value.
> A ceiling operation is applied in the calculation process to resolve this.
>
> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> Signed-off-by: Sangwook Shin <sw617.shin@samsung.com>
> ---
> drivers/watchdog/s3c2410_wdt.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 95f7207e390a..31f7e1ec779e 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -411,8 +411,8 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
> {
> const unsigned long freq = s3c2410wdt_get_freq(wdt);
>
> - return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> - / S3C2410_WTCON_MAXDIV);
> + return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq,
> + (S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV);
> }
>
How about something like this instead?
8<--------------------------------------------------------------------->8
static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq)
{
const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
S3C2410_WTCON_MAXDIV; /* 32768 */
const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
u64 t_max = n_max / freq;
if (t_max > UINT_MAX)
t_max = UINT_MAX;
return (unsigned int)t_max;
}
8<--------------------------------------------------------------------->8
This implementation's result:
- is never greater than real timeout, as it loses the decimal part
after integer division in t_max
- much closer to the real timeout value, as it benefits from very
big n_max in the numerator (this is the main trick here)
- prepared for using 32-bit max counter value in your next patch, as
it uses u64 type for calculations
For example, at the clock frequency of 33 kHz:
- real timeout is: 65074.269 sec
- old function returns: 65535 sec
- your function returns: 32767 sec
- the suggested function returns: 65074 sec
I've prepared the test program you can run on your machine to play
with all implementations: [1].
Thanks!
[1] https://gist.github.com/joe-skb7/c2b2290bb0b0572a4018d81fc4ba1f3e
> static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> --
> 2.25.1
>
On Saturday, August 2, 2025 at 1:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
> How about something like this instead?
>
> 8<--------------------------------------------------------------------->8
> static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq) {
> const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
> S3C2410_WTCON_MAXDIV; /* 32768 */
> const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
> u64 t_max = n_max / freq;
>
> if (t_max > UINT_MAX)
> t_max = UINT_MAX;
>
> return (unsigned int)t_max;
> }
> 8<--------------------------------------------------------------------->8
>
> This implementation's result:
> - is never greater than real timeout, as it loses the decimal part after
> integer division in t_max
> - much closer to the real timeout value, as it benefits from very big
> n_max in the numerator (this is the main trick here)
> - prepared for using 32-bit max counter value in your next patch, as it
> uses u64 type for calculations
>
> For example, at the clock frequency of 33 kHz:
> - real timeout is: 65074.269 sec
> - old function returns: 65535 sec
> - your function returns: 32767 sec
> - the suggested function returns: 65074 sec
Thank you for your feedback.
I'll make the code changes as follows in the next patch set:
static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
{
const unsigned long freq = s3c2410wdt_get_freq(wdt);
+ const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
+ S3C2410_WTCON_MAXDIV;
+ const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
+ u64 t_max = n_max / freq;
- return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
- / S3C2410_WTCON_MAXDIV);
+ if (t_max > UINT_MAX)
+ t_max = UINT_MAX;
+
+ return (unsigned int)t_max;
}
On 8/4/25 21:22, sw617.shin@samsung.com wrote:
> On Saturday, August 2, 2025 at 1:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
>> How about something like this instead?
>>
>> 8<--------------------------------------------------------------------->8
>> static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq) {
>> const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
>> S3C2410_WTCON_MAXDIV; /* 32768 */
>> const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
>> u64 t_max = n_max / freq;
>>
>> if (t_max > UINT_MAX)
>> t_max = UINT_MAX;
>>
>> return (unsigned int)t_max;
>> }
>> 8<--------------------------------------------------------------------->8
>>
>> This implementation's result:
>> - is never greater than real timeout, as it loses the decimal part after
>> integer division in t_max
>> - much closer to the real timeout value, as it benefits from very big
>> n_max in the numerator (this is the main trick here)
>> - prepared for using 32-bit max counter value in your next patch, as it
>> uses u64 type for calculations
>>
>> For example, at the clock frequency of 33 kHz:
>> - real timeout is: 65074.269 sec
>> - old function returns: 65535 sec
>> - your function returns: 32767 sec
>> - the suggested function returns: 65074 sec
>
> Thank you for your feedback.
> I'll make the code changes as follows in the next patch set:
>
> static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
> {
> const unsigned long freq = s3c2410wdt_get_freq(wdt);
> + const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
> + S3C2410_WTCON_MAXDIV;
> + const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
Not sure if splitting this expression adds any value. Why not just the following ?
const u64 n_max = (u64)(S3C2410_WTCON_PRESCALE_MAX + 1) *
S3C2410_WTCON_MAXDIV * S3C2410_WTCNT_MAXCNT;
Or just use a define ?
> + u64 t_max = n_max / freq;
>
Make sure this compiles on 32-bit builds.
> - return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> - / S3C2410_WTCON_MAXDIV);
> + if (t_max > UINT_MAX)
> + t_max = UINT_MAX;
> +
> + return (unsigned int)t_max;
I am quite sure that this typecast is unnecessary.
Guenter
On Mon, Aug 4, 2025 at 11:47 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/4/25 21:22, sw617.shin@samsung.com wrote:
> > On Saturday, August 2, 2025 at 1:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
> >
> >> How about something like this instead?
> >>
> >> 8<--------------------------------------------------------------------->8
> >> static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq) {
> >> const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
> >> S3C2410_WTCON_MAXDIV; /* 32768 */
> >> const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
> >> u64 t_max = n_max / freq;
> >>
> >> if (t_max > UINT_MAX)
> >> t_max = UINT_MAX;
> >>
> >> return (unsigned int)t_max;
> >> }
> >> 8<--------------------------------------------------------------------->8
> >>
> >> This implementation's result:
> >> - is never greater than real timeout, as it loses the decimal part after
> >> integer division in t_max
> >> - much closer to the real timeout value, as it benefits from very big
> >> n_max in the numerator (this is the main trick here)
> >> - prepared for using 32-bit max counter value in your next patch, as it
> >> uses u64 type for calculations
> >>
> >> For example, at the clock frequency of 33 kHz:
> >> - real timeout is: 65074.269 sec
> >> - old function returns: 65535 sec
> >> - your function returns: 32767 sec
> >> - the suggested function returns: 65074 sec
> >
> > Thank you for your feedback.
> > I'll make the code changes as follows in the next patch set:
> >
> > static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
> > {
> > const unsigned long freq = s3c2410wdt_get_freq(wdt);
> > + const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
> > + S3C2410_WTCON_MAXDIV;
> > + const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
>
> Not sure if splitting this expression adds any value. Why not just the following ?
>
> const u64 n_max = (u64)(S3C2410_WTCON_PRESCALE_MAX + 1) *
> S3C2410_WTCON_MAXDIV * S3C2410_WTCNT_MAXCNT;
>
> Or just use a define ?
>
Oh, that code snippet above is just a suggestion, please treat it as
pseudo-code for this purpose, -- I just wanted to illustrate the idea,
and should've been more clear! Yes, definitely should be revised and
re-tested. And yeah, I agree, one single const or define would be
enough, no need to make it too verbose. Also, the naming may be not
the best, e.g. cnt_max might be better than n_max for example. But
I'll leave it to Sangwook Shin to decide on the implementation, just
wanted to communicate the idea.
> > + u64 t_max = n_max / freq;
> >
>
> Make sure this compiles on 32-bit builds.
>
Can you please elaborate what might be the possible problem -- just
curious? I admit I never though about 32-bit case when writing that
code, but don't see any immediate issues with that too.
> > - return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> > - / S3C2410_WTCON_MAXDIV);
> > + if (t_max > UINT_MAX)
> > + t_max = UINT_MAX;
> > +
> > + return (unsigned int)t_max;
>
> I am quite sure that this typecast is unnecessary.
>
> Guenter
>
On Tuesday, August 5, 2025 at 2:03 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> > > + u64 t_max = n_max / freq;
> > >
> >
> > Make sure this compiles on 32-bit builds.
> >
>
> Can you please elaborate what might be the possible problem -- just
> curious? I admit I never though about 32-bit case when writing that code,
> but don't see any immediate issues with that too.
>
In my opinion, it seems that Gunter Reck's explanation is correct.
I've found out that the error of "undefined reference to '__aeabi_uldivmod'" may occur when compiling new code on a 32-bit architecture.
If you don't mind, I would like to proceed with maintaining the previous revision below.
From my perspective, this approach appears to be the most reasonable solution for supporting both 32-bit and 64-bit architectures.
@@ -411,8 +411,8 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
{
const unsigned long freq = s3c2410wdt_get_freq(wdt);
- return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
- / S3C2410_WTCON_MAXDIV);
+ return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq,
+ (S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV);
}
On 8/5/25 00:26, sw617.shin@samsung.com wrote: > On Tuesday, August 5, 2025 at 2:03 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > >> >>>> + u64 t_max = n_max / freq; >>>> >>> >>> Make sure this compiles on 32-bit builds. >>> >> >> Can you please elaborate what might be the possible problem -- just >> curious? I admit I never though about 32-bit case when writing that code, >> but don't see any immediate issues with that too. >> > > In my opinion, it seems that Gunter Reck's explanation is correct. > I've found out that the error of "undefined reference to '__aeabi_uldivmod'" may occur when compiling new code on a 32-bit architecture. Also see include/linux/math64.h. The functions implemented or declared in that include file do have a reason to exist. Guenter
On Tue, Aug 5, 2025 at 2:26 AM <sw617.shin@samsung.com> wrote:
>
> On Tuesday, August 5, 2025 at 2:03 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> >
> > > > + u64 t_max = n_max / freq;
> > > >
> > >
> > > Make sure this compiles on 32-bit builds.
> > >
> >
> > Can you please elaborate what might be the possible problem -- just
> > curious? I admit I never though about 32-bit case when writing that code,
> > but don't see any immediate issues with that too.
> >
>
> In my opinion, it seems that Gunter Reck's explanation is correct.
> I've found out that the error of "undefined reference to '__aeabi_uldivmod'" may occur when compiling new code on a 32-bit architecture.
Indeed. I was able to reproduce that behavior when building for ARM32 too.
> If you don't mind, I would like to proceed with maintaining the previous revision below.
> From my perspective, this approach appears to be the most reasonable solution for supporting both 32-bit and 64-bit architectures.
>
> @@ -411,8 +411,8 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
> {
> const unsigned long freq = s3c2410wdt_get_freq(wdt);
>
> - return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> - / S3C2410_WTCON_MAXDIV);
> + return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq,
> + (S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV);
> }
>
I don't mind, although it's quite easy to fix the code I suggested by
replacing this line:
u64 t_max = n_max / freq;
with this one:
u64 t_max = div64_ul(n_max, freq);
from <math64.h>, as Guenter suggested. But I'm totally fine with your
implementation as well.
On Wednesday, August 6, 2025 at 7:53 AM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> I don't mind, although it's quite easy to fix the code I suggested by
> replacing this line:
>
> u64 t_max = n_max / freq;
>
> with this one:
>
> u64 t_max = div64_ul(n_max, freq);
>
> from <math64.h>, as Guenter suggested. But I'm totally fine with your
> implementation as well.
Sam Protsenko and Guenter Roeck, thank you for your kind advice.
As you mentioned, I will proceed with the following code for the next patch set.
@@ -27,6 +27,7 @@
#include <linux/mfd/syscon.h>
#include <linux/regmap.h>
#include <linux/delay.h>
+#include <linux/math64.h>
#define S3C2410_WTCON 0x00
#define S3C2410_WTDAT 0x04
@@ -410,9 +411,13 @@ static inline unsigned long s3c2410wdt_get_freq(struct s3c2410_wdt *wdt)
static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
{
const unsigned long freq = s3c2410wdt_get_freq(wdt);
+ //(S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV = 0x8000
+ u64 t_max = div64_ul((u64)S3C2410_WTCNT_MAXCNT * 0x8000, freq);
- return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
- / S3C2410_WTCON_MAXDIV);
+ if (t_max > UINT_MAX)
+ t_max = UINT_MAX;
+
+ return t_max;
}
© 2016 - 2025 Red Hat, Inc.