Message-ID: <20131202161811.GA31606@roeck-us.net>
Date:	Mon, 2 Dec 2013 08:18:11 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	leroy christophe <christophe.leroy@....fr>
Cc:	Wim Van Sebroeck <wim@...ana.be>, scottwood@...escale.com,
	linux-kernel@...r.kernel.org, linux-watchdog@...r.kernel.org,
	linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH] watchdog: mpc8xxx_wdt convert to watchdog core

On Mon, Dec 02, 2013 at 11:31:01AM +0100, leroy christophe wrote:
> 
> Le 01/12/2013 20:38, Guenter Roeck a écrit :
> >On 11/30/2013 07:33 AM, Christophe Leroy wrote:
> >>Convert mpc8xxx_wdt.c to the new watchdog API.
> >>
> >>Signed-off-by: Christophe Leroy <christophe.leroy@....fr>
> >>
> >>diff -ur a/drivers/watchdog/mpc8xxx_wdt.c
> >>b/drivers/watchdog/mpc8xxx_wdt.c
> >>--- a/drivers/watchdog/mpc8xxx_wdt.c    2013-05-11
> >>22:57:46.000000000 +0200
> >>+++ b/drivers/watchdog/mpc8xxx_wdt.c    2013-11-30
> >>16:14:53.803495472 +0100
> >>@@ -72,28 +72,31 @@
> >>   * to 0
> >>   */
> >>  static int prescale = 1;
> >>-static unsigned int timeout_sec;
> >>
> >>-static unsigned long wdt_is_open;
> >>  static DEFINE_SPINLOCK(wdt_spinlock);
> 
_> >>-static void mpc8xxxwdt_keepalive(void)
> >>+static int mpc8xxx_wdt_ping(struct watchdog_device *w)
> >>  {
> >>      /* Ping the WDT */
> >>      spin_lock(&wdt_spinlock);
> >>      out_be16(&wd_base->swsrr, 0x556c);
> >>      out_be16(&wd_base->swsrr, 0xaa39);
> >>      spin_unlock(&wdt_spinlock);
> >>+    return 0;
> >
> >The return code is never checked, so you can make this function void.
> watchdog .h expects an int function.
> Wouldn't it be an error to make it void, if for instance in the
> future the return code is checked by the core ?

This would be correct if the watchdog core would ever call this function,
which it could only do if it was specified in mpc8xxx_wdt_ops, which it isn't.
As written, mpc8xxx_wdt_ping is only called from mpc8xxx_wdt_timer_ping.

If this is not intentional, ie if mpc8xxx_wdt_ping is supposed to be called
from the infrastructure, something else is wrong.

Guenter

> >
> >>  }
> >>
> >>+static struct watchdog_device mpc8xxx_wdt_dev;
> >>  static void mpc8xxx_wdt_timer_ping(unsigned long arg);
> >>-static DEFINE_TIMER(wdt_timer, mpc8xxx_wdt_timer_ping, 0, 0);
> >>+static DEFINE_TIMER(wdt_timer, mpc8xxx_wdt_timer_ping, 0,
> >>+        (unsigned long)&mpc8xxx_wdt_dev);
> >>
> >>  static void mpc8xxx_wdt_timer_ping(unsigned long arg)
> >>  {
> >>-    mpc8xxx_wdt_keepalive();
> >>+    struct watchdog_device *w = (struct watchdog_device *)arg;
> >>+
> >>+    mpc8xxx_wdt_ping(w);
> >>      /* We're pinging it twice faster than needed, just to be sure. */
> >>-    mod_timer(&wdt_timer, jiffies + HZ * timeout_sec / 2);
> >>+    mod_timer(&wdt_timer, jiffies + HZ * w->timeout / 2);
> >>  }
> >>
> >>  static void mpc8xxx_wdt_pr_warn(const char *msg)
> >>@@ -102,19 +105,9 @@
> >>          reset ? "reset" : "machine check exception");
> >>  }
> >>
> >>-static ssize_t mpc8xxx_wdt_write(struct file *file, const char
> >>__user *buf,
> >>-                 size_t count, loff_t *ppos)
> >>-{
> >>-    if (count)
> >>-        mpc8xxx_wdt_keepalive();
> >>-    return count;
> >>-}
> >>-
> >>-static int mpc8xxx_wdt_open(struct inode *inode, struct file *file)
> >>+static int mpc8xxx_wdt_start(struct watchdog_device *w)
> >>  {
> >>      u32 tmp = SWCRR_SWEN;
> >>-    if (test_and_set_bit(0, &wdt_is_open))
> >>-        return -EBUSY;
> >>
> >>      /* Once we start the watchdog we can't stop it */
> >>      if (nowayout)
> >
> >This code and the subsequent module_get can be removed; it is
> >handled by the infrastructure.
> >
> >
> >>@@ -132,59 +125,30 @@
> >>
> >>      del_timer_sync(&wdt_timer);
> >>
> >>-    return nonseekable_open(inode, file);
> >>+    return 0;
> >>  }
> >>
> >>-static int mpc8xxx_wdt_release(struct inode *inode, struct file *file)
> >>+static int mpc8xxx_wdt_stop(struct watchdog_device *w)
> >>  {
> >>-    if (!nowayout)
> >>-        mpc8xxx_wdt_timer_ping(0);
> >>-    else
> >>-        mpc8xxx_wdt_pr_warn("watchdog closed");
> >>-    clear_bit(0, &wdt_is_open);
> >>+    mpc8xxx_wdt_timer_ping((unsigned long)w);
> >
> >I really dislike this typecasting back and forth.
> >
> >I think it would be better to move the mod_timer() call from
> >mpc8xxx_wdt_timer_ping
> >into mpc8xxx_wdt_ping and call mpc8xxx_wdt_ping directly whenever
> >possible.
> >From mpc8xxx_wdt_timer_ping you can then just call
> >mpc8xxx_wdt_ping with the
> >typecasted parameter.
> I'm not sure I understand what you mean.
> mpc8xxx_wdt_ping() is called by the core when the user app writes
> into /dev/watchdog.
> We don't want the set the timer in that case.
> The timer is only used for when no user app has opened /dev/watchdog yet.
> 
> >
> >>      return 0;
> >>  }
> >>
> >>-static long mpc8xxx_wdt_ioctl(struct file *file, unsigned int cmd,
> >>-                            unsigned long arg)
> >>-{
> >>-    void __user *argp = (void __user *)arg;
> >>-    int __user *p = argp;
> >>-    static const struct watchdog_info ident = {
> >>-        .options = WDIOF_KEEPALIVEPING,
> >>-        .firmware_version = 1,
> >>-        .identity = "MPC8xxx",
> >>-    };
> >>-
> >>-    switch (cmd) {
> >>-    case WDIOC_GETSUPPORT:
> >>-        return copy_to_user(argp, &ident, sizeof(ident)) ? -EFAULT : 0;
> >>-    case WDIOC_GETSTATUS:
> >>-    case WDIOC_GETBOOTSTATUS:
> >>-        return put_user(0, p);
> >>-    case WDIOC_KEEPALIVE:
> >>-        mpc8xxx_wdt_keepalive();
> >>-        return 0;
> >>-    case WDIOC_GETTIMEOUT:
> >>-        return put_user(timeout_sec, p);
> >>-    default:
> >>-        return -ENOTTY;
> >>-    }
> >>-}
> >>+static struct watchdog_info mpc8xxx_wdt_info = {
> >>+    .options = WDIOF_KEEPALIVEPING,
> >>+    .firmware_version = 1,
> >>+    .identity = "MPC8xxx",
> >>+};
> >>
> >>-static const struct file_operations mpc8xxx_wdt_fops = {
> >>-    .owner        = THIS_MODULE,
> >>-    .llseek        = no_llseek,
> >>-    .write        = mpc8xxx_wdt_write,
> >>-    .unlocked_ioctl    = mpc8xxx_wdt_ioctl,
> >>-    .open        = mpc8xxx_wdt_open,
> >>-    .release    = mpc8xxx_wdt_release,
> >>+static struct watchdog_ops mpc8xxx_wdt_ops = {
> >>+    .owner = THIS_MODULE,
> >>+    .start = mpc8xxx_wdt_start,
> >>+    .stop = mpc8xxx_wdt_stop,
> >>  };
> >>
> >>-static struct miscdevice mpc8xxx_wdt_miscdev = {
> >>-    .minor    = WATCHDOG_MINOR,
> >>-    .name    = "watchdog",
> >>-    .fops    = &mpc8xxx_wdt_fops,
> >>+static struct watchdog_device mpc8xxx_wdt_dev = {
> >>+    .info = &mpc8xxx_wdt_info,
> >>+    .ops = &mpc8xxx_wdt_ops,
> >>  };
> >>
> >>  static const struct of_device_id mpc8xxx_wdt_match[];
> >>@@ -196,6 +160,7 @@
> >>      const struct mpc8xxx_wdt_type *wdt_type;
> >>      u32 freq = fsl_get_sys_freq();
> >>      bool enabled;
> >>+    unsigned int timeout_sec;
> >>
> >>      match = of_match_device(mpc8xxx_wdt_match, &ofdev->dev);
> >>      if (!match)
> >>@@ -222,6 +187,7 @@
> >>      else
> >>          timeout_sec = timeout / freq;
> >>
> >>+    mpc8xxx_wdt_dev.timeout = timeout_sec;
> >>  #ifdef MODULE
> >>      ret = mpc8xxx_wdt_init_late();
> >>      if (ret)
> >>@@ -237,7 +203,7 @@
> >>       * userspace handles it.
> >>       */
> >>      if (enabled)
> >>-        mpc8xxx_wdt_timer_ping(0);
> >>+        mpc8xxx_wdt_timer_ping((unsigned long)&mpc8xxx_wdt_dev);
> >>      return 0;
> >>  err_unmap:
> >>      iounmap(wd_base);
> >>@@ -249,7 +215,7 @@
> >>  {
> >>      mpc8xxx_wdt_pr_warn("watchdog removed");
> >
> >The mpc8xxx_wdt_pr_warn function is now rather unnecessary since
> >it is called only once. Might as well merge it into this function.
> >
> >>      del_timer_sync(&wdt_timer);
> >>-    misc_deregister(&mpc8xxx_wdt_miscdev);
> >>+    watchdog_unregister_device(&mpc8xxx_wdt_dev);
> >>      iounmap(wd_base);
> >>
> >>      return 0;
> >>@@ -301,10 +267,11 @@
> >>      if (!wd_base)
> >>          return -ENODEV;
> >>
> >>-    ret = misc_register(&mpc8xxx_wdt_miscdev);
> >>+    watchdog_set_nowayout(&mpc8xxx_wdt_dev, nowayout);
> >>+
> >>+    ret = watchdog_register_device(&mpc8xxx_wdt_dev);
> >>      if (ret) {
> >>-        pr_err("cannot register miscdev on minor=%d (err=%d)\n",
> >>-               WATCHDOG_MINOR, ret);
> >>+        pr_err("cannot register watchdog device (err=%d)\n", ret);
> >>          return ret;
> >>      }
> >>      return 0;
> >>diff -ur a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> >>--- a/drivers/watchdog/Kconfig
> >>+++ b/drivers/watchdog/Kconfig
> >>@@ -1146,6 +1146,7 @@
> >>  config 8xxx_WDT
> >>      tristate "MPC8xxx Platform Watchdog Timer"
> >>      depends on PPC_8xx || PPC_83xx || PPC_86xx
> >>+    select WATCHDOG_CORE
> >>      help
> >>        This driver is for a SoC level watchdog that exists on some
> >>        Freescale PowerPC processors. So far this driver supports:
> >>
> >>---
> >>Ce courrier électronique ne contient aucun virus ou logiciel
> >>malveillant parce que la protection avast! Antivirus est active.
> >>http://www.avast.com
> >>
> >>-- 
> >>To unsubscribe from this list: send the line "unsubscribe
> >>linux-watchdog" in
> >>the body of a message to majordomo@...r.kernel.org
> >>More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >>
> >
> 
> 
--
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