linux-kernel - Re: [PATCH v3 2/2] watchdog: rti

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: <299c363a-23c7-4522-b58c-100f49c4eece@ti.com>
Date: Thu, 10 Jul 2025 09:08:44 -0500
From: Judith Mendez <jm@...com>
To: Guenter Roeck <linux@...ck-us.net>, Andrew Davis <afd@...com>
CC: Wim Van Sebroeck <wim@...ux-watchdog.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Vignesh Raghavendra <vigneshr@...com>, Tero Kristo <kristo@...nel.org>,
        <linux-watchdog@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] watchdog: rti_wdt: Add reaction control

Hi Guenter, Andrew,

On 7/7/25 5:55 PM, Guenter Roeck wrote:
> On Mon, Jul 07, 2025 at 04:49:31PM -0500, Andrew Davis wrote:
>> On 7/7/25 3:58 PM, Guenter Roeck wrote:
>>> On Mon, Jul 07, 2025 at 01:00:02PM -0500, Judith Mendez wrote:
>>>> This allows to configure reaction between NMI and reset for WWD.
>>>>
>>>> On K3 SoC's other than AM62L SoC [0], watchdog reset output is routed
>>>> to the ESM module which can subsequently route the signal to safety
>>>> master or SoC reset. On AM62L, the watchdog reset output is routed
>>>> to the SoC HW reset block. So, add a new compatible for AM62l to add
>>>> SoC data and configure reaction to reset instead of NMI.
>>>>
>>>> [0] https://www.ti.com/product/AM62L
>>>> Signed-off-by: Judith Mendez <jm@...com>
>>>> ---
>>>>    drivers/watchdog/rti_wdt.c | 32 ++++++++++++++++++++++++++++----
>>>>    1 file changed, 28 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>>>> index d1f9ce4100a8..c9ee443c70af 100644
>>>> --- a/drivers/watchdog/rti_wdt.c
>>>> +++ b/drivers/watchdog/rti_wdt.c
>>>> @@ -35,7 +35,8 @@
>>>>    #define RTIWWDRXCTRL	0xa4
>>>>    #define RTIWWDSIZECTRL	0xa8
>>>> -#define RTIWWDRX_NMI	0xa
>>>> +#define RTIWWDRXN_RST	0x5
>>>> +#define RTIWWDRXN_NMI	0xa
>>>>    #define RTIWWDSIZE_50P		0x50
>>>>    #define RTIWWDSIZE_25P		0x500
>>>> @@ -63,22 +64,29 @@
>>>>    static int heartbeat;
>>>> +struct rti_wdt_data {
>>>> +	bool reset;
>>>> +};
>>>> +
>>>>    /*
>>>>     * struct to hold data for each WDT device
>>>>     * @base - base io address of WD device
>>>>     * @freq - source clock frequency of WDT
>>>>     * @wdd  - hold watchdog device as is in WDT core
>>>> + * @data - hold configuration data
>>>>     */
>>>>    struct rti_wdt_device {
>>>>    	void __iomem		*base;
>>>>    	unsigned long		freq;
>>>>    	struct watchdog_device	wdd;
>>>> +	const struct rti_wdt_data *data;
>>>>    };
>>>>    static int rti_wdt_start(struct watchdog_device *wdd)
>>>>    {
>>>>    	u32 timer_margin;
>>>>    	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
>>>> +	u8 reaction;
>>>>    	int ret;
>>>>    	ret = pm_runtime_resume_and_get(wdd->parent);
>>>> @@ -101,8 +109,13 @@ static int rti_wdt_start(struct watchdog_device *wdd)
>>>>    	 */
>>>>    	wdd->min_hw_heartbeat_ms = 520 * wdd->timeout + MAX_HW_ERROR;
>>>> -	/* Generate NMI when wdt expires */
>>>> -	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
>>>> +	/* Reset device if wdt serviced outside of window or generate NMI if available */
>>>
>>> Shouldn't that be "or generate NMI if _not_ available" ?
>>>
>>
>> For almost all the K3 devices, the WDT has two selectable outputs, one resets
>> the device directly, the other is this "NMI" which is wired to an ESM module
>> which can take other actions (but usually it just also resets the device).
>> For AM62L that second NMI output is not wired (no ESM module), so our only
>> choice is to set the WDT to direct reset mode.
>>
>> The wording is a little strange, but the "or generate NMI if available" meaning
>> if NMI is available, then do that. Reset being the fallback when _not_ available.
>>
>> Maybe this would work better:
>>
>> /* If WDT is serviced outside of window, generate NMI if available, or reset device */
>>
> 
> The problem is that the code doesn't match the comment. The code checks the
> "reset" flag and requests a reset if available. If doesn't check an "nmi"
> flag.
> 
> If the preference is NMI, as your comment suggests, the flag should be named
> "nmi" and be set if NMI is available. That would align the code and the
> comment. Right now both code and comment are misleading, since the presence
> of a reset flag (and setting it to false) suggests that a direct reset is
> not available, and that reset is preferred if available. A reset is the
> normally expected behavior for a watchdog, so the fact that this is _not_
> the case for this watchdog should be made more visible.


How about:


/* If WWDT serviced outside of window, generate NMI or reset the device
if NMI not available */

if (wdt->data->reset)
	reaction = RTIWWDRXN_RST;
else
	reaction = RTIWWDRXN_NMI;

~ Judith

...