| 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+4mo-Fw5+KmwHdZGAM4uNpOWL6QakTgP-wMSxR=+dMGqsQ@mail.gmail.com>
Date: Fri, 1 Aug 2025 23:36:44 -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, 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 3/4] watchdog: s3c2410_wdt: Increase max timeout value
of watchdog
On Thu, Jul 24, 2025 at 3:13 AM Sangwook Shin <sw617.shin@...sung.com> wrote:
>
> Increase max_timeout value from 55s to 3664647s (1017h 57min 27s) with
> 38400000 frequency system if the system has 32-bit WTCNT register.
>
> cat /sys/devices/platform/10060000.watchdog_cl0/watchdog/watchdog0/max_timeout
> 3664647
>
> [ 0.302473] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: count=1099394100000, timeout=3664647, freq=300000
> [ 0.302479] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: timeout=3664647, divisor=256, count=1099394100000 (fff8feac)
> [ 0.302510] s3c2410-wdt 10060000.watchdog_cl0: starting watchdog timer
> [ 0.302722] 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.
>
> Reviewed-by: Alim Akhtar <alim.akhtar@...sung.com>
> Signed-off-by: Sangwook Shin <sw617.shin@...sung.com>
> ---
Not a strong point, but I'd break this patch into two:
1. Add 32-bit counter feature (without enabling it in exynosautov920
implementation)
2. Enable 32-bit counter feature in exynosautov920
> drivers/watchdog/s3c2410_wdt.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 31f7e1ec779e..184b1ad46ca6 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
Suggest renaming this to S3C2410_WTCNT_MAXCNT_16, to emphasize the
fact this value is for 16-bit counters. And for consistency with the
below one.
> +#define S3C2410_WTCNT_MAXCNT_32 0xffffffff
>
> #define S3C2410_WTCON_RSTEN BIT(0)
> #define S3C2410_WTCON_INTEN BIT(2)
> @@ -123,6 +124,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
Why not name it like QUIRK_HAS_32BIT_CNT or QUIRK_HAS_32BIT_COUNTER?
As I understand, the quirk means that the chip has 32-bit counter, so
MAX bit is not really needed?
> + * 32-bit registers, larger values to be set, which means that larger timeouts
Spelling: "to be set" -> "will be set" (or "have to be set").
> + * value can be set.
> */
> #define QUIRK_HAS_WTCLRINT_REG BIT(0)
> #define QUIRK_HAS_PMU_MASK_RESET BIT(1)
> @@ -130,6 +135,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 \
> @@ -198,6 +204,7 @@ struct s3c2410_wdt {
> struct notifier_block freq_transition;
> const struct s3c2410_wdt_variant *drv_data;
> struct regmap *pmureg;
> + unsigned int max_cnt;
Maybe make it u32? It definitely refers to a 32-bit register value, so
will be more explicit that way. Not a strong opinion though.
> };
>
> static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> @@ -349,7 +356,7 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl0 = {
> .cnt_en_bit = 8,
> .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
> QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN |
> - QUIRK_HAS_DBGACK_BIT,
> + QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT,
> };
>
> static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl1 = {
> @@ -362,7 +369,7 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl1 = {
> .cnt_en_bit = 8,
> .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
> QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN |
> - QUIRK_HAS_DBGACK_BIT,
> + QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT,
Yeah, I think it would be easier to review and handle further if this
exynosautov920 enablement is extracted into a separate patch.
> };
>
> static const struct of_device_id s3c2410_wdt_match[] = {
> @@ -411,7 +418,7 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
> {
> const unsigned long freq = s3c2410wdt_get_freq(wdt);
>
> - return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq,
> + return wdt->max_cnt / DIV_ROUND_UP(freq,
> (S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV);
> }
>
> @@ -566,7 +573,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;
>
> @@ -576,7 +583,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,
> @@ -584,8 +591,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) {
wdt->max_cnt + 1?
> + 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);
> @@ -593,7 +600,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);
> @@ -801,6 +808,10 @@ 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");
>
> + wdt->max_cnt = S3C2410_WTCNT_MAXCNT;
> + if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT))
Double braces don't seem to be needed.
> + wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32;
> +
Style (minor nitpick): this block can be more explicit, i.e.:
if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT))
wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32;
else
wdt->max_cnt = S3C2410_WTCNT_MAXCNT;
> wdt->wdt_device.min_timeout = 1;
> wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt);
>
> --
> 2.25.1
>
Powered by blists - more mailing lists