Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations

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

On 7/28/20 10:02 PM, Anson Huang wrote:
> Hi, Guenter
> 
> 
>> Subject: RE: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence
>> for wdog operations
>>
>> Hi, Guenter
>>
>>
>>> Subject: Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the
>>> sequence for wdog operations
>>>
>>> On 7/28/20 7:20 PM, Anson Huang wrote:
>>>> According to reference manual, the i.MX7ULP WDOG's operations should
>>>> follow below sequence:
>>>>
>>>> 1. disable global interrupts;
>>>> 2. unlock the wdog and wait unlock bit set; 3. reconfigure the wdog
>>>> and wait for reconfiguration bit set; 4. enabel global interrupts.
>>>>
>>>> Strictly follow the recommended sequence can make it more robust.
>>>>
>>>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
>>>> ---
>>>> Changes since V1:
>>>> 	- use readl_poll_timeout_atomic() instead of usleep_ranges() since
>>>> IRQ is
>>> disabled.
>>>> ---
>>>>  drivers/watchdog/imx7ulp_wdt.c | 29 +++++++++++++++++++++++++++++
>>>>  1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/drivers/watchdog/imx7ulp_wdt.c
>>>> b/drivers/watchdog/imx7ulp_wdt.c index 7993c8c..7d2b12e 100644
>>>> --- a/drivers/watchdog/imx7ulp_wdt.c
>>>> +++ b/drivers/watchdog/imx7ulp_wdt.c
>>>> @@ -5,6 +5,7 @@
>>>>
>>>>  #include <linux/clk.h>
>>>>  #include <linux/io.h>
>>>> +#include <linux/iopoll.h>
>>>>  #include <linux/kernel.h>
>>>>  #include <linux/module.h>
>>>>  #include <linux/of.h>
>>>> @@ -36,6 +37,7 @@
>>>>  #define DEFAULT_TIMEOUT	60
>>>>  #define MAX_TIMEOUT	128
>>>>  #define WDOG_CLOCK_RATE	1000
>>>> +#define WDOG_WAIT_TIMEOUT	10000
>>>>
>>>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>>> module_param(nowayout,
>>>> bool, 0000); @@ -48,17 +50,31 @@ struct imx7ulp_wdt_device {
>>>>  	struct clk *clk;
>>>>  };
>>>>
>>>> +static inline void imx7ulp_wdt_wait(void __iomem *base, u32 mask) {
>>>> +	u32 val = readl(base + WDOG_CS);
>>>> +
>>>> +	if (!(val & mask))
>>>> +		WARN_ON(readl_poll_timeout_atomic(base + WDOG_CS, val,
>>>> +						  val & mask, 0,
>>>> +						  WDOG_WAIT_TIMEOUT));
>>>
>>> I am not a friend of WARN_ON, especially in situations like this.
>>> Please explain why this is needed, and why a return of -ETIMEDOUT is
>>> not feasible.
>>
>> OK, I will use return value of -ETIMEOUT and handle it in the caller.
> 
> After a further look, some of the imx7ulp_wdt_wait () callers are void function, so if want
> to handle the return value, all those functions return type need to be changed. And, when
> the return value is -ETIMEDOUT, the ONLY action is to print out some error message
> for these void function, need to use pr_err() due to no dev pointer available, so
> do you think it is acceptable to just replace the WARN_ON with pr_err() as below?
> 
First, the point here is that the callers can't do their work if the function times
out. So, if the return value isn't necessary, and callers don't need to check it,
the function would not be necessary to start with. If it is necessary, and if there
is a concern that it can fail, callers should make sure that it actually succeeded.
With that in mind, yes, imx7ulp_wdt_init() should fail and return an error,
because presumably that is what happened. The same is true for imx7ulp_wdt_enable().
Really, what is the point of detecting a problem just to ignore it ?

Second, the wait function is also called _after_ a register was set. In many cases
that won't do any good or bad. While it is ok to ignore the error in that case
(when nothing else is done), the error message is pointless in that situation.

Thanks,
Guenter