linux-kernel - Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

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] [day] [month] [year] [list]

Message-ID: <e1827ee6d586333b91a4193fa0e27725baba8739.camel@svanheule.net>
Date:   Thu, 04 Nov 2021 10:04:05 +0100
From:   Sander Vanheule <sander@...nheule.net>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Rob Herring <robh+dt@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

On Thu, 2021-10-14 at 09:56 -0700, Guenter Roeck wrote:
> On 10/14/21 3:26 AM, Sander Vanheule wrote:
> > On Wed, 2021-10-13 at 14:03 -0700, Guenter Roeck wrote:
> > > On 10/13/21 12:46 PM, Sander Vanheule wrote:
> > > > On Wed, 2021-10-13 at 11:48 -0700, Guenter Roeck wrote:
> > > > > On Wed, Oct 13, 2021 at 03:29:00PM +0200, Sander Vanheule wrote:
> > > > [...]
> > > > 
> > > > > > 
> > > > > > diff --git a/drivers/watchdog/realtek_otto_wdt.c
> > > > > > b/drivers/watchdog/realtek_otto_wdt.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..64c9cba6b0b1
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/watchdog/realtek_otto_wdt.c
> > > > > > @@ -0,0 +1,411 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > +
> > > > > > +/*
> > > > > > + * Realtek Otto MIPS platform watchdog
> > > > > > + *
> > > > > > + * Watchdog timer that will reset the system after timeout, using the
> > > > > > selected
> > > > > > + * reset mode.
> > > > > > + *
> > > > > > + * Counter scaling and timeouts:
> > > > > > + * - Base prescale of (2 << 25), providing tick duration T_0: 168ms @
> > > > > > 200MHz
> > > > > > + * - PRESCALE: logarithmic prescaler adding a factor of {1, 2, 4, 8}
> > > > > > + * - Phase 1: Times out after (PHASE1 + 1) × PRESCALE × T_0
> > > > > > + *   Generates an interrupt, WDT cannot be stopped after phase 1
> > > > > > + * - Phase 2: starts after phase 1, times out after (PHASE2 + 1) ×
> > > > > > PRESCALE × T_0
> > > > > > + *   Resets the system according to RST_MODE
> > > > > 
> > > > > Why is there a phase2 interrupt if phase2 resets the chip ?
> > > > > 
> > > > 
> > > > The SoC's reset controller has an interrupt line for phase2, even though
> > > > then it then the
> > > > WDT also resets the system. I don't have any documentation about this
> > > > peripheral; just
> > > > some vendor code and there the phase2 interrupt isn't enabled. I mainly
> > > > added it here for
> > > > completeness.
> > > > 
> > > 
> > > It seems pointless to mandate an interrupt just for completeness.
> > 
> > Okay, then I will just drop it here. As I understand, the bindings should be as
> > complete as possible, so I think the phase2 interrupt definition should remain
> > there?
> > 
> 
> I still don't see the point of it if there is no known use case. At the very
> least it will need to be optional, but even then I would expect a description
> of the use case.
> 
> FWIW, technically I suspect that there is a means for the watchdog to generate
> a second interrupt instead of resetting the hardware (otherwise the second
> interrupt would not really make sense), but without hardware and without
> datasheet it is impossible to confirm that.

After acquiring a device with an RTL9302B SoC and testing the driver, I couldn't find
any bits that disable the WDT reset and only trigger the interrupt. So I just dropped
the phase2 interrupt implementation from the v2 patches and added a note in the
commit message.

Best,
Sander