| 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: <ec89d59b-bb8d-ae7e-2284-1fd6bca22366@roeck-us.net>
Date: Tue, 11 Jul 2023 22:01:30 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Li, Hua Qian" <HuaQian.Li@...mens.com>,
"wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>
Cc: "kristo@...nel.org" <kristo@...nel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"huaqianlee@...il.com" <huaqianlee@...il.com>,
"nm@...com" <nm@...com>, "vigneshr@...com" <vigneshr@...com>,
"Kiszka, Jan" <jan.kiszka@...mens.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"Su, Bao Cheng" <baocheng.su@...mens.com>,
"linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET
On 7/11/23 21:03, Li, Hua Qian wrote:
> On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
>> On 7/11/23 02:17, huaqian.li@...mens.com wrote:
>>> From: Li Hua Qian <huaqian.li@...mens.com>
>>>
>>> This patch adds the WDIOF_CARDRESET support for the platform
>>> watchdog
>>> whose hardware does not support this feature, to know if the board
>>> reboot is due to a watchdog reset.
>>>
>>> This is done via reserved memory(RAM), which indicates if specific
>>> info saved, triggering the watchdog reset in last boot.
>>>
>>> Signed-off-by: Li Hua Qian <huaqian.li@...mens.com>
>>> ---
>>> drivers/watchdog/rti_wdt.c | 48
>>> ++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/rti_wdt.c
>>> b/drivers/watchdog/rti_wdt.c
>>> index ce8f18e93aa9..77fd6b54137c 100644
>>> --- a/drivers/watchdog/rti_wdt.c
>>> +++ b/drivers/watchdog/rti_wdt.c
>>> @@ -18,6 +18,7 @@
>>> #include <linux/pm_runtime.h>
>>> #include <linux/types.h>
>>> #include <linux/watchdog.h>
>>> +#include <linux/of.h>
>>>
>>> #define DEFAULT_HEARTBEAT 60
>>>
>>> @@ -52,6 +53,11 @@
>>>
>>> #define DWDST BIT(1)
>>>
>>> +#define PON_REASON_SOF_NUM 0xBBBBCCCC
>>> +#define PON_REASON_MAGIC_NUM 0xDDDDDDDD
>>> +#define PON_REASON_EOF_NUM 0xCCCCBBBB
>>> +#define PON_REASON_ITEM_BITS 0xFFFFFFFF
>>> +
>>> static int heartbeat = DEFAULT_HEARTBEAT;
>>>
>>> /*
>>> @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct
>>> platform_device *pdev)
>>> struct rti_wdt_device *wdt;
>>> struct clk *clk;
>>> u32 last_ping = 0;
>>> + u32 reserved_mem_size;
>>> + unsigned long *vaddr;
>>> + unsigned long paddr;
>>> + u32 data[3];
>>> + u32 reg[8];
>>>
>>> wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>> if (!wdt)
>>> @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct
>>> platform_device *pdev)
>>> }
>>> }
>>>
>>> + ret = of_property_read_variable_u32_array(pdev-
>>>> dev.of_node, "reg", reg,
>>> + 0, ARRAY_SIZE(reg));
>>> + if (ret < 0) {
>>> + dev_err(dev, "cannot read the reg info.\n");
>>> + goto err_iomap;
>>> + }
>>
>> This aborts if the property does not exist, which is unacceptable.
>> Any such addition must be optional.
> Agree, refactor it.
>>
>>> +
>>> + /*
>>> + * If reserved memory is defined for watchdog reset cause.
>>> + * Readout the Power-on(PON) reason and pass to bootstatus.
>>> + */
>>> + if (ret == 8) {
>>> + paddr = reg[5];
>>> + reserved_mem_size = reg[7];
>>
>> It seems odd that reserved_mem_size is not checked,
> ACK
>> and that it is even provided
>> given that it needs to be (at least) 24 bytes, and any other value
>> does not really
>> make sense.
>>
> I was thinkg to add the reliability, but it seems to be unnecessary and
> pointless. Were you suggesting that 8 bytes are enough?
>
No.
>>> MEMREMAP_WB);
>>> + if (vaddr == NULL) {
>>> + dev_err(dev, "Failed to map memory-
>>> region.\n");
>>> + goto err_iomap;
>>
>> This returns 8, which would be an odd error return.
>>
> ACK,refactor it.
>>> + }
>>> +
>>> + data[0] = *vaddr & PON_REASON_ITEM_BITS;
>>> + data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
>>> + data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
>>> +
>>
>> The & seems pointless / wasteful. Why ignore the upper 32 bits of
>> each location ?
>> Either make it u32 or make it u64 and use the entire 64 bit. Besides,
>> vaddr[0..2] would make the code much easier to read.
>>
> ACK, refactor it.
>>> + dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof
>>> = %lX\n",
>>> + data[0], data[1], data[2]);
>>> +
>>> + if ((data[0] == PON_REASON_SOF_NUM)
>>> + && (data[1] == PON_REASON_MAGIC_NUM)
>>> + && (data[1] == PON_REASON_MAGIC_NUM)) {
>>
>> Unnecessary inner (), and I don't see the point of checking data[1]
>> twice.
> Yeah, a typo happened.
>>
>>> + dev_info(dev, "Watchdog reset cause
>>> detected.\n");
>>
>> Unnecessary noise.
> ACK,rename dev_info to dev_dbg.
>>
>>> + wdd->bootstatus |= WDIOF_CARDRESET;
>>> + }
>>> + memset(vaddr, 0, reserved_mem_size);
>>> + memunmap(vaddr);
>>> + }
>>
>> And some random data in the property is acceptable ? That is odd,
>> especially
>> after mandating the property itself.
>>
> Yeah, do you have any suggestions about how to store the watchdog
> reset cause?
>
No, and that is not the point I was trying to make. Your code actively
aborts probe if the "reg" property does not exist at all, but then it
silently ignores if it contains a random number of elements (other
than 8). For example, something like
reg = <0x1234 0x5678>
is silently ignored. If anything, _that_ should return -EINVAL
because it is obviously wrong.
On a higher level, the entire code puzzles me. Obviously there
must be a means for the BIOS or ROMMON to write PON_REASON_SOF_NUM
into some memory area. That means the BIOS/ROMMON must be able
to detect that situation. Why not use the same code to detect this
in the driver without the complexity of passing it from BIOS/ROMMON
to driver in some random memory area ?
> Best regards,
> Li Hua Qian
>>> +
>>> watchdog_init_timeout(wdd, heartbeat, dev);
>>>
>>> ret = watchdog_register_device(wdd);
>>
>
Powered by blists - more mailing lists