| 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
| ||
|
Message-ID: <64390424-5c79-4354-a08b-b6ff7638903e@roeck-us.net>
Date: Tue, 13 May 2025 06:37:53 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Sangwook Shin <sw617.shin@...sung.com>, krzk@...nel.org,
alim.akhtar@...sung.com, wim@...ux-watchdog.org
Cc: linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org,
Kyunghwan Seo <khwan.seo@...sung.com>
Subject: Re: [PATCH 1/3] watchdog: s3c2410_wdt: Increase max timeout value of
watchdog
On 5/13/25 02:47, Sangwook Shin wrote:
> Increase max_timeout value from 55s to 3665038s (1018h 3min 58s) with
> 38400000 frequency system if the system has 32-bit WTCNT register.
>
> cat /sys/devices/platform/10060000.watchdog_cl0/watchdog/watchdog0/max_timeout
> 3665038
>
> [ 0.208763][ T1] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: count=1099511400000, timeout=3665038, freq=300000
> [ 0.208767][ T1] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: timeout=3665038, divisor=256, count=1099511400000 (fffffc87)
> [ 0.208799][ T1] s3c2410-wdt 10060000.watchdog_cl0: starting watchdog timer
> [ 0.208969][ T1] s3c2410-wdt 10060000.watchdog_cl0: watchdog active, reset enabled, irq disabled
>
> If system has 32-bit WTCNT, add QUIRK_HAS_32BIT_MAXCNT to its quirk flags, then
> it will operation with 32-bit counter. If not, with 16-bit counter like previous.
>
> Signed-off-by: Sangwook Shin <sw617.shin@...sung.com>
> Signed-off-by: Kyunghwan Seo <khwan.seo@...sung.com>
> ---
> drivers/watchdog/s3c2410_wdt.c | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index bdd81d8074b2..a13768a11f20 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -34,6 +34,7 @@
> #define S3C2410_WTCLRINT 0x0c
>
> #define S3C2410_WTCNT_MAXCNT 0xffff
> +#define S3C2410_WTCNT_MAXCNT_32 0xffffffff
>
> #define S3C2410_WTCON_RSTEN BIT(0)
> #define S3C2410_WTCON_INTEN BIT(2)
> @@ -119,6 +120,10 @@
> * %QUIRK_HAS_DBGACK_BIT: WTCON register has DBGACK_MASK bit. Setting the
> * DBGACK_MASK bit disables the watchdog outputs when the SoC is in debug mode.
> * Debug mode is determined by the DBGACK CPU signal.
> + *
> + * %QUIRK_HAS_32BIT_MAXCNT: WTDAT and WTCNT are 32-bit registers. With these
> + * 32-bit registers, larger values to be set, which means that larger timeouts
> + * value can be set.
> */
> #define QUIRK_HAS_WTCLRINT_REG BIT(0)
> #define QUIRK_HAS_PMU_MASK_RESET BIT(1)
> @@ -126,6 +131,7 @@
> #define QUIRK_HAS_PMU_AUTO_DISABLE BIT(3)
> #define QUIRK_HAS_PMU_CNT_EN BIT(4)
> #define QUIRK_HAS_DBGACK_BIT BIT(5)
> +#define QUIRK_HAS_32BIT_MAXCNT BIT(6)
>
> /* These quirks require that we have a PMU register map */
> #define QUIRKS_HAVE_PMUREG \
> @@ -378,9 +384,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);
> + unsigned long max_cnt = S3C2410_WTCNT_MAXCNT;
> +
> + if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT))
> + max_cnt = S3C2410_WTCNT_MAXCNT_32;
>
> - return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> - / S3C2410_WTCON_MAXDIV);
> + return max_cnt * (S3C2410_WTCON_PRESCALE_MAX + 1)
> + * S3C2410_WTCON_MAXDIV / freq;
This will overflow if the build is a 32-bit build with sizeof(long) == 4.
The previous calculation did not have that problem. If there was a secondary
reason to change the calculation, please submit a second patch with that change
and explanation on its own.
Thanks,
Guenter
> }
>
> static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> @@ -534,9 +544,10 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
> {
> struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> unsigned long freq = s3c2410wdt_get_freq(wdt);
> - unsigned int count;
> + unsigned long count;
> unsigned int divisor = 1;
> unsigned long wtcon;
> + unsigned int max_cnt = S3C2410_WTCNT_MAXCNT;
>
> if (timeout < 1)
> return -EINVAL;
> @@ -544,7 +555,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
> freq = DIV_ROUND_UP(freq, 128);
> count = timeout * freq;
>
> - dev_dbg(wdt->dev, "Heartbeat: count=%d, timeout=%d, freq=%lu\n",
> + dev_dbg(wdt->dev, "Heartbeat: count=%lu, timeout=%d, freq=%lu\n",
> count, timeout, freq);
>
> /* if the count is bigger than the watchdog register,
> @@ -552,16 +563,19 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
> actually make this value
> */
>
> - if (count >= 0x10000) {
> - divisor = DIV_ROUND_UP(count, 0xffff);
> + if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT))
> + max_cnt = S3C2410_WTCNT_MAXCNT_32;
> +
I think it would make sense to store max_cnt in struct s3c2410_wdt instead
of recalculating it repeatedly during runtime.
> + if (count > max_cnt) {
> + divisor = DIV_ROUND_UP(count, max_cnt);
>
> - if (divisor > 0x100) {
> + if (divisor > (S3C2410_WTCON_PRESCALE_MAX + 1)) {
That is another change that seems unrelated to this patch. Note that inner
the ( ) s unnecessary.
> dev_err(wdt->dev, "timeout %d too big\n", timeout);
> return -EINVAL;
> }
> }
>
> - dev_dbg(wdt->dev, "Heartbeat: timeout=%d, divisor=%d, count=%d (%08x)\n",
> + dev_dbg(wdt->dev, "Heartbeat: timeout=%d, divisor=%d, count=%lu (%08lx)\n",
> timeout, divisor, count, DIV_ROUND_UP(count, divisor));
>
> count = DIV_ROUND_UP(count, divisor);
Powered by blists - more mailing lists