On Thu, Jun 11, 2020 at 09:17:48PM +0200, Ahmad Fatoum wrote: > Migrating the driver lets us drop the watchdog misc device boilerplate > and reduce size by 270~ lines. It also brings us support for new > functionality like CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED. > > This changes behavior on module unloading: Whereas before, the watchdog > was automatically stopped on module unload, now explicit stopping of > the watchdog before unload is required. >
Any chance you can convert the driver to a platform device ? Thanks, Guenter > Signed-off-by: Ahmad Fatoum <[email protected]> > --- > drivers/watchdog/Kconfig | 1 + > drivers/watchdog/f71808e_wdt.c | 497 ++++++++------------------------- > 2 files changed, 111 insertions(+), 387 deletions(-) > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 0663c604bd64..c312dd5fc0ca 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -1052,6 +1052,7 @@ config EBC_C384_WDT > config F71808E_WDT > tristate "Fintek F718xx, F818xx Super I/O Watchdog" > depends on X86 > + select WATCHDOG_CORE > help > This is the driver for the hardware watchdog on the Fintek F71808E, > F71862FG, F71868, F71869, F71882FG, F71889FG, F81803, F81865, and > diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c > index 9299ad4d4a52..7c42cbf9912e 100644 > --- a/drivers/watchdog/f71808e_wdt.c > +++ b/drivers/watchdog/f71808e_wdt.c > @@ -9,16 +9,10 @@ > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > #include <linux/err.h> > -#include <linux/fs.h> > #include <linux/init.h> > #include <linux/io.h> > #include <linux/ioport.h> > -#include <linux/miscdevice.h> > #include <linux/module.h> > -#include <linux/mutex.h> > -#include <linux/notifier.h> > -#include <linux/reboot.h> > -#include <linux/uaccess.h> > #include <linux/watchdog.h> > > #define DRVNAME "f71808e_wdt" > @@ -129,23 +123,15 @@ struct f71808e_variant { > }; > > struct watchdog_data { > + struct watchdog_device wdd; > unsigned short sioaddr; > const struct f71808e_variant *variant; > - unsigned long opened; > - struct mutex lock; > - char expect_close; > struct watchdog_info ident; > > - unsigned short timeout; > u8 timer_val; /* content for the wd_time register */ > char minutes_mode; > u8 pulse_val; /* pulse width flag */ > char pulse_mode; /* enable pulse output mode? */ > - char caused_reboot; /* last reboot was by the watchdog */ > -}; > - > -static struct watchdog_data watchdog = { > - .lock = __MUTEX_INITIALIZER(watchdog.lock), > }; > > static inline bool has_f81865_wdo_conf(struct watchdog_data *wd) > @@ -216,487 +202,225 @@ static inline void superio_exit(int base) > release_region(base, 2); > } > > -static int watchdog_set_timeout(int timeout) > +static int watchdog_set_timeout(struct watchdog_device *wdd, unsigned int > new_timeout) > { > - if (timeout <= 0 > - || timeout > max_timeout) { > - pr_err("watchdog timeout out of range\n"); > - return -EINVAL; > - } > + struct watchdog_data *wd = watchdog_get_drvdata(wdd); > > - mutex_lock(&watchdog.lock); > - > - watchdog.timeout = timeout; > - if (timeout > 0xff) { > - watchdog.timer_val = DIV_ROUND_UP(timeout, 60); > - watchdog.minutes_mode = true; > + wdd->timeout = new_timeout; > + if (new_timeout > 0xff) { > + wd->timer_val = DIV_ROUND_UP(new_timeout, 60); > + wd->minutes_mode = true; > } else { > - watchdog.timer_val = timeout; > - watchdog.minutes_mode = false; > + wd->timer_val = new_timeout; > + wd->minutes_mode = false; > } > > - mutex_unlock(&watchdog.lock); > - > return 0; > } > > -static int watchdog_set_pulse_width(unsigned int pw) > +static int watchdog_set_pulse_width(struct watchdog_data *wd, unsigned int > pw) > { > - int err = 0; > unsigned int t1 = 25, t2 = 125, t3 = 5000; > > - if (watchdog.variant->id == SIO_F71868_ID) { > + if (wd->variant->id == SIO_F71868_ID) { > t1 = 30; > t2 = 150; > t3 = 6000; > } > > - mutex_lock(&watchdog.lock); > - > if (pw <= 1) { > - watchdog.pulse_val = 0; > + wd->pulse_val = 0; > } else if (pw <= t1) { > - watchdog.pulse_val = 1; > + wd->pulse_val = 1; > } else if (pw <= t2) { > - watchdog.pulse_val = 2; > + wd->pulse_val = 2; > } else if (pw <= t3) { > - watchdog.pulse_val = 3; > + wd->pulse_val = 3; > } else { > pr_err("pulse width out of range\n"); > - err = -EINVAL; > - goto exit_unlock; > + return -EINVAL; > } > > - watchdog.pulse_mode = pw; > + wd->pulse_mode = pw; > > -exit_unlock: > - mutex_unlock(&watchdog.lock); > - return err; > + return 0; > } > > -static int watchdog_keepalive(void) > +static int watchdog_keepalive(struct watchdog_device *wdd) > { > - int err = 0; > + struct watchdog_data *wd = watchdog_get_drvdata(wdd); > + int err; > > - mutex_lock(&watchdog.lock); > - err = superio_enter(watchdog.sioaddr); > + err = superio_enter(wd->sioaddr); > if (err) > - goto exit_unlock; > - superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT); > + return err; > + superio_select(wd->sioaddr, SIO_F71808FG_LD_WDT); > > - if (watchdog.minutes_mode) > + if (wd->minutes_mode) > /* select minutes for timer units */ > - superio_set_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF, > + superio_set_bit(wd->sioaddr, F71808FG_REG_WDT_CONF, > F71808FG_FLAG_WD_UNIT); > else > /* select seconds for timer units */ > - superio_clear_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF, > + superio_clear_bit(wd->sioaddr, F71808FG_REG_WDT_CONF, > F71808FG_FLAG_WD_UNIT); > > /* Set timer value */ > - superio_outb(watchdog.sioaddr, F71808FG_REG_WD_TIME, > - watchdog.timer_val); > + superio_outb(wd->sioaddr, F71808FG_REG_WD_TIME, > + wd->timer_val); > > - superio_exit(watchdog.sioaddr); > + superio_exit(wd->sioaddr); > > -exit_unlock: > - mutex_unlock(&watchdog.lock); > - return err; > + return 0; > } > > -static int watchdog_start(void) > +static int watchdog_start(struct watchdog_device *wdd) > { > + struct watchdog_data *wd = watchdog_get_drvdata(wdd); > int err; > > /* Make sure we don't die as soon as the watchdog is enabled below */ > - err = watchdog_keepalive(); > + err = watchdog_keepalive(wdd); > if (err) > return err; > > - mutex_lock(&watchdog.lock); > - err = superio_enter(watchdog.sioaddr); > + err = superio_enter(wd->sioaddr); > if (err) > - goto exit_unlock; > - superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT); > + return err; > + superio_select(wd->sioaddr, SIO_F71808FG_LD_WDT); > > /* Watchdog pin configuration */ > - watchdog.variant->pinconf(&watchdog); > + wd->variant->pinconf(wd); > > - superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT); > - superio_set_bit(watchdog.sioaddr, SIO_REG_ENABLE, 0); > + superio_select(wd->sioaddr, SIO_F71808FG_LD_WDT); > + superio_set_bit(wd->sioaddr, SIO_REG_ENABLE, 0); > > - if (has_f81865_wdo_conf(&watchdog)) > - superio_set_bit(watchdog.sioaddr, F81865_REG_WDO_CONF, > + if (has_f81865_wdo_conf(wd)) > + superio_set_bit(wd->sioaddr, F81865_REG_WDO_CONF, > F81865_FLAG_WDOUT_EN); > else > - superio_set_bit(watchdog.sioaddr, F71808FG_REG_WDO_CONF, > + superio_set_bit(wd->sioaddr, F71808FG_REG_WDO_CONF, > F71808FG_FLAG_WDOUT_EN); > > - superio_set_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF, > + superio_set_bit(wd->sioaddr, F71808FG_REG_WDT_CONF, > F71808FG_FLAG_WD_EN); > > - if (watchdog.pulse_mode) { > + set_bit(WDOG_HW_RUNNING, &wdd->status); > + > + if (wd->pulse_mode) { > /* Select "pulse" output mode with given duration */ > - u8 wdt_conf = superio_inb(watchdog.sioaddr, > - F71808FG_REG_WDT_CONF); > + u8 wdt_conf = superio_inb(wd->sioaddr, F71808FG_REG_WDT_CONF); > > /* Set WD_PSWIDTH bits (1:0) */ > - wdt_conf = (wdt_conf & 0xfc) | (watchdog.pulse_val & 0x03); > + wdt_conf = (wdt_conf & 0xfc) | (wd->pulse_val & 0x03); > /* Set WD_PULSE to "pulse" mode */ > wdt_conf |= BIT(F71808FG_FLAG_WD_PULSE); > > - superio_outb(watchdog.sioaddr, F71808FG_REG_WDT_CONF, > - wdt_conf); > + superio_outb(wd->sioaddr, F71808FG_REG_WDT_CONF, wdt_conf); > } else { > /* Select "level" output mode */ > - superio_clear_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF, > - F71808FG_FLAG_WD_PULSE); > + superio_clear_bit(wd->sioaddr, F71808FG_REG_WDT_CONF, > + F71808FG_FLAG_WD_PULSE); > } > > - superio_exit(watchdog.sioaddr); > -exit_unlock: > - mutex_unlock(&watchdog.lock); > + superio_exit(wd->sioaddr); > > return err; > } > > -static int watchdog_stop(void) > -{ > - int err = 0; > - > - mutex_lock(&watchdog.lock); > - err = superio_enter(watchdog.sioaddr); > - if (err) > - goto exit_unlock; > - superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT); > - > - superio_clear_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF, > - F71808FG_FLAG_WD_EN); > - > - superio_exit(watchdog.sioaddr); > - > -exit_unlock: > - mutex_unlock(&watchdog.lock); > - > - return err; > -} > - > -static int watchdog_get_status(void) > -{ > - int status = 0; > - > - mutex_lock(&watchdog.lock); > - status = (watchdog.caused_reboot) ? WDIOF_CARDRESET : 0; > - mutex_unlock(&watchdog.lock); > - > - return status; > -} > - > -static bool watchdog_is_running(void) > -{ > - /* > - * if we fail to determine the watchdog's status assume it to be > - * running to be on the safe side > - */ > - bool is_running = true; > - > - mutex_lock(&watchdog.lock); > - if (superio_enter(watchdog.sioaddr)) > - goto exit_unlock; > - superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT); > - > - is_running = (superio_inb(watchdog.sioaddr, SIO_REG_ENABLE) & BIT(0)) > - && (superio_inb(watchdog.sioaddr, F71808FG_REG_WDT_CONF) > - & BIT(F71808FG_FLAG_WD_EN)); > - > - superio_exit(watchdog.sioaddr); > - > -exit_unlock: > - mutex_unlock(&watchdog.lock); > - return is_running; > -} > - > -/* /dev/watchdog api */ > - > -static int watchdog_open(struct inode *inode, struct file *file) > +static int watchdog_stop(struct watchdog_device *wdd) > { > + struct watchdog_data *wd = watchdog_get_drvdata(wdd); > int err; > > - /* If the watchdog is alive we don't need to start it again */ > - if (test_and_set_bit(0, &watchdog.opened)) > - return -EBUSY; > - > - err = watchdog_start(); > - if (err) { > - clear_bit(0, &watchdog.opened); > + err = superio_enter(wd->sioaddr); > + if (err) > return err; > - } > + superio_select(wd->sioaddr, SIO_F71808FG_LD_WDT); > > - if (nowayout) > - __module_get(THIS_MODULE); > + superio_clear_bit(wd->sioaddr, F71808FG_REG_WDT_CONF, > + F71808FG_FLAG_WD_EN); > > - watchdog.expect_close = 0; > - return stream_open(inode, file); > -} > - > -static int watchdog_release(struct inode *inode, struct file *file) > -{ > - clear_bit(0, &watchdog.opened); > + superio_exit(wd->sioaddr); > > - if (!watchdog.expect_close) { > - watchdog_keepalive(); > - pr_crit("Unexpected close, not stopping watchdog!\n"); > - } else if (!nowayout) { > - watchdog_stop(); > - } > return 0; > } > > -/* > - * watchdog_write: > - * @file: file handle to the watchdog > - * @buf: buffer to write > - * @count: count of bytes > - * @ppos: pointer to the position to write. No seeks allowed > - * > - * A write to a watchdog device is defined as a keepalive signal. Any > - * write of data will do, as we we don't define content meaning. > - */ > - > -static ssize_t watchdog_write(struct file *file, const char __user *buf, > - size_t count, loff_t *ppos) > -{ > - if (count) { > - if (!nowayout) { > - size_t i; > - > - /* In case it was set long ago */ > - bool expect_close = false; > - > - for (i = 0; i != count; i++) { > - char c; > - if (get_user(c, buf + i)) > - return -EFAULT; > - if (c == 'V') > - expect_close = true; > - } > - > - /* Properly order writes across fork()ed processes */ > - mutex_lock(&watchdog.lock); > - watchdog.expect_close = expect_close; > - mutex_unlock(&watchdog.lock); > - } > - > - /* someone wrote to us, we should restart timer */ > - watchdog_keepalive(); > - } > - return count; > -} > - > -/* > - * watchdog_ioctl: > - * @inode: inode of the device > - * @file: file handle to the device > - * @cmd: watchdog command > - * @arg: argument pointer > - * > - * The watchdog API defines a common set of functions for all watchdogs > - * according to their available features. > - */ > -static long watchdog_ioctl(struct file *file, unsigned int cmd, > - unsigned long arg) > -{ > - int status; > - int new_options; > - int new_timeout; > - union { > - struct watchdog_info __user *ident; > - int __user *i; > - } uarg; > - > - uarg.i = (int __user *)arg; > - > - switch (cmd) { > - case WDIOC_GETSUPPORT: > - return copy_to_user(uarg.ident, &watchdog.ident, > - sizeof(watchdog.ident)) ? -EFAULT : 0; > - > - case WDIOC_GETSTATUS: > - status = watchdog_get_status(); > - if (status < 0) > - return status; > - return put_user(status, uarg.i); > - > - case WDIOC_GETBOOTSTATUS: > - return put_user(0, uarg.i); > - > - case WDIOC_SETOPTIONS: > - if (get_user(new_options, uarg.i)) > - return -EFAULT; > - > - if (new_options & WDIOS_DISABLECARD) > - watchdog_stop(); > - > - if (new_options & WDIOS_ENABLECARD) > - return watchdog_start(); > - /* fall through */ > - > - case WDIOC_KEEPALIVE: > - watchdog_keepalive(); > - return 0; > - > - case WDIOC_SETTIMEOUT: > - if (get_user(new_timeout, uarg.i)) > - return -EFAULT; > - > - if (watchdog_set_timeout(new_timeout)) > - return -EINVAL; > - > - watchdog_keepalive(); > - /* fall through */ > - > - case WDIOC_GETTIMEOUT: > - return put_user(watchdog.timeout, uarg.i); > - > - default: > - return -ENOTTY; > - > - } > -} > +static const struct watchdog_ops f71808e_wdog_ops = { > + .owner = THIS_MODULE, > + .start = watchdog_start, > + .stop = watchdog_stop, > + .ping = watchdog_keepalive, > + .set_timeout = watchdog_set_timeout, > +}; > > -static int watchdog_notify_sys(struct notifier_block *this, unsigned long > code, > - void *unused) > +static bool watchdog_is_running(struct watchdog_data *wd, u8 wdt_conf) > { > - if (code == SYS_DOWN || code == SYS_HALT) > - watchdog_stop(); > - return NOTIFY_DONE; > + return (superio_inb(wd->sioaddr, SIO_REG_ENABLE) & BIT(0)) > + && (wdt_conf & BIT(F71808FG_FLAG_WD_EN)); > } > > -static const struct file_operations watchdog_fops = { > - .owner = THIS_MODULE, > - .llseek = no_llseek, > - .open = watchdog_open, > - .release = watchdog_release, > - .write = watchdog_write, > - .unlocked_ioctl = watchdog_ioctl, > - .compat_ioctl = compat_ptr_ioctl, > -}; > - > -static struct miscdevice watchdog_miscdev = { > - .minor = WATCHDOG_MINOR, > - .name = "watchdog", > - .fops = &watchdog_fops, > -}; > - > -static struct notifier_block watchdog_notifier = { > - .notifier_call = watchdog_notify_sys, > -}; > - > -static int __init watchdog_init(int sioaddr) > +static int __init watchdog_init(struct watchdog_data *wd) > { > + struct watchdog_device *wdd = &wd->wdd; > int wdt_conf, err = 0; > > /* No need to lock watchdog.lock here because no entry points > * into the module have been registered yet. > */ > - watchdog.sioaddr = sioaddr; > - watchdog.ident.options = WDIOF_MAGICCLOSE > + wd->ident.options = WDIOF_SETTIMEOUT > + | WDIOF_MAGICCLOSE > | WDIOF_KEEPALIVEPING > | WDIOF_CARDRESET; > > - snprintf(watchdog.ident.identity, > - sizeof(watchdog.ident.identity), "%s watchdog", > - watchdog.variant->wdt_name); > + snprintf(wd->ident.identity, sizeof(wd->ident.identity), > + "%s watchdog", wd->variant->wdt_name); > > - err = superio_enter(sioaddr); > + err = superio_enter(wd->sioaddr); > if (err) > return err; > - superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT); > + superio_select(wd->sioaddr, SIO_F71808FG_LD_WDT); > > - wdt_conf = superio_inb(sioaddr, F71808FG_REG_WDT_CONF); > - watchdog.caused_reboot = wdt_conf & BIT(F71808FG_FLAG_WDTMOUT_STS); > + wdt_conf = superio_inb(wd->sioaddr, F71808FG_REG_WDT_CONF); > > /* > * We don't want WDTMOUT_STS to stick around till regular reboot. > * Write 1 to the bit to clear it to zero. > */ > - superio_outb(sioaddr, F71808FG_REG_WDT_CONF, > + superio_outb(wd->sioaddr, F71808FG_REG_WDT_CONF, > wdt_conf | BIT(F71808FG_FLAG_WDTMOUT_STS)); > > - superio_exit(sioaddr); > + if (watchdog_is_running(wd, wdt_conf)) > + set_bit(WDOG_HW_RUNNING, &wdd->status); > > - err = watchdog_set_timeout(timeout); > - if (err) > - return err; > - err = watchdog_set_pulse_width(pulse_width); > - if (err) > - return err; > + superio_exit(wd->sioaddr); > > - err = register_reboot_notifier(&watchdog_notifier); > + err = watchdog_set_pulse_width(wd, pulse_width); > if (err) > return err; > > - err = misc_register(&watchdog_miscdev); > - if (err) { > - pr_err("cannot register miscdev on minor=%d\n", > - watchdog_miscdev.minor); > - goto exit_reboot; > - } > - > - if (start_withtimeout) { > - if (start_withtimeout <= 0 > - || start_withtimeout > max_timeout) { > - pr_err("starting timeout out of range\n"); > - err = -EINVAL; > - goto exit_miscdev; > - } > - > - err = watchdog_start(); > - if (err) { > - pr_err("cannot start watchdog timer\n"); > - goto exit_miscdev; > - } > - > - mutex_lock(&watchdog.lock); > - err = superio_enter(sioaddr); > - if (err) > - goto exit_unlock; > - superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT); > - > - if (start_withtimeout > 0xff) { > - /* select minutes for timer units */ > - superio_set_bit(sioaddr, F71808FG_REG_WDT_CONF, > - F71808FG_FLAG_WD_UNIT); > - superio_outb(sioaddr, F71808FG_REG_WD_TIME, > - DIV_ROUND_UP(start_withtimeout, 60)); > - } else { > - /* select seconds for timer units */ > - superio_clear_bit(sioaddr, F71808FG_REG_WDT_CONF, > - F71808FG_FLAG_WD_UNIT); > - superio_outb(sioaddr, F71808FG_REG_WD_TIME, > - start_withtimeout); > - } > - > - superio_exit(sioaddr); > - mutex_unlock(&watchdog.lock); > - > - if (nowayout) > - __module_get(THIS_MODULE); > + wdd->info = &wd->ident; > + wdd->ops = &f71808e_wdog_ops; > + wdd->min_timeout = 1; > + wdd->max_timeout = max_timeout; > + wdd->timeout = timeout; > > - pr_info("watchdog started with initial timeout of %u sec\n", > - start_withtimeout); > - } > + watchdog_set_nowayout(wdd, nowayout); > + watchdog_stop_on_unregister(wdd); > + watchdog_stop_on_reboot(wdd); > + watchdog_init_timeout(wdd, start_withtimeout, NULL); > + watchdog_set_drvdata(wdd, wd); > > - return 0; > + if (wdt_conf & BIT(F71808FG_FLAG_WDTMOUT_STS)) > + wdd->bootstatus = WDIOF_CARDRESET; > > -exit_unlock: > - mutex_unlock(&watchdog.lock); > -exit_miscdev: > - misc_deregister(&watchdog_miscdev); > -exit_reboot: > - unregister_reboot_notifier(&watchdog_notifier); > + /* > + * WATCHDOG_HANDLE_BOOT_ENABLED can result in keepalive being directly > + * called without a set_timeout before, so it needs to be done here once > + */ > + watchdog_set_timeout(wdd, wdd->timeout); > > - return err; > + return watchdog_register_device(wdd); > } > > static void f71808fg_pinconf(struct watchdog_data *wd) > @@ -812,8 +536,11 @@ static struct f71808e_variant __init *f71808e_find(int > sioaddr) > return variant; > } > > +static struct watchdog_data watchdog; > + > static int __init f71808e_init(void) > { > + struct watchdog_data *wd = &watchdog; > static const unsigned short addrs[] = { 0x2e, 0x4e }; > struct f71808e_variant *variant; > int i; > @@ -831,19 +558,15 @@ static int __init f71808e_init(void) > if (i == ARRAY_SIZE(addrs)) > return PTR_ERR(variant); > > - watchdog.variant = variant; > + wd->variant = variant; > + wd->sioaddr = addrs[i]; > > - return watchdog_init(addrs[i]); > + return watchdog_init(wd); > } > > static void __exit f71808e_exit(void) > { > - if (watchdog_is_running()) { > - pr_warn("Watchdog timer still running, stopping it\n"); > - watchdog_stop(); > - } > - misc_deregister(&watchdog_miscdev); > - unregister_reboot_notifier(&watchdog_notifier); > + watchdog_unregister_device(&watchdog.wdd); > } > > MODULE_DESCRIPTION("F71808E Watchdog Driver");
