linux-kernel - Re: [PATCH v4 3/4] watchdog: s3c2410_wdt: Increase max timeout value of watchdog

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: <CAPLW+4=sJM4UwsqfC8t=swzhGU=BdRn3T6GjJje0unZSgNg-ng@mail.gmail.com>
Date: Mon, 4 Aug 2025 23:51:00 -0500
From: Sam Protsenko <semen.protsenko@...aro.org>
To: 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 Mon, Aug 4, 2025 at 11:23 PM <sw617.shin@...sung.com> wrote:
>
> On Saturday, August 2, 2025 at 1:37 PM Sam Protsenko <semen.protsenko@...aro.org> wrote:
> > 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
> >
>
> I'll break this patch into two in the next patch set.
>
> > >  #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
> > >
>
> I'll rename this to S3C2410_WTCNT_MAXCNT_16 in the next patch set.
>
> > > + * %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").
>
> I'll modify this in the next patch set.
>
> > > +       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.
> >
>
> I'll change this to u32 in the next patch set.
>
> > >  };
> > >
> > >  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.
> >
>
> I'll break this patch into two in the next patch set.
>
> > >
> > > -       if (count >= 0x10000) {
> > > -               divisor = DIV_ROUND_UP(count, 0xffff);
> > > +       if (count > wdt->max_cnt) {
> >
> > wdt->max_cnt + 1?
> >
>
> Yes, 0x10000 represented 'wdt->max_cnt + 1.'
> Would you like to suggest any revisions?
>

Ah, sorry, I didn't notice you also changed >= to just >. All well and
good, no change is needed here!

> > > +       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;
> >
>
> I'll fix these in the next patch set.
>
>
>