v2.6.26-rc3 patches

drivers/watchdog/geodewdt.c new file +23 -0

+
+static void geodewdt_ping(void)
+{
+	/* Stop the counter */
+	geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+
+	/* Reset the counter */
+	geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+
+	/* Enable the counter */
+	geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, MFGPT_SETUP_CNTEN);
+}
+
+static void geodewdt_disable(void)
+{
+	geode_mfgpt_write(wdt_timer, MFGPT_REG_SETUP, 0);
+	geode_mfgpt_write(wdt_timer, MFGPT_REG_COUNTER, 0);
+}
+
+static int geodewdt_set_heartbeat(int val)
+{
+	if (val < 1 || val > GEODEWDT_MAX_SECONDS)
+		return -EINVAL;

There is no locking in geodewdt - so all those routines can end up being run together. Not sure it is a problem just noting it while reviewing.

Author: Jordan Crouse <[email protected]>
Date:   Mon Jan 21 10:07:00 2008 -0700

[WATCHDOG] Add a watchdog driver based on the CS5535/CS5536 MFGPT timers

Quoted 4 lines · Wim Van Sebroeck

Author: Jordan Crouse <[email protected]>
Date:   Mon Jan 21 10:07:00 2008 -0700

[WATCHDOG] Add a watchdog driver based on the CS5535/CS5536 MFGPT timers

-tip testing found the following build failure on latest -git:

drivers/watchdog/geodewdt.c: In function 'geodewdt_probe':
drivers/watchdog/geodewdt.c:225: error: too many arguments to function 'geode_mfgpt_alloc_timer' make[1]: *** [drivers/watchdog/geodewdt.o] Error 1
make: *** [drivers/watchdog/geodewdt.o] Error 2

with this config:

http://redhat.com/~mingo/misc/config-Fri_May_30_15_19_52_CEST_2008.bad

find the fix below.

Signed-off-by Ingo Molnar <[email protected]>

---
 drivers/watchdog/geodewdt.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: tip/drivers/watchdog/geodewdt.c
===================================================================

drivers/watchdog/geodewdt.c +1 -2

@@ -221,8 +221,7 @@ geodewdt_probe(struct platform_device *d
 {
 	int ret, timer;
 
-	timer = geode_mfgpt_alloc_timer(MFGPT_TIMER_ANY,
-					MFGPT_DOMAIN_WORKING, THIS_MODULE);
+	timer = geode_mfgpt_alloc_timer(MFGPT_TIMER_ANY, MFGPT_DOMAIN_WORKING);
 
 	if (timer == -1) {
 		printk(KERN_ERR "geodewdt:  No timers were available\n");

We had planned to use the 'owner' field for allowing re-allocation of MFGPTs; however, doing it by module owner name isn't flexible enough. So, drop this for now. If it turns out that we need timers in modules, we'll need to come up with a scheme that matches the write-once fields of the MFGPTx_SETUP register, and drops ponies from > the sky.

Quoted 13 lines · Ingo Molnar

Quoted 4 lines · Wim Van Sebroeck

Author: Jordan Crouse <[email protected]>
Date:   Mon Jan 21 10:07:00 2008 -0700

[WATCHDOG] Add a watchdog driver based on the CS5535/CS5536 MFGPT timers

-tip testing found the following build failure on latest -git:

drivers/watchdog/geodewdt.c: In function 'geodewdt_probe':
drivers/watchdog/geodewdt.c:225: error: too many arguments to function 'geode_mfgpt_alloc_timer' make[1]: *** [drivers/watchdog/geodewdt.o] Error 1
make: *** [drivers/watchdog/geodewdt.o] Error 2

Quoted 7 lines

with this config:

http://redhat.com/~mingo/misc/config-Fri_May_30_15_19_52_CEST_2008.bad

find the fix below.

Signed-off-by Ingo Molnar <[email protected]>

Acked-by Jordan Crouse <[email protected]>

Quoted 19 lines

---
 drivers/watchdog/geodewdt.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: tip/drivers/watchdog/geodewdt.c
===================================================================

drivers/watchdog/geodewdt.c +1 -2

@@ -221,8 +221,7 @@ geodewdt_probe(struct platform_device *d
 {
 	int ret, timer;
 
-	timer = geode_mfgpt_alloc_timer(MFGPT_TIMER_ANY,
-					MFGPT_DOMAIN_WORKING, THIS_MODULE);
+	timer = geode_mfgpt_alloc_timer(MFGPT_TIMER_ANY, MFGPT_DOMAIN_WORKING);
 
 	if (timer == -1) {
 		printk(KERN_ERR "geodewdt:  No timers were available\n");

I was trying to compile a kernel with this support, and noticed this bug, too. The interface was changed in commit fa28e067c3b8af96c79c060e163b1387c172ae75, with the message:

Quoted 2 lines

We had planned to use the 'owner' field for allowing re-allocation of MFGPTs; however, doing it by module owner name isn't flexible enough. >

So, drop this for now. If it turns out that we need timers in

Quoted 2 lines

modules, we'll need to come up with a scheme that matches the write-once fields of the MFGPTx_SETUP register, and drops ponies from >

the sky.

I may be reading this incorrectly, but it seems that to do this correctly, we would either need to drop module support for this driver or rework the MFGPT code. I'm not sure there's a kernel hook for dropping ponies from the sky quite yet.

Quoted 41 lines · Jordan Crouse

Quoted 15 lines · Ingo Molnar

Quoted 5 lines · Wim Van Sebroeck

Author: Jordan Crouse <[email protected]>
Date:   Mon Jan 21 10:07:00 2008 -0700

[WATCHDOG] Add a watchdog driver based on the CS5535/CS5536 MFGPT
timers

-tip testing found the following build failure on latest -git:

drivers/watchdog/geodewdt.c: In function 'geodewdt_probe':
drivers/watchdog/geodewdt.c:225: error: too many arguments to function
'geode_mfgpt_alloc_timer'
make[1]: *** [drivers/watchdog/geodewdt.o] Error 1
make: *** [drivers/watchdog/geodewdt.o] Error 2

Quoted 7 lines

with this config:

http://redhat.com/~mingo/misc/config-Fri_May_30_15_19_52_CEST_2008.bad

find the fix below.

Signed-off-by Ingo Molnar <[email protected]>

Acked-by Jordan Crouse <[email protected]>

Quoted 17 lines

---
 drivers/watchdog/geodewdt.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: tip/drivers/watchdog/geodewdt.c
===================================================================

drivers/watchdog/geodewdt.c +1 -1

@@ -221,8 +221,7 @@ geodewdt_probe(struct platform_device *d
 {
 	int ret, timer;
 -	timer = geode_mfgpt_alloc_timer(MFGPT_TIMER_ANY,
-					MFGPT_DOMAIN_WORKING, THIS_MODULE);
+	timer = geode_mfgpt_alloc_timer(MFGPT_TIMER_ANY, MFGPT_DOMAIN_WORKING);
  	if (timer == -1) {
 		printk(KERN_ERR "geodewdt:  No timers were available\n");
 

I think the hamster dropping code is queued for 2.6.27, so at least we're up to mammals.

The story here is that in an unfortunate instance of bad planning the MFGPT timers can only be configured once, so a module can't allocate a timer at init and release it when it is done. The original object of this code was to try give the timer back to a module if it happened to go away and come back, but that is clearly a more complex process then just simply storing the module name, and this code fell into bitrot.

So its not so much that we need to drop module support, rather it needs to be understood that if you remove and insert your module on a regular basis you will run out of timers, and deprive others of the timers too. I think that is a reasonable restriction to impose, given the limited usefulness of these timers for general purpose use.

Quoted 9 lines · Jordan Crouse

I think the hamster dropping code is queued for 2.6.27, so at least we're up to mammals.

The story here is that in an unfortunate instance of bad planning the MFGPT timers can only be configured once, so a module can't allocate a timer at init and release it when it is done. The original object of this code was to try give the timer back to a module if it happened to go away and come back, but that is clearly a more complex process then just simply storing the module name, and this code fell into bitrot.

It is a bit unfortunate that someone decided to design 'configure once' hardware. What were they thinking?

I run a watchdog using the mfgpt, and I simply tore out the code that prevents reuse of the timers, and I decided which timers I am going to use for which purpose and never reuse them for anything else, so the parts of the configuration in the hardware that is fixed isn't an issue then. I start the watchdog from grub, so I had to override the check when the kernel takes over the timer watchdog management after all.

Quoted 5 lines

So its not so much that we need to drop module support, rather it needs to be understood that if you remove and insert your module on a regular basis you will run out of timers, and deprive others of the timers too. I think that is a reasonable restriction to impose, given the limited usefulness of these timers for general purpose use.

They are useful timers, but yes perhaps not for general purpose. As a source of interrupts at certain intervals or as a watchdog they are not too bad.