Skip to contentSkip to conversation

v2.5.25-rc patches

WATCHDOG In review Wim Van Sebroeck 4 participants 5 messages Mar 10, 2008 – Apr 1, 2008

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

Hi Linus,

Please pull from 'master' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/wim/linux-2.6-watchdog.git
or if master.kernel.org hasn't synced up yet:
master.kernel.org:/pub/scm/linux/kernel/git/wim/linux-2.6-watchdog.git

This will update the following files:

 drivers/watchdog/cpu5wdt.c     |    4 
 drivers/watchdog/hpwdt.c       |  214 ++++-------------------------------------
 drivers/watchdog/it8712f_wdt.c |   78 ++++++++++++--
 drivers/watchdog/machzwd.c     |    2 
 drivers/watchdog/mtx-1_wdt.c   |    4 
 drivers/watchdog/pcwd_usb.c    |    4 
 drivers/watchdog/s3c2410_wdt.c |    8 -
 drivers/watchdog/shwdt.c       |    2 
 8 files changed, 100 insertions(+), 216 deletions(-)

with these Changes:

Author: Adrian Bunk <[email protected]>
Date:   Fri Feb 22 21:58:02 2008 +0200

[WATCHDOG] make watchdog/hpwdt.c:asminline_call() static

This patch makes the needlessly global asminline_call() static and removes the not required "asmlinkage".

Author: Florian Fainelli <[email protected]>
Date:   Mon Feb 25 13:39:57 2008 +0100

[WATCHDOG] Remove volatiles from watchdog device structures

Remove the volatile since those are useless in such a structure.

Author: Harvey Harrison <[email protected]>
Date:   Wed Mar 5 18:24:58 2008 -0800

[WATCHDOG] replace remaining __FUNCTION__ occurrences

__FUNCTION__ is gcc-specific, use __func__

Author: Roland Dreier <[email protected]>
Date:   Thu Feb 28 12:34:42 2008 -0800

[WATCHDOG] hpwdt: Use dmi_walk() instead of own copy

We can simplify the code by deleting all of the duplicated DMI table walking code and using the kernel's existing dmi_walk() interface to find the DMI entry the driver is looking for.

Author: Roland Dreier <[email protected]>
Date:   Thu Feb 28 09:48:10 2008 -0800

[WATCHDOG] Fix return value warning in hpwdt

The return value of smbios_scan_machine() is never used, and when it succeeds it doesn't return anything, so just make it void. This fixes:

drivers/watchdog/hpwdt.c: In function 'smbios_scan_machine':
drivers/watchdog/hpwdt.c:562: warning: control reaches end of non-void function

Author: Roland Dreier <[email protected]>
Date:   Thu Feb 28 09:38:44 2008 -0800

[WATCHDOG] Fix declaration of struct smbios_entry_point in hpwdt

On my HP DL380 G5 system running a 64-bit kernel, loading the hpwdt driver causes a crash because the driver attempts to ioremap an invalid physical address. This is because the driver has an incorrect definition of the SMBIOS table entry point structure: the table address is only a 32-bit quantity, and making it a u64 means that the high-order 32 bits end up containing garbage.

Correcting the structure definition fixes the driver so that it loads without any problems on my system.

Author: Andrew Paprocki <[email protected]>
Date:   Sun Feb 10 22:11:15 2008 -0500

[WATCHDOG] it8712f_wdt support for 16-bit timeout values, WDIOC_GETSTATUS

This patch adds support for 16-bit watchdog timeout values which are available in chip revisions >= 0x08. Values <= 65535 are seconds precision, otherwise minutes precision is used up to a maximum value of 3932100. Added implementation for WDIOC_GETSTATUS which checks the WDT status bit in the WDT control register.

The Changes can also be looked at on:

	http://www.kernel.org/git/?p=linux/kernel/git/wim/linux-2.6-watchdog.git;a=summary

For completeness, I added the overal diff below.

Greetings,
Wim.

================================================================================

