linux-kernel - Re: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init

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 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:   Fri, 17 Feb 2017 06:48:36 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Alexandre Belloni <alexandre.belloni@...e-electrons.com>
Cc:     Wim Van Sebroeck <wim@...ana.be>,
        Nicolas Ferre <nicolas.ferre@...el.com>,
        linux-watchdog@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init

On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
> .config is used to cache a part of WDT_MR at probe time and is not used
> afterwards. Also functions are used while they always return 0. Simplify
> the flow by having everything in .probe().
>

Does that really improve anything ? It makes the code harder to read
and, ultimately, the dropped value in sama5d4_wdt is added back in a
later patch as 'mr'.

Guenter

> Signed-off-by: Alexandre Belloni <alexandre.belloni@...e-electrons.com>
> ---
> Changes in v2:
>  - completely get rid of .config instead of caching it
>  - merge init functions in probe()
>
>  drivers/watchdog/sama5d4_wdt.c | 92 ++++++++++++++++--------------------------
>  1 file changed, 34 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index a49634cdc1cc..2c6f5a70ae67 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -28,7 +28,6 @@
>  struct sama5d4_wdt {
>  	struct watchdog_device	wdd;
>  	void __iomem		*reg_base;
> -	u32	config;
>  };
>
>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> @@ -128,57 +127,15 @@ static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>
> -static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
> -{
> -	const char *tmp;
> -
> -	wdt->config = AT91_WDT_WDDIS;
> -
> -	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> -	    !strcmp(tmp, "software"))
> -		wdt->config |= AT91_WDT_WDFIEN;
> -	else
> -		wdt->config |= AT91_WDT_WDRSTEN;
> -
> -	if (of_property_read_bool(np, "atmel,idle-halt"))
> -		wdt->config |= AT91_WDT_WDIDLEHLT;
> -
> -	if (of_property_read_bool(np, "atmel,dbg-halt"))
> -		wdt->config |= AT91_WDT_WDDBGHLT;
> -
> -	return 0;
> -}
> -
> -static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> -{
> -	struct watchdog_device *wdd = &wdt->wdd;
> -	u32 value = WDT_SEC2TICKS(wdd->timeout);
> -	u32 reg;
> -
> -	/*
> -	 * Because the fields WDV and WDD must not be modified when the WDDIS
> -	 * bit is set, so clear the WDDIS bit before writing the WDT_MR.
> -	 */
> -	reg = wdt_read(wdt, AT91_WDT_MR);
> -	reg &= ~AT91_WDT_WDDIS;
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> -
> -	reg = wdt->config;
> -	reg |= AT91_WDT_SET_WDD(value);
> -	reg |= AT91_WDT_SET_WDV(value);
> -
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> -
> -	return 0;
> -}
> -
>  static int sama5d4_wdt_probe(struct platform_device *pdev)
>  {
>  	struct watchdog_device *wdd;
>  	struct sama5d4_wdt *wdt;
>  	struct resource *res;
>  	void __iomem *regs;
> -	u32 irq = 0;
> +	struct device_node *np = pdev->dev.of_node;
> +	const char *tmp;
> +	u32 mr, reg, irq = 0;
>  	int ret;
>
>  	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> @@ -201,17 +158,26 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>
>  	wdt->reg_base = regs;
>
> -	if (pdev->dev.of_node) {
> -		irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> -		if (!irq)
> -			dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
> +	irq = irq_of_parse_and_map(np, 0);
> +	if (!irq)
> +		dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
>
> -		ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt);
> -		if (ret)
> -			return ret;
> -	}
>
> -	if ((wdt->config & AT91_WDT_WDFIEN) && irq) {
> +	mr = AT91_WDT_WDDIS;
> +
> +	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> +	    !strcmp(tmp, "software"))
> +		mr |= AT91_WDT_WDFIEN;
> +	else
> +		mr |= AT91_WDT_WDRSTEN;
> +
> +	if (of_property_read_bool(np, "atmel,idle-halt"))
> +		mr |= AT91_WDT_WDIDLEHLT;
> +
> +	if (of_property_read_bool(np, "atmel,dbg-halt"))
> +		mr |= AT91_WDT_WDDBGHLT;
> +
> +	if ((mr & AT91_WDT_WDFIEN) && irq) {
>  		ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
>  				       IRQF_SHARED | IRQF_IRQPOLL |
>  				       IRQF_NO_SUSPEND, pdev->name, pdev);
> @@ -228,9 +194,19 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>
> -	ret = sama5d4_wdt_init(wdt);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * WDV and WDD must not be modified when the WDDIS bit is set, so clear
> +	 * the WDDIS bit before writing the WDT_MR.
> +	 */
> +	reg = wdt_read(wdt, AT91_WDT_MR);
> +	reg &= ~AT91_WDT_WDDIS;
> +	wdt_write(wdt, AT91_WDT_MR, reg);
> +
> +	reg = mr;
> +	reg |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(wdd->timeout));
> +	reg |= AT91_WDT_SET_WDV(WDT_SEC2TICKS(wdd->timeout));
> +
> +	wdt_write(wdt, AT91_WDT_MR, reg);
>
>  	watchdog_set_nowayout(wdd, nowayout);
>
>