| 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: <0d1b15b1016d451862b895f851c6c6c81472755f.camel@siemens.com>
Date: Wed, 12 Jul 2023 04:03:48 +0000
From: "Li, Hua Qian" <HuaQian.Li@...mens.com>
To: "linux@...ck-us.net" <linux@...ck-us.net>,
"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 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?
> > + vaddr = memremap(paddr, reserved_mem_size,
> > 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?
Best regards,
Li Hua Qian
> > +
> > watchdog_init_timeout(wdd, heartbeat, dev);
> >
> > ret = watchdog_register_device(wdd);
>
Powered by blists - more mailing lists