Date:	Tue, 24 Nov 2015 10:21:38 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	Simon Arlott <simon@...e.lp0.eu>,
	Florian Fainelli <f.fainelli@...il.com>
Cc:	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Ralf Baechle <ralf@...ux-mips.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	Marc Zyngier <marc.zyngier@....com>,
	Kevin Cernekee <cernekee@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Wim Van Sebroeck <wim@...ana.be>,
	Maxime Bizon <mbizon@...ebox.fr>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-mips@...ux-mips.org, linux-watchdog@...r.kernel.org,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Jonas Gorski <jogo@...nwrt.org>
Subject: Re: [PATCH 4/10] watchdog: bcm63xx_wdt: Handle hardware interrupt
 and remove software timer

On Sun, Nov 22, 2015 at 02:05:16PM +0000, Simon Arlott wrote:
> There is a level triggered interrupt for the watchdog timer as part of
> the bcm63xx_timer device. The interrupt occurs when the hardware watchdog
> timer reaches 50% of the remaining time.
> 
> It is not possible to mask the interrupt within the bcm63xx_timer device.
> To get around this limitation, handle the interrupt by restarting the
> watchdog with the current remaining time (which will be half the previous
> timeout) so that the interrupt occurs again at 1/4th, 1/8th, etc. of the
> original timeout value until the watchdog forces a reboot.
> 
> The software timer was restarting the hardware watchdog with a 85 second
> timeout until the software timer expired, and then causing a panic()
> about 42.5 seconds later when the hardware interrupt occurred. The
> hardware watchdog would not reboot until a further 42.5 seconds had
> passed.
> 
> Remove the software timer and rely on the hardware timer directly,
> reducing the maximum timeout from 256 seconds to 85 seconds
> (2^32 / WDT_HZ).
> 

Florian,

can you have a look into this patch and confirm that there is no better
way to clear the interrupt status ?

Thanks,
Guenter

