On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
> On 7/11/23 02:17, huaqian.li@xxxxxxxxxxx wrote:
> > From: Li Hua Qian <huaqian.li@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> > 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);
>
