From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from relay.parallels.com ([195.214.232.42]:51100 "EHLO relay.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752565Ab2FSNLi convert rfc822-to-8bit (ORCPT ); Tue, 19 Jun 2012 09:11:38 -0400 Message-ID: <4FE07A85.9020901@parallels.com> Date: Tue, 19 Jun 2012 17:11:33 +0400 From: Tony Zelenoff MIME-Version: 1.0 To: Hans de Goede CC: "linux-watchdog@vger.kernel.org" , "wim@iguana.be" Subject: Re: [PATCH] iTCO_wdt: move watchdog to proper kernel interface References: <1339150154-25247-1-git-send-email-antonz@parallels.com> <4FE05279.7080301@parallels.com> <4FE0733B.6020602@redhat.com> In-Reply-To: <4FE0733B.6020602@redhat.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Sender: linux-watchdog-owner@vger.kernel.org List-Id: linux-watchdog@vger.kernel.org Content-Transfer-Encoding: quoted-printable 19/06/12 4:40 PM, Hans de Goede =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > Hi, > > On 06/19/2012 12:20 PM, Tony Zelenoff wrote: >> Ping. >> >> Is this patch wrong? Or just missed? > > Definitely not wrong (on the conceptual level I've not actually > checked the code). We do want the iTCO_wdt driver to be converted > to the (new) watchdog device framework. > > I think you just need to give Wim some more time to take a good > look at this. Wim does the watchdog subsystem maintainer ship in > his spare time, so sometimes you need to be a bit patient :) :) Of course i be patient. Just want to be sure, that patch is not missed.=20 Thanks for your reply. > Regards, > > Hans > > > >> >> 8/06/12 2:09 PM, Anton Zelenov =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >>> Remove obsoleted fs interface and move to proper usage of >>> watchdog_device interface. >>> >>> Signed-off-by: Tony Zelenoff >>> --- >>> drivers/watchdog/iTCO_wdt.c | 246 ++++++++++++++-----------------= ------------ >>> 1 files changed, 77 insertions(+), 169 deletions(-) >>> >>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.= c >>> index bc47e90..14e48d9 100644 >>> --- a/drivers/watchdog/iTCO_wdt.c >>> +++ b/drivers/watchdog/iTCO_wdt.c >>> @@ -87,9 +87,11 @@ >>> #define TCO2_CNT (TCOBASE + 0x0a) /* TCO2 Control Register *= / >>> #define TCOv2_TMR (TCOBASE + 0x12) /* TCOv2 Timer Initial Value= */ >>> >>> +/* Maximum allowed timeouts for iTCO */ >>> +#define iTCO_V1_MAX_TIMEOUT 0x03f >>> +#define iTCO_V2_MAX_TIMEOUT 0x3ff >>> + >>> /* internal variables */ >>> -static unsigned long is_active; >>> -static char expect_release; >>> static struct { /* this is private data for the iTCO_wdt de= vice */ >>> /* TCO version/generation */ >>> unsigned int iTCO_version; >>> @@ -107,9 +109,9 @@ static struct { /* this is private data fo= r the iTCO_wdt device */ >>> >>> /* module parameters */ >>> #define WATCHDOG_HEARTBEAT 30 /* 30 sec default heartbeat */ >>> -static int heartbeat =3D WATCHDOG_HEARTBEAT; /* in seconds */ >>> -module_param(heartbeat, int, 0); >>> -MODULE_PARM_DESC(heartbeat, "Watchdog timeout in seconds. " >>> +static unsigned int heartbeat_param =3D WATCHDOG_HEARTBEAT; /* in s= econds */ >>> +module_param(heartbeat_param, uint, 0); >>> +MODULE_PARM_DESC(heartbeat_param, "Watchdog timeout in seconds. " >>> "5..76 (TCO v1) or 3..614 (TCO v2), default=3D" >>> __MODULE_STRING(WATCHDOG_HEARTBEAT) ")"); >>> >>> @@ -178,13 +180,13 @@ static int iTCO_wdt_unset_NO_REBOOT_bit(void) >>> return ret; /* returns: 0 =3D OK, -EIO =3D Error */ >>> } >>> >>> -static int iTCO_wdt_start(void) >>> +static int iTCO_wdt_start(struct watchdog_device *w) >>> { >>> unsigned int val; >>> >>> spin_lock(&iTCO_wdt_private.io_lock); >>> >>> - iTCO_vendor_pre_start(iTCO_wdt_private.smi_res, heartbeat); >>> + iTCO_vendor_pre_start(iTCO_wdt_private.smi_res, w->timeout); >>> >>> /* disable chipset's NO_REBOOT bit */ >>> if (iTCO_wdt_unset_NO_REBOOT_bit()) { >>> @@ -212,7 +214,7 @@ static int iTCO_wdt_start(void) >>> return 0; >>> } >>> >>> -static int iTCO_wdt_stop(void) >>> +static int iTCO_wdt_stop(struct watchdog_device *w) >>> { >>> unsigned int val; >>> >>> @@ -236,11 +238,11 @@ static int iTCO_wdt_stop(void) >>> return 0; >>> } >>> >>> -static int iTCO_wdt_keepalive(void) >>> +static int iTCO_wdt_ping(struct watchdog_device *w) >>> { >>> spin_lock(&iTCO_wdt_private.io_lock); >>> >>> - iTCO_vendor_pre_keepalive(iTCO_wdt_private.smi_res, heartbeat); >>> + iTCO_vendor_pre_keepalive(iTCO_wdt_private.smi_res, w->timeout); >>> >>> /* Reload the timer by writing to the TCO Timer Counter regist= er */ >>> if (iTCO_wdt_private.iTCO_version =3D=3D 2) >>> @@ -257,7 +259,24 @@ static int iTCO_wdt_keepalive(void) >>> return 0; >>> } >>> >>> -static int iTCO_wdt_set_heartbeat(int t) >>> +static inline int iTCO_wdt_timeout_valid(unsigned int t) >>> +{ >>> + unsigned int timeout =3D 0; >>> + >>> + if (iTCO_wdt_private.iTCO_version =3D=3D 2) >>> + timeout =3D iTCO_V2_MAX_TIMEOUT; >>> + >>> + if (iTCO_wdt_private.iTCO_version =3D=3D 1) >>> + timeout =3D iTCO_V1_MAX_TIMEOUT; >>> + >>> + if (t > timeout) >>> + return 0; >>> + >>> + return 1; >>> +} >>> + >>> +static int iTCO_wdt_set_timeout(struct watchdog_device *w, >>> + unsigned int t) >>> { >>> unsigned int val16; >>> unsigned char val8; >>> @@ -273,8 +292,8 @@ static int iTCO_wdt_set_heartbeat(int t) >>> /* "Values of 0h-3h are ignored and should not be attempted" *= / >>> if (tmrval < 0x04) >>> return -EINVAL; >>> - if (((iTCO_wdt_private.iTCO_version =3D=3D 2) && (tmrval > 0x3ff= )) || >>> - ((iTCO_wdt_private.iTCO_version =3D=3D 1) && (tmrval > 0x03f= ))) >>> + >>> + if (!iTCO_wdt_timeout_valid(t)) >>> return -EINVAL; >>> >>> iTCO_vendor_pre_set_heartbeat(tmrval); >>> @@ -304,11 +323,11 @@ static int iTCO_wdt_set_heartbeat(int t) >>> return -EINVAL; >>> } >>> >>> - heartbeat =3D t; >>> + w->timeout =3D t; >>> return 0; >>> } >>> >>> -static int iTCO_wdt_get_timeleft(int *time_left) >>> +static unsigned int iTCO_wdt_get_timeleft(struct watchdog_device *w) >>> { >>> unsigned int val16; >>> unsigned char val8; >>> @@ -320,8 +339,10 @@ static int iTCO_wdt_get_timeleft(int *time_left) >>> val16 &=3D 0x3ff; >>> spin_unlock(&iTCO_wdt_private.io_lock); >>> >>> - *time_left =3D (val16 * 6) / 10; >>> - } else if (iTCO_wdt_private.iTCO_version =3D=3D 1) { >>> + return (val16 * 6) / 10; >>> + } >>> + >>> + if (iTCO_wdt_private.iTCO_version =3D=3D 1) { >>> spin_lock(&iTCO_wdt_private.io_lock); >>> val8 =3D inb(TCO_RLD); >>> val8 &=3D 0x3f; >>> @@ -329,156 +350,39 @@ static int iTCO_wdt_get_timeleft(int *time_lef= t) >>> val8 +=3D (inb(TCOv1_TMR) & 0x3f); >>> spin_unlock(&iTCO_wdt_private.io_lock); >>> >>> - *time_left =3D (val8 * 6) / 10; >>> - } else >>> - return -EINVAL; >>> - return 0; >>> -} >>> - >>> -/* >>> - * /dev/watchdog handling >>> - */ >>> - >>> -static int iTCO_wdt_open(struct inode *inode, struct file *file) >>> -{ >>> - /* /dev/watchdog can only be opened once */ >>> - if (test_and_set_bit(0, &is_active)) >>> - return -EBUSY; >>> - >>> - /* >>> - * Reload and activate timer >>> - */ >>> - iTCO_wdt_start(); >>> - return nonseekable_open(inode, file); >>> -} >>> - >>> -static int iTCO_wdt_release(struct inode *inode, struct file *file) >>> -{ >>> - /* >>> - * Shut off the timer. >>> - */ >>> - if (expect_release =3D=3D 42) { >>> - iTCO_wdt_stop(); >>> - } else { >>> - pr_crit("Unexpected close, not stopping watchdog!\n"); >>> - iTCO_wdt_keepalive(); >>> - } >>> - clear_bit(0, &is_active); >>> - expect_release =3D 0; >>> - return 0; >>> -} >>> - >>> -static ssize_t iTCO_wdt_write(struct file *file, const char __user *= data, >>> - size_t len, loff_t *ppos) >>> -{ >>> - /* See if we got the magic character 'V' and reload the timer */ >>> - if (len) { >>> - if (!nowayout) { >>> - size_t i; >>> - >>> - /* note: just in case someone wrote the magic >>> - character five months ago... */ >>> - expect_release =3D 0; >>> - >>> - /* scan to see whether or not we got the >>> - magic character */ >>> - for (i =3D 0; i !=3D len; i++) { >>> - char c; >>> - if (get_user(c, data + i)) >>> - return -EFAULT; >>> - if (c =3D=3D 'V') >>> - expect_release =3D 42; >>> - } >>> - } >>> - >>> - /* someone wrote to us, we should reload the timer */ >>> - iTCO_wdt_keepalive(); >>> - } >>> - return len; >>> -} >>> - >>> -static long iTCO_wdt_ioctl(struct file *file, unsigned int cmd, >>> - unsigned long arg) >>> -{ >>> - int new_options, retval =3D -EINVAL; >>> - int new_heartbeat; >>> - void __user *argp =3D (void __user *)arg; >>> - int __user *p =3D argp; >>> - static const struct watchdog_info ident =3D { >>> - .options =3D WDIOF_SETTIMEOUT | >>> - WDIOF_KEEPALIVEPING | >>> - WDIOF_MAGICCLOSE, >>> - .firmware_version =3D 0, >>> - .identity =3D DRV_NAME, >>> - }; >>> - >>> - 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_SETOPTIONS: >>> - { >>> - if (get_user(new_options, p)) >>> - return -EFAULT; >>> - >>> - if (new_options & WDIOS_DISABLECARD) { >>> - iTCO_wdt_stop(); >>> - retval =3D 0; >>> - } >>> - if (new_options & WDIOS_ENABLECARD) { >>> - iTCO_wdt_keepalive(); >>> - iTCO_wdt_start(); >>> - retval =3D 0; >>> - } >>> - return retval; >>> + return (val8 * 6) / 10; >>> } >>> - case WDIOC_KEEPALIVE: >>> - iTCO_wdt_keepalive(); >>> - return 0; >>> >>> - case WDIOC_SETTIMEOUT: >>> - { >>> - if (get_user(new_heartbeat, p)) >>> - return -EFAULT; >>> - if (iTCO_wdt_set_heartbeat(new_heartbeat)) >>> - return -EINVAL; >>> - iTCO_wdt_keepalive(); >>> - /* Fall */ >>> - } >>> - case WDIOC_GETTIMEOUT: >>> - return put_user(heartbeat, p); >>> - case WDIOC_GETTIMELEFT: >>> - { >>> - int time_left; >>> - if (iTCO_wdt_get_timeleft(&time_left)) >>> - return -EINVAL; >>> - return put_user(time_left, p); >>> - } >>> - default: >>> - return -ENOTTY; >>> - } >>> + return -EINVAL; >>> } >>> >>> /* >>> * Kernel Interfaces >>> */ >>> >>> -static const struct file_operations iTCO_wdt_fops =3D { >>> - .owner =3D THIS_MODULE, >>> - .llseek =3D no_llseek, >>> - .write =3D iTCO_wdt_write, >>> - .unlocked_ioctl =3D iTCO_wdt_ioctl, >>> - .open =3D iTCO_wdt_open, >>> - .release =3D iTCO_wdt_release, >>> +static const struct watchdog_info iTCO_wdt_info =3D { >>> + .options =3D WDIOF_SETTIMEOUT | >>> + WDIOF_KEEPALIVEPING | >>> + WDIOF_MAGICCLOSE, >>> + .firmware_version =3D 0, >>> + .identity =3D DRV_NAME, >>> +}; >>> + >>> +static struct watchdog_ops iTCO_wdt_ops =3D { >>> + .owner =3D THIS_MODULE, >>> + .start =3D iTCO_wdt_start, >>> + .stop =3D iTCO_wdt_stop, >>> + .ping =3D iTCO_wdt_ping, >>> + .set_timeout =3D iTCO_wdt_set_timeout, >>> + .get_timeleft =3D iTCO_wdt_get_timeleft, >>> }; >>> >>> -static struct miscdevice iTCO_wdt_miscdev =3D { >>> - .minor =3D WATCHDOG_MINOR, >>> - .name =3D "watchdog", >>> - .fops =3D &iTCO_wdt_fops, >>> +static struct watchdog_device iTCO_wdt_dev =3D { >>> + .info =3D &iTCO_wdt_info, >>> + .ops =3D &iTCO_wdt_ops, >>> + .min_timeout =3D 1, >>> + /* It will be set up at _probe */ >>> + .max_timeout =3D 0xFFFF >>> }; >>> >>> /* >>> @@ -489,10 +393,10 @@ static void __devexit iTCO_wdt_cleanup(void) >>> { >>> /* Stop the timer before we leave */ >>> if (!nowayout) >>> - iTCO_wdt_stop(); >>> + iTCO_wdt_stop(&iTCO_wdt_dev); >>> >>> /* Deregister */ >>> - misc_deregister(&iTCO_wdt_miscdev); >>> + watchdog_unregister_device(&iTCO_wdt_dev); >>> >>> /* release resources */ >>> release_region(iTCO_wdt_private.tco_res->start, >>> @@ -559,7 +463,10 @@ static int __devinit iTCO_wdt_probe(struct platf= orm_device *dev) >>> ret =3D -EIO; >>> goto unreg_gcs; >>> } >>> - } >>> + >>> + iTCO_wdt_dev.max_timeout =3D iTCO_V2_MAX_TIMEOUT; >>> + } else >>> + iTCO_wdt_dev.max_timeout =3D iTCO_V1_MAX_TIMEOUT; >>> >>> /* Check chipset's NO_REBOOT bit */ >>> if (iTCO_wdt_unset_NO_REBOOT_bit() && iTCO_vendor_check_norebo= ot_on()) { >>> @@ -606,24 +513,25 @@ static int __devinit iTCO_wdt_probe(struct plat= form_device *dev) >>> outw(0x0004, TCO2_STS); /* Clear BOOT_STS bit */ >>> >>> /* Make sure the watchdog is not running */ >>> - iTCO_wdt_stop(); >>> + iTCO_wdt_stop(&iTCO_wdt_dev); >>> >>> /* Check that the heartbeat value is within it's range; >>> if not reset to the default */ >>> - if (iTCO_wdt_set_heartbeat(heartbeat)) { >>> - iTCO_wdt_set_heartbeat(WATCHDOG_HEARTBEAT); >>> - pr_info("timeout value out of range, using %d\n", heartbeat)= ; >>> + if (iTCO_wdt_set_timeout(&iTCO_wdt_dev, heartbeat_param)) { >>> + iTCO_wdt_set_timeout(&iTCO_wdt_dev, WATCHDOG_HEARTBEAT); >>> + pr_info("timeout value out of range, using %d\n", heartbeat_= param); >>> } >>> >>> - ret =3D misc_register(&iTCO_wdt_miscdev); >>> + watchdog_set_nowayout(&iTCO_wdt_dev, nowayout); >>> + >>> + ret =3D watchdog_register_device(&iTCO_wdt_dev); >>> if (ret !=3D 0) { >>> - pr_err("cannot register miscdev on minor=3D%d (err=3D%d)\n", >>> - WATCHDOG_MINOR, ret); >>> + pr_err("cannot register watchdog device (err=3D%d)\n", ret); >>> goto unreg_tco; >>> } >>> >>> pr_info("initialized. heartbeat=3D%d sec (nowayout=3D%d)\n", >>> - heartbeat, nowayout); >>> + heartbeat_param, nowayout); >>> >>> return 0; >>> >>> @@ -659,7 +567,7 @@ static int __devexit iTCO_wdt_remove(struct platf= orm_device *dev) >>> >>> static void iTCO_wdt_shutdown(struct platform_device *dev) >>> { >>> - iTCO_wdt_stop(); >>> + iTCO_wdt_stop(&iTCO_wdt_dev); >>> } >>> >>> static struct platform_driver iTCO_wdt_driver =3D { >>> >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-watchd= og" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog"= in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html