linux-kernel - Re: [v2, 7/7] watchdog: coh901327: make use of timeout-secs provided in devicetree

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]

Message-ID: <20180211173356.GA18292@roeck-us.net>
Date:   Sun, 11 Feb 2018 09:33:56 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Marcus Folkesson <marcus.folkesson@...il.com>
Cc:     Wim Van Sebroeck <wim@...ana.be>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Carlo Caione <carlo@...one.org>,
        Kevin Hilman <khilman@...libre.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Barry Song <baohua@...nel.org>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Chen-Yu Tsai <wens@...e.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Vladimir Zapolskiy <vz@...ia.com>,
        Sylvain Lemieux <slemieux.tyco@...il.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mediatek@...ts.infradead.org
Subject: Re: [v2, 7/7] watchdog: coh901327: make use of timeout-secs provided
 in devicetree

On Sat, Feb 10, 2018 at 10:19:11AM +0100, Marcus Folkesson wrote:
> watchdog_init_timeout() will allways pick timeout_param since it
> defaults to a valid timeout.
> 
> Following best practice described in
> Documentation/watchdog/watchdog-kernel-api.txt to make use of
> the parameter logic.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@...il.com>
> ---
> 
> v2:
> 	- Set .timeout in coh901327_wdt structure declaration.
> 	- Set .min_timeout to 1 instead of 0. I could not find a datasheet
> 	  for coh901327, so I'm not sure if 0 is valid. However, 0 seems
> 		  wrong to me and most driver has 1 as min value. If it should
> 		  be 0, please let me know and I have to set another initial
> 		  value for margin.

Makes sense to me.

> 
>  drivers/watchdog/coh901327_wdt.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/coh901327_wdt.c b/drivers/watchdog/coh901327_wdt.c
> index 4410337f4f7f..5d8eb9a30879 100644
> --- a/drivers/watchdog/coh901327_wdt.c
> +++ b/drivers/watchdog/coh901327_wdt.c
> @@ -67,7 +67,9 @@
>  #define U300_WDOG_IFR_WILL_BARK_IRQ_FORCE_ENABLE			0x0001U
>  
>  /* Default timeout in seconds = 1 minute */
> -static unsigned int margin = 60;
> +#define U300_WDOG_DEFAULT_TIMEOUT					60
> +
> +static unsigned int margin;

I just realized that 'margin' instead of the actual timeout is used
at the end of the probe function to display the selected timeout.
This will have to change as well (and I'll have to go back to the
other patches to make sure that this doesn't happen there as well).

Guenter

>  static int irq;
>  static void __iomem *virtbase;
>  static struct device *parent;
> @@ -235,8 +237,9 @@ static struct watchdog_device coh901327_wdt = {
>  	 * timeout register is max
>  	 * 0x7FFF = 327670ms ~= 327s.
>  	 */
> -	.min_timeout = 0,
> +	.min_timeout = 1,
>  	.max_timeout = 327,
> +	.timeout = U300_WDOG_DEFAULT_TIMEOUT,
>  };
>  
>  static int __exit coh901327_remove(struct platform_device *pdev)
> @@ -315,9 +318,7 @@ static int __init coh901327_probe(struct platform_device *pdev)
>  		goto out_no_irq;
>  	}
>  
> -	ret = watchdog_init_timeout(&coh901327_wdt, margin, dev);
> -	if (ret < 0)
> -		coh901327_wdt.timeout = 60;
> +	watchdog_init_timeout(&coh901327_wdt, margin, dev);
>  
>  	coh901327_wdt.parent = dev;
>  	ret = watchdog_register_device(&coh901327_wdt);