linux-kernel - Re: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <18adfcd0-e955-4c3f-a68a-6a2f75ebd24d@roeck-us.net>
Date: Mon, 4 Aug 2025 21:47:12 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: sw617.shin@...sung.com, 'Sam Protsenko' <semen.protsenko@...aro.org>
Cc: krzk@...nel.org, alim.akhtar@...sung.com, wim@...ux-watchdog.org,
 khwan.seo@...sung.com, dongil01.park@...sung.com,
 linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
 linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being
 calculated larger

On 8/4/25 21:22, sw617.shin@...sung.com wrote:
> On Saturday, August 2, 2025 at 1:12 PM Sam Protsenko <semen.protsenko@...aro.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