| 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: <CAPLW+4n=H3cPe=XqMfUJKqpPJdot9t0j_kBMkNY3W+x9buokTg@mail.gmail.com>
Date: Wed, 6 Aug 2025 13:06:46 -0500
From: Sam Protsenko <semen.protsenko@...aro.org>
To: Sangwook Shin <sw617.shin@...sung.com>
Cc: krzk@...nel.org, alim.akhtar@...sung.com, wim@...ux-watchdog.org,
linux@...ck-us.net, dongil01.park@...sung.com, khwan.seo@...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 v5 3/5] watchdog: s3c2410_wdt: Increase max timeout value
of watchdog
On Wed, Aug 6, 2025 at 2:00 AM Sangwook Shin <sw617.shin@...sung.com> 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/class/watchdog/watchdog0/max_timeout
> 3665038
>
> [ 0.330082] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: count=1099511400000, timeout=3665038, freq=300000
> [ 0.330087] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: timeout=3665038, divisor=256, count=1099511400000 (fffffc87)
> [ 0.330127] s3c2410-wdt 10060000.watchdog_cl0: starting watchdog timer
> [ 0.330134] s3c2410-wdt 10060000.watchdog_cl0: Starting watchdog: count=0xfffffc87, wtcon=0001ff39
> [ 0.330319] s3c2410-wdt 10060000.watchdog_cl0: watchdog active, reset enabled, irq disabled
>
> If the system has a 32-bit WTCNT, add QUIRK_HAS_32BIT_CNT to its quirk flags,
> and it will operate with a 32-bit counter. If not, it will operate with a 16-bit
> counter like in the previous version.
>
> Signed-off-by: Sangwook Shin <sw617.shin@...sung.com>
> ---
> drivers/watchdog/s3c2410_wdt.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 0a4c0ab2a3d6..673ab6768688 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -34,7 +34,8 @@
> #define S3C2410_WTCNT 0x08
> #define S3C2410_WTCLRINT 0x0c
>
> -#define S3C2410_WTCNT_MAXCNT 0xffff
> +#define S3C2410_WTCNT_MAXCNT_16 0xffff
> +#define S3C2410_WTCNT_MAXCNT_32 0xffffffff
>
> #define S3C2410_WTCON_RSTEN BIT(0)
> #define S3C2410_WTCON_INTEN BIT(2)
> @@ -124,6 +125,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_CNT: WTDAT and WTCNT are 32-bit registers. With these
> + * 32-bit registers, larger values will 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)
> @@ -131,6 +136,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_CNT BIT(6)
>
> /* These quirks require that we have a PMU register map */
> #define QUIRKS_HAVE_PMUREG \
> @@ -199,6 +205,7 @@ struct s3c2410_wdt {
> struct notifier_block freq_transition;
> const struct s3c2410_wdt_variant *drv_data;
> struct regmap *pmureg;
> + u32 max_cnt;
> };
>
> static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> @@ -412,7 +419,7 @@ 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);
> + u64 t_max = div64_ul((u64)wdt->max_cnt * 0x8000, freq);
>
If you rework the hard-coded 0x8000 value w.r.t. my comments for the
previous patch, this should be changed accordingly too. So either:
u32 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV;
u64 t_max = div64_ul((u64)wdt->max_cnt * div_max, freq);
...or...
u32 cnt_max = (u64)wdt->max_cnt * (S3C2410_WTCON_PRESCALE_MAX + 1) *
S3C2410_WTCON_MAXDIV;
u64 t_max = div64_ul(cnt_max, freq);
Apart from that, looks good to me:
Reviewed-by: Sam Protsenko <semen.protsenko@...aro.org>
> if (t_max > UINT_MAX)
> t_max = UINT_MAX;
> @@ -571,7 +578,7 @@ 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;
>
> @@ -581,7 +588,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,
> @@ -589,8 +596,8 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
> actually make this value
> */
>
> - if (count >= 0x10000) {
> - divisor = DIV_ROUND_UP(count, 0xffff);
> + if (count > wdt->max_cnt) {
> + divisor = DIV_ROUND_UP(count, wdt->max_cnt);
>
> if (divisor > S3C2410_WTCON_PRESCALE_MAX + 1) {
> dev_err(wdt->dev, "timeout %d too big\n", timeout);
> @@ -598,7 +605,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
> }
> }
>
> - 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);
> @@ -806,6 +813,11 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> if (IS_ERR(wdt->src_clk))
> return dev_err_probe(dev, PTR_ERR(wdt->src_clk), "failed to get source clock\n");
>
> + if (wdt->drv_data->quirks & QUIRK_HAS_32BIT_CNT)
> + wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32;
> + else
> + wdt->max_cnt = S3C2410_WTCNT_MAXCNT_16;
> +
> wdt->wdt_device.min_timeout = 1;
> wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt);
>
> --
> 2.25.1
>
Powered by blists - more mailing lists