> Signed-off-by: Simon Arlott <simon@...e.lp0.eu>
> ---
>  drivers/watchdog/bcm63xx_wdt.c | 124 ++++++++++++++++++++++++-----------------
>  1 file changed, 72 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/watchdog/bcm63xx_wdt.c b/drivers/watchdog/bcm63xx_wdt.c
> index ab26fd9..f88fc97 100644
> --- a/drivers/watchdog/bcm63xx_wdt.c
> +++ b/drivers/watchdog/bcm63xx_wdt.c
> @@ -3,6 +3,7 @@
>   *
>   *  Copyright (C) 2007, Miguel Gaio <miguel.gaio@...xo.com>
>   *  Copyright (C) 2008, Florian Fainelli <florian@...nwrt.org>
> + *  Copyright 2015 Simon Arlott
>   *
>   *  This program is free software; you can redistribute it and/or
>   *  modify it under the terms of the GNU General Public License
> @@ -20,11 +21,10 @@
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/spinlock.h>
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  #include <linux/watchdog.h>
> -#include <linux/timer.h>
> -#include <linux/jiffies.h>
>  #include <linux/interrupt.h>
>  #include <linux/ptrace.h>
>  #include <linux/resource.h>
> @@ -37,16 +37,17 @@
>  
>  #define PFX KBUILD_MODNAME
>  
> -#define WDT_HZ		50000000 /* Fclk */
> -#define WDT_DEFAULT_TIME	30      /* seconds */
> -#define WDT_MAX_TIME		256     /* seconds */
> +#define WDT_HZ			50000000		/* Fclk */
> +#define WDT_DEFAULT_TIME	30			/* seconds */
> +#define WDT_MAX_TIME		(0xffffffff / WDT_HZ)	/* seconds */
>  
> -static struct {
> +struct bcm63xx_wdt_hw {
> +	raw_spinlock_t lock;
>  	void __iomem *regs;
> -	struct timer_list timer;
>  	unsigned long inuse;
> -	atomic_t ticks;
> -} bcm63xx_wdt_device;
> +	bool running;
> +};
> +static struct bcm63xx_wdt_hw bcm63xx_wdt_device;
>  
>  static int expect_close;
>  
> @@ -59,48 +60,67 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>  /* HW functions */
>  static void bcm63xx_wdt_hw_start(void)
>  {
> -	bcm_writel(0xfffffffe, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&bcm63xx_wdt_device.lock, flags);
> +	bcm_writel(wdt_time * WDT_HZ, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
>  	bcm_writel(WDT_START_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
>  	bcm_writel(WDT_START_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
> +	bcm63xx_wdt_device.running = true;
> +	raw_spin_unlock_irqrestore(&bcm63xx_wdt_device.lock, flags);
>  }
>  
>  static void bcm63xx_wdt_hw_stop(void)
>  {
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&bcm63xx_wdt_device.lock, flags);
>  	bcm_writel(WDT_STOP_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
>  	bcm_writel(WDT_STOP_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
> +	bcm63xx_wdt_device.running = false;
> +	raw_spin_unlock_irqrestore(&bcm63xx_wdt_device.lock, flags);
>  }
>  
> +/* The watchdog interrupt occurs when half the timeout is remaining */
>  static void bcm63xx_wdt_isr(void *data)
>  {
> -	struct pt_regs *regs = get_irq_regs();
> -
> -	die(PFX " fire", regs);
> -}
> -
> -static void bcm63xx_timer_tick(unsigned long unused)
> -{
> -	if (!atomic_dec_and_test(&bcm63xx_wdt_device.ticks)) {
> -		bcm63xx_wdt_hw_start();
> -		mod_timer(&bcm63xx_wdt_device.timer, jiffies + HZ);
> -	} else
> -		pr_crit("watchdog will restart system\n");
> -}
> -
> -static void bcm63xx_wdt_pet(void)
> -{
> -	atomic_set(&bcm63xx_wdt_device.ticks, wdt_time);
> -}
> -
> -static void bcm63xx_wdt_start(void)
> -{
> -	bcm63xx_wdt_pet();
> -	bcm63xx_timer_tick(0);
> -}
> +	struct bcm63xx_wdt_hw *hw = &bcm63xx_wdt_device;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&hw->lock, flags);
> +	if (!hw->running) {
> +		/* Stop the watchdog as it shouldn't be running */
> +		bcm_writel(WDT_STOP_1, hw->regs + WDT_CTL_REG);
> +		bcm_writel(WDT_STOP_2, hw->regs + WDT_CTL_REG);
> +	} else {
> +		u32 timeleft = bcm_readl(hw->regs + WDT_CTL_REG);
> +		u32 ms;
> +
> +		if (timeleft >= 2) {
> +			/* The only way to clear this level triggered interrupt
> +			 * without disrupting the normal running of the watchdog
> +			 * is to restart the watchdog with the current remaining
> +			 * time value (which will be half the previous timeout)
> +			 * so the interrupt occurs again at 1/4th, 1/8th, etc.
> +			 * of the original timeout value until we reboot.
> +			 *
> +			 * This is done with a lock held in case userspace is
> +			 * trying to restart the watchdog on another CPU.
> +			 */
> +			bcm_writel(timeleft, hw->regs + WDT_DEFVAL_REG);
> +			bcm_writel(WDT_START_1, hw->regs + WDT_CTL_REG);
> +			bcm_writel(WDT_START_2, hw->regs + WDT_CTL_REG);
> +		} else {
> +			/* The watchdog cannot be started with a time of less
> +			 * than 2 ticks (it won't fire).
> +			 */
> +			die(PFX ": watchdog timer expired\n", get_irq_regs());
> +		}
>  
> -static void bcm63xx_wdt_pause(void)
> -{
> -	del_timer_sync(&bcm63xx_wdt_device.timer);
> -	bcm63xx_wdt_hw_stop();
> +		ms = timeleft / (WDT_HZ / 1000);
> +		pr_alert("warning timer fired, reboot in %ums\n", ms);
> +	}
> +	raw_spin_unlock_irqrestore(&hw->lock, flags);
>  }
>  
>  static int bcm63xx_wdt_settimeout(int new_time)
> @@ -118,17 +138,17 @@ static int bcm63xx_wdt_open(struct inode *inode, struct file *file)
>  	if (test_and_set_bit(0, &bcm63xx_wdt_device.inuse))
>  		return -EBUSY;
>  
> -	bcm63xx_wdt_start();
> +	bcm63xx_wdt_hw_start();
>  	return nonseekable_open(inode, file);
>  }
>  
>  static int bcm63xx_wdt_release(struct inode *inode, struct file *file)
>  {
>  	if (expect_close == 42)
> -		bcm63xx_wdt_pause();
> +		bcm63xx_wdt_hw_stop();
>  	else {
>  		pr_crit("Unexpected close, not stopping watchdog!\n");
> -		bcm63xx_wdt_start();
> +		bcm63xx_wdt_hw_start();
>  	}
>  	clear_bit(0, &bcm63xx_wdt_device.inuse);
>  	expect_close = 0;
> @@ -153,7 +173,7 @@ static ssize_t bcm63xx_wdt_write(struct file *file, const char *data,
>  					expect_close = 42;
>  			}
>  		}
> -		bcm63xx_wdt_pet();
> +		bcm63xx_wdt_hw_start();
>  	}
>  	return len;
>  }
> @@ -187,18 +207,18 @@ static long bcm63xx_wdt_ioctl(struct file *file, unsigned int cmd,
>  			return -EFAULT;
>  
>  		if (new_value & WDIOS_DISABLECARD) {
> -			bcm63xx_wdt_pause();
> +			bcm63xx_wdt_hw_stop();
>  			retval = 0;
>  		}
>  		if (new_value & WDIOS_ENABLECARD) {
> -			bcm63xx_wdt_start();
> +			bcm63xx_wdt_hw_start();
>  			retval = 0;
>  		}
>  
>  		return retval;
>  
>  	case WDIOC_KEEPALIVE:
> -		bcm63xx_wdt_pet();
> +		bcm63xx_wdt_hw_start();
>  		return 0;
>  
>  	case WDIOC_SETTIMEOUT:
> @@ -208,7 +228,7 @@ static long bcm63xx_wdt_ioctl(struct file *file, unsigned int cmd,
>  		if (bcm63xx_wdt_settimeout(new_value))
>  			return -EINVAL;
>  
> -		bcm63xx_wdt_pet();
> +		bcm63xx_wdt_hw_start();
>  
>  	case WDIOC_GETTIMEOUT:
>  		return put_user(wdt_time, p);
> @@ -240,8 +260,6 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
>  	int ret;
>  	struct resource *r;
>  
> -	setup_timer(&bcm63xx_wdt_device.timer, bcm63xx_timer_tick, 0L);
> -
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!r) {
>  		dev_err(&pdev->dev, "failed to get resources\n");
> @@ -255,6 +273,8 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
>  		return -ENXIO;
>  	}
>  
> +	raw_spin_lock_init(&bcm63xx_wdt_device.lock);
> +
>  	ret = bcm63xx_timer_register(TIMER_WDT_ID, bcm63xx_wdt_isr, NULL);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to register wdt timer isr\n");
> @@ -264,8 +284,8 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
>  	if (bcm63xx_wdt_settimeout(wdt_time)) {
>  		bcm63xx_wdt_settimeout(WDT_DEFAULT_TIME);
>  		dev_info(&pdev->dev,
> -			": wdt_time value must be 1 <= wdt_time <= 256, using %d\n",
> -			wdt_time);
> +			": wdt_time value must be 1 <= wdt_time <= %d, using %d\n",
> +			WDT_MAX_TIME, wdt_time);
>  	}
>  
>  	ret = misc_register(&bcm63xx_wdt_miscdev);
> @@ -287,7 +307,7 @@ unregister_timer:
>  static int bcm63xx_wdt_remove(struct platform_device *pdev)
>  {
>  	if (!nowayout)
> -		bcm63xx_wdt_pause();
> +		bcm63xx_wdt_hw_stop();
>  
>  	misc_deregister(&bcm63xx_wdt_miscdev);
>  	bcm63xx_timer_unregister(TIMER_WDT_ID);
> @@ -296,7 +316,7 @@ static int bcm63xx_wdt_remove(struct platform_device *pdev)
>  
>  static void bcm63xx_wdt_shutdown(struct platform_device *pdev)
>  {
> -	bcm63xx_wdt_pause();
> +	bcm63xx_wdt_hw_stop();
>  }
>  
>  static struct platform_driver bcm63xx_wdt_driver = {
> -- 
> 2.1.4
> 
> -- 
> Simon Arlott
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists