[PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check

[Stefan Wahren]

Am 27.10.24 um 14:36 schrieb Guenter Roeck:
> On 10/27/24 03:53, Stefan Wahren wrote:
>> The WDOG Timeout Value (TOVAL) is a 16 bit value, which is stored
>> at the beginning of a 32 bit register. So add a range check to
>> prevent writing in the reserved register area.
>>
>> Signed-off-by: Stefan Wahren <wahrenst at gmx.net>
>> ---
>>   drivers/watchdog/imx7ulp_wdt.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/watchdog/imx7ulp_wdt.c
>> b/drivers/watchdog/imx7ulp_wdt.c
>> index 0f92d2217088..a7574f9c9150 100644
>> --- a/drivers/watchdog/imx7ulp_wdt.c
>> +++ b/drivers/watchdog/imx7ulp_wdt.c
>> @@ -48,6 +48,8 @@
>>
>>   #define RETRY_MAX 5
>>
>> +#define TOVAL_MAX    0xFFFF
>> +
>>   static bool nowayout = WATCHDOG_NOWAYOUT;
>>   module_param(nowayout, bool, 0000);
>>   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started
>> (default="
>> @@ -192,6 +194,9 @@ static int imx7ulp_wdt_set_timeout(struct
>> watchdog_device *wdog,
>>       int ret;
>>       u32 loop = RETRY_MAX;
>>
>> +    if (toval > TOVAL_MAX)
>> +        return -EINVAL;
>> +
>
> The whole idea of having max_timeout in struct watchdog_device is to
> avoid the need
> for this check. max_timeout should be set to 0xffff /
> wdt->hw->wdog_clock_rate.
> It is currently set to 128. With wdt->hw->wdog_clock_rate set to
> either 125 or 1000,
> it can indeed overflow. However, checking the value above is wrong.
> max_timeout should
> be initialized correctly instead.
>
> Even better would be to set max_hw_heartbeat_ms and let the watchdog
> core handle
> larger timeouts.
It's funny because I tried this on a i.MX93 board but it didn't work for
me. But I must confess that I didn't spend much time in the investigation.
>
> Another question is why the driver enables a clock but doesn't use its
> actual
> frequency.
Yes, this would be better

Regards
>
> Guenter
>
>