drivers/watchdog/cpu5wdt.c +2 -2
index 5941ca6..df72f90 100644
@@ -59,9 +59,9 @@ static int ticks = 10000;
 
 static struct {
 	struct completion stop;
-	volatile int running;
+	int running;
 	struct timer_list timer;
-	volatile int queue;
+	int queue;
 	int default_ticks;
 	unsigned long inuse;
 } cpu5wdt_device;
drivers/watchdog/hpwdt.c +24 -190

260 lines not on this page · raw message

drivers/watchdog/it8712f_wdt.c +64 -14

143 lines not on this page · raw message

drivers/watchdog/machzwd.c +1 -1
index e6e07b4..6905135 100644
@@ -141,7 +141,7 @@ static unsigned long next_heartbeat = 0;
 #ifndef ZF_DEBUG
 #	define dprintk(format, args...)
 #else
-#	define dprintk(format, args...) printk(KERN_DEBUG PFX ":%s:%d: " format, __FUNCTION__, __LINE__ , ## args)
+#	define dprintk(format, args...) printk(KERN_DEBUG PFX ":%s:%d: " format, __func__, __LINE__ , ## args)
 #endif
 
 
drivers/watchdog/mtx-1_wdt.c +2 -2
index 789831b..10b89f2 100644
@@ -59,9 +59,9 @@ static int ticks = 100 * HZ;
 
 static struct {
 	struct completion stop;
-	volatile int running;
+	int running;
 	struct timer_list timer;
-	volatile int queue;
+	int queue;
 	int default_ticks;
 	unsigned long inuse;
 	unsigned gpio;
drivers/watchdog/pcwd_usb.c +2 -2
index 0f3fd6c..bf443d0 100644
@@ -179,11 +179,11 @@ static void usb_pcwd_intr_done(struct urb *urb)
 	case -ENOENT:
 	case -ESHUTDOWN:
 		/* this urb is terminated, clean up */
-		dbg("%s - urb shutting down with status: %d", __FUNCTION__, urb->status);
+		dbg("%s - urb shutting down with status: %d", __func__, urb->status);
 		return;
 	/* -EPIPE:  should clear the halt */
 	default:		/* error */
-		dbg("%s - nonzero urb status received: %d", __FUNCTION__, urb->status);
+		dbg("%s - nonzero urb status received: %d", __func__, urb->status);
 		goto resubmit;
 	}
 
drivers/watchdog/s3c2410_wdt.c +4 -4
index 5d1c15f..7645e88 100644
@@ -144,7 +144,7 @@ static int s3c2410wdt_start(void)
 	}
 
 	DBG("%s: wdt_count=0x%08x, wtcon=%08lx\n",
-	    __FUNCTION__, wdt_count, wtcon);
+	    __func__, wdt_count, wtcon);
 
 	writel(wdt_count, wdt_base + S3C2410_WTDAT);
 	writel(wdt_count, wdt_base + S3C2410_WTCNT);
