Re: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being calculated larger

[Guenter Roeck <linux@roeck-us.net>]

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