Re: [PATCH v2 1/3] watchdog: wdat_wdg: Using the existed function to check parameter timeout

[Guenter Roeck]

On 4/25/22 20:04, Tzung-Bi Shih wrote:
> On Mon, Apr 25, 2022 at 04:22:10PM +0800, Liu Xinpeng wrote:
> >   #define MAX_WDAT_ACTIONS ACPI_WDAT_ACTION_RESERVED
> > +#define WDAT_TIMEOUT_MIN     1
> >   
> >   /**
> >    * struct wdat_instruction - Single ACPI WDAT instruction
> > @@ -344,6 +345,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
> >   	wdat->period = tbl->timer_period;
> >   	wdat->wdd.min_hw_heartbeat_ms = wdat->period * tbl->min_count;
> >   	wdat->wdd.max_hw_heartbeat_ms = wdat->period * tbl->max_count;
> > +	wdat->wdd.min_timeout = WDAT_TIMEOUT_MIN;
> >   	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
> >   	wdat->wdd.info = &wdat_wdt_info;
> >   	wdat->wdd.ops = &wdat_wdt_ops;
> > @@ -450,8 +452,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
> >   	 * watchdog properly after it has opened the device. In some cases
> >   	 * the BIOS default is too short and causes immediate reboot.
> >   	 */
> > -	if (timeout * 1000 < wdat->wdd.min_hw_heartbeat_ms ||
> > -	    timeout * 1000 > wdat->wdd.max_hw_heartbeat_ms) {
> > +	if (watchdog_timeout_invalid(&wdat->wdd, timeout)) {
>
> Probably lacking of the context, I failed to see why the checks are
> equivalent.  Could you provide more information?

Not exactly equivalent; this is a fix.

Context: If max_hw_heartbeat_ms is provided, the configured maximum timeout
is not limited by it. The limit check in this driver therefore doesn't make
much sense. Similar, the watchdog core ensures that minimum timeout limits
are met if min_hw_heartbeat_ms is set. Using watchdog_timeout_invalid()
makes more sense because it takes this into account.

Guenter