Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Guenter Roeck <linux@roeck-us.net>]

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@siemens.com wrote:
>>> From: Li Hua Qian <huaqian.li@siemens.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@siemens.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);
>>
>