linux-kernel - Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP S32 platform

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: <5ee20c44-0846-440d-8ae8-8f7e6b8743cf@roeck-us.net>
Date: Fri, 28 Mar 2025 12:56:22 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Daniel Lezcano <daniel.lezcano@...aro.org>, wim@...ux-watchdog.org
Cc: linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org,
 S32@....com, Ghennadi Procopciuc <ghennadi.procopciuc@....com>,
 Thomas Fossati <thomas.fossati@...aro.org>
Subject: Re: [PATCH 2/2] watchdog: Add the Software Watchdog Timer for the NXP
 S32 platform

On 3/28/25 12:42, Daniel Lezcano wrote:
> 
> Hi Guenter,
> 
> thanks for your review
> 
> On 28/03/2025 19:10, Guenter Roeck wrote:
>> On 3/28/25 08:15, Daniel Lezcano wrote:
>>> The S32 platform has several Software Watchdog Timer available and
>>
>> Why "Software" ? This is a hardware watchdog, or am I missing something ?
> 
> I have no idea why it is called 'Software' because it is indeed a hardware watchdog. It is how NXP called it in their technical reference manual.
> 

I guess it is because it resets the software. Please drop the term;
it is misleading.

>>> tied with a CPU. The SWT0 is the only one which directly asserts the
>>> reset line, other SWT require an external setup to configure the reset
>>> behavior which is not part of this change.
>>>
>>> The maximum watchdog value depends on the clock feeding the SWT
>>> counter which is 32bits wide. On the s32g274-rb2, the clock has a rate
>>> of 51MHz which lead to 83 seconds maximum timeout.
>>>
>>> The timeout can be specified via the device tree with the usual
>>> existing bindings 'timeout-sec' or via the module param timeout.
>>>
>>> The watchdog can be loaded with the 'nowayout' option, preventing the
>>> watchdog to be stopped.
>>>
>>> The watchdog can be started at boot time with the 'early-enable'
>>> option, thus letting the watchdog framework to service the watchdog
>>> counter.
>>>
>>> the watchdog support the magic character to stop when the userspace
>>> releases the device.
>>>
>>> Cc: Ghennadi Procopciuc <ghennadi.procopciuc@....com>
>>> Cc: Thomas Fossati <thomas.fossati@...aro.org>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
>>> ---
> 
> [ ... ]
> 
>>> --- /dev/null
>>> +++ b/drivers/watchdog/s32g_wdt.c
>>> @@ -0,0 +1,362 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Watchdog driver for S32G SoC
>>> + *
>>> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
>>> + * Copyright 2017-2019, 2021-2025 NXP.
>>
>> Does this originate from out-of-tree code ?
>> If so, a reference would be helpful.
> 
> Well, I kept the copyright but this implementation is mostly from scratch.
> 
I am not a copyright expert, but if this isn't derived from some other driver
it should not include old copyrights.

>>> +#include <linux/debugfs.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/moduleparam.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/watchdog.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/io.h>
>>> +#include <linux/of.h>
>>
>> Alphabetic include file order, please.
>>
>>> +
>>> +#define DRIVER_NAME "s32g-wdt"
>>> +
>>> +#define S32G_SWT_CR(__base)    (__base + 0x00)        /* Control Register offset    */
>>
>> checkpatch:
>>      CHECK: Macro argument '__base' may be better as '(__base)' to avoid precedence issues
> 
> I'm not sure to get this one.
> 

#define S32G_SWT_CR(__base)    ((__base) + 0x00)

>>> +#define S32G_SWT_CR_SM        BIT(9) | BIT(10)    /* -> Service Mode        */
>>
>> checkpatch:
>>      ERROR: Macros with complex values should be enclosed in parentheses
>>
>> I am not going to comment on the other issues reported by checkpatch,
>> but I expect them to be fixed in the next version. I would strongly suggest
>> to run "checkpatch o--strict" on the patch and fix what it reports.
> 
> Sure, I will do that, thanks
> 
> [ ... ]
> 
>>> +static void s32g_wdt_debugfs_init(struct device *dev, struct s32g_wdt_device *wdev)
>>> +{
>>> +    struct debugfs_regset32 *regset;
>>> +    static struct dentry *dentry = NULL;
>>> +
>>> +    if (!dentry)
>>> +        dentry = debugfs_create_dir("watchdog", NULL);
>>
>> That is a terribly generic debugfs directory name. That is unacceptable.
>> Pick a name that is driver specific.
> 
> Why is it terrible ? We end up with:
> 
> watchdog/40100000.watchdog
> 
> There are 7 watchdogs on the platform, the directory is there to group them all. It seems to me it is self-explanatory, no ?
> 

It should be something like "s32g/40100000.watchdog". Someone could have some other watchdog
(say, a real software watchdog) in the system and decide to use the top level directory name
for whatever reason. The top level should be something driver specific, not a generic name.

>>> +
>>> +    dentry = debugfs_create_dir(dev_name(dev), dentry);
>>> +
>>
>> Where is this removed if the driver is unloaded ?
> 
> Oh right, I missed it. Thanks for pointing this out.
> 
>> Also, if the driver is built into the kernel, it seems to me that a second
>> instance will create a nested directory. That seems odd.
> 
> No, because there is the test above if (!dentry) which is a static variable.
> 

Yes, and then the second watchdog will create "watchdog/40100000.watchdog/40200000.watchdog"
or similar because dentry is overwritten with the reference to "40100000.watchdog"

Thanks,
Guenter