@@ -167,7 +167,7 @@ static int s3c2410wdt_set_heartbeat(int timeout)
 	count = timeout * freq;
 
 	DBG("%s: count=%d, timeout=%d, freq=%d\n",
-	    __FUNCTION__, count, timeout, freq);
+	    __func__, count, timeout, freq);
 
 	/* if the count is bigger than the watchdog register,
 	   then work out what we need to do (and if) we can
@@ -189,7 +189,7 @@ static int s3c2410wdt_set_heartbeat(int timeout)
 	tmr_margin = timeout;
 
 	DBG("%s: timeout=%d, divisor=%d, count=%d (%08x)\n",
-	    __FUNCTION__, timeout, divisor, count, count/divisor);
+	    __func__, timeout, divisor, count, count/divisor);
 
 	count /= divisor;
 	wdt_count = count;
@@ -355,7 +355,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	int ret;
 	int size;
 
-	DBG("%s: probe=%p\n", __FUNCTION__, pdev);
+	DBG("%s: probe=%p\n", __func__, pdev);
 
 	dev = &pdev->dev;
 	wdt_dev = &pdev->dev;
drivers/watchdog/shwdt.c +1 -1
index 61dde86..1277f7e 100644
@@ -298,7 +298,7 @@ static int sh_wdt_mmap(struct file *file, struct vm_area_struct *vma)
 	if (io_remap_pfn_range(vma, vma->vm_start, addr >> PAGE_SHIFT,
 			       PAGE_SIZE, vma->vm_page_prot)) {
 		printk(KERN_ERR PFX "%s: io_remap_pfn_range failed\n",
-		       __FUNCTION__);
+		       __func__);
 		return -EAGAIN;
 	}
 
#1 lore raw

Hi,

WARNING

this patch can cause serious problems in the case, that someone use it with an it8712 rev. 8 and above.

Quoted 9 lines · Wim Van Sebroeck

Hi Linus,

Please pull from 'master' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/wim/linux-2.6-watchdog.git
or if master.kernel.org hasn't synced up yet:
master.kernel.org:/pub/scm/linux/kernel/git/wim/linux-2.6-watchdog.git

This will update the following files:
....

Your patch changes it8712f_wdt.c in function t8712f_wdt_update_margin():

drivers/watchdog/it8712f_wdt.c static void · line 175

Quoted 5 lines
drivers/watchdog/it8712f_wdt.c +4 -1
-	superio_outb((margin > 255) ? (margin / 60) : margin, WDT_TIMEOUT);
+	if (revision >= 0x08)
+		superio_outw(units, WDT_TIMEOUT);
+	else
+		superio_outb(units, WDT_TIMEOUT);

but here you can't use superio_outw, because the bytes are swapped --historical reason.

I suggest to substitute

	superio_outw(units, WDT_TIMEOUT);
with
	superio_outb(units >> 8, WDT_TIMEOUT + 1);
	superio_outb(units, WDT_TIMEOUT);

or to change the patch with (only the referenced position above)

+ if (revision >= 0x08)
+ superio_outb(units >> 8, WDT_TIMEOUT + 1);
+
+ superio_outb(units, WDT_TIMEOUT);

I'm sorry, that I resend this message.
Now I'm able to respond correctly to existing thread.

Regards,
Oliver

#2 lore raw

Hi Oliver,

Quoted 2 lines

this patch can cause serious problems in the case, that someone use it with an it8712 rev. 8 and above.

...

Quoted 11 lines

Your patch changes it8712f_wdt.c in function t8712f_wdt_update_margin():

drivers/watchdog/it8712f_wdt.c static void · line 175

Quoted 5 lines
drivers/watchdog/it8712f_wdt.c +4 -1
-	superio_outb((margin > 255) ? (margin / 60) : margin, WDT_TIMEOUT);
+	if (revision >= 0x08)
+		superio_outw(units, WDT_TIMEOUT);
+	else
+		superio_outb(units, WDT_TIMEOUT);

but here you can't use superio_outw, because the bytes are swapped --historical reason.

Quoted 6 lines

I suggest to substitute

	superio_outw(units, WDT_TIMEOUT);
with
	superio_outb(units >> 8, WDT_TIMEOUT + 1);
	superio_outb(units, WDT_TIMEOUT);

Can you keep "Andrew Paprocki" <[email protected]> in the loop? Can you also create a patch and test it together with Andrew? We need to make sure that we fix this before 2.6.25 is there.

Thanks,
Wim.

#3 lore raw

PATCH 1/1 v2.6.25-rc7: it8712f_wdt.c contains an error - was Re: [WATCHDOG] v2.5.25-rc patches

Hi,

i've mailed before to lkml and Andrew Paprocki (2008-03-12) directly. But the original patch wasn't changed. Here's a patch to correct the error.

Description of the error:
By introducing the support of 16 Bit timer values for later it8712 an error was made. The value is stored through superio_outw() which cause, that the upper Byte is in the Register called "Watch Dog Timer Time-Out Value (LSB) Register" and the lower in "Watch Dog Timer Time-Out Value (MSB) Register".

A common value is 60: stored this way the watchdog timer runs out in 4h 16mins. In case of a deadlocked server this is a long time to wait, before the service comes back!

Regards,
Oliver

Quoted 21 lines · Wim Van Sebroeck

Hi Oliver,

Quoted 1 line

this patch can cause serious problems in the case, that someone use it with an it8712 rev. 8 and above.

...

Quoted 10 lines

Your patch changes it8712f_wdt.c in function t8712f_wdt_update_margin():

Review

Quoted 5 lines
(quoted diff) +4 -1
-	superio_outb((margin > 255) ? (margin / 60) : margin, WDT_TIMEOUT);
+	if (revision >= 0x08)
+		superio_outw(units, WDT_TIMEOUT);
+	else
+		superio_outb(units, WDT_TIMEOUT);

but here you can't use superio_outw, because the bytes are swapped --historical reason.

Quoted 6 lines

I suggest to substitute

	superio_outw(units, WDT_TIMEOUT);
with
	superio_outb(units >> 8, WDT_TIMEOUT + 1);
	superio_outb(units, WDT_TIMEOUT);
Quoted 6 lines

Can you keep "Andrew Paprocki" <[email protected]> in the loop? Can you also create a patch and test it together with Andrew? We need to make sure that we fix this before 2.6.25 is there.

Thanks,
Wim

This patch corrects an error in the driver it8712f_wdt

===

drivers/watchdog/it8712f_wdt.c +2 -12
diff -urP linux-2.6.25-rc7/drivers/watchdog/it8712f_wdt.c linux-2.6.25-rc7-aaa/drivers/watchdog/it8712f_wdt.c
@@ -111,15 +111,6 @@
 	return val;
 }
 
-static void
-superio_outw(int val, int reg)
-{
-	outb(reg++, REG);
-	outb((val >> 8) & 0xff, VAL);
-	outb(reg, REG);
-	outb(val & 0xff, VAL);
-}
-
 static inline void
 superio_select(int ldn)
 {
@@ -170,9 +161,8 @@
 	superio_outb(config, WDT_CONFIG);
 
 	if (revision >= 0x08)
-		superio_outw(units, WDT_TIMEOUT);
-	else
-		superio_outb(units, WDT_TIMEOUT);
+		superio_outb(units >> 8, WDT_TIMEOUT + 1);
+	superio_outb(units, WDT_TIMEOUT);
 }
 
 static int
#4 lore raw

Re: [PATCH][WATCHDOG] v2.6.25-rc7: it8712f_wdt.c contains an error - was Re: [WATCHDOG] v2.5.25-rc patches

Oliver,

Sorry, the previous e-mail slipped by me. I'll try this out later and update.

Thanks,
-Andrew

Quoted 52 lines · Oliver Schuster

Hi,

i've mailed before to lkml and Andrew Paprocki (2008-03-12) directly. But the original patch wasn't changed. Here's a patch to correct the error.

Description of the error:
By introducing the support of 16 Bit timer values for later it8712 an error was made. The value is stored through superio_outw() which cause, that the upper Byte is in the Register called "Watch Dog Timer Time-Out Value (LSB) Register" and the lower in "Watch Dog Timer Time-Out Value (MSB) Register".

A common value is 60: stored this way the watchdog timer runs out in 4h 16mins. In case of a deadlocked server this is a long time to wait, before the service comes back!

Regards,
Oliver

Quoted 28 lines · Wim Van Sebroeck

Hi Oliver,

Quoted 2 lines

this patch can cause serious problems in the case, that someone use it with an it8712 rev. 8 and above.

...

Quoted 7 lines

Your patch changes it8712f_wdt.c in function t8712f_wdt_update_margin():

Review

Quoted 5 lines
(quoted diff) +4 -1
-   superio_outb((margin > 255) ? (margin / 60) : margin, WDT_TIMEOUT);
+   if (revision >= 0x08)
+           superio_outw(units, WDT_TIMEOUT);
+   else
+           superio_outb(units, WDT_TIMEOUT);
Quoted 9 lines

but here you can't use superio_outw, because the bytes are swapped --historical reason.

I suggest to substitute

     superio_outw(units, WDT_TIMEOUT);
with
     superio_outb(units >> 8, WDT_TIMEOUT + 1);
     superio_outb(units, WDT_TIMEOUT);

Can you keep "Andrew Paprocki" <[email protected]> in the loop? Can you also create a patch and test it together with Andrew? We need to make sure that we fix this before 2.6.25 is there.

Thanks,
Wim