linux-kernel - RE: R: [PATCH v1] watchdog: wdat: add param. to start wdog on module insertion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 22 Feb 2021 11:28:18 +0000
From:   Flavio Suligoi <f.suligoi@...m.it>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Mika Westerberg" <mika.westerberg@...ux.intel.com>
Subject: RE: R: [PATCH v1] watchdog: wdat: add param. to start wdog on module
 insertion

Hi Guenter

> >>>  	 const struct wdat_instruction *instr, u32 *value)  { @@ -437,6
> >>> +443,8 @@ static int wdat_wdt_probe(struct platform_device
> >> *pdev)
> >>>  	}
> >>>
> >>>  	wdat_wdt_boot_status(wdat);
> >>> +	if (start_enabled)
> >>> +		wdat_wdt_start(&wdat->wdd);
> >>
> >> No objections to this if it is really needed. However, I think it is
> >> better start the watchdog after devm_watchdog_register_device() has
> >> been called so we have everything initialized.
> >
> > Yes, it is needed. We need this feature to enable the watchdog as soon
> > as possible and this is essential for unmanned applications, such as
> > routers, water pumping stations, climate data collections, etc.
> >
> FWIW, in your use case the watchdog should be enabled in the
> BIOS/ROMMON.

Yes, you are right,  with the new BIOS version for the new boards
we'll implement this features, but for the old boards it is no more possible.

> 
> > Right, ok for the correct positioning of the wdat_wdt_start function
> > at the end of the watchdog device initialization. Thanks!
> >
> 
> No, it isn't, because it won't set WDOG_HW_RUNNING, and the watchdog
> core won't know that the watchdog is running.

Ok

> The watchdog has to be started before the call to wdat_wdt_set_running().
> If that isn't possible with the current location of wdat_wdt_set_running(),
> then
> wdat_wdt_set_running() has to be moved accordingly.
> Either case, both have to be called before calling
> devm_watchdog_register_device().

Ok

> 
> Having said that, I'd prefer to have a module parameter in the watchdog
> core. We already have a number of similar module parameters in various
> drivers, all named differently, and I'd rather not have more.

Ok, I'll study how to introduce a this new parameter in the wdog core,
so that it can be available for all watchdog drivers.
Then we'll have to think what to do with the existent similar parameters.
I think we have to keep them for compatibility reasons.

> 
> Guenter
> 

Regards,
Flavio