[parisc-linux] J5000 LED LCD patch (take 2)

[parisc-linux] J5000 LED LCD patch (take 2)

[David Pye]

[-- Attachment #1.1.1: Type: text/plain, Size: 1058 bytes --]

Hi,

Attached is the latest iteration of my patch, with the feedback from Grant 
Grundler incorporated (hopefully!)

The other changes are:

Adjust the scheduling so the work queue item runs only every 5 jiffies, 
instead of each one (as per the original tasklet). This means we can do a 
couple more calculations, and still hopefully be ahead in the efficiency 
stakes.

Set and enforce a minimum on-time for LEDs - this prevents the LCD ones 
'ghosting' under rapid LAN/IO on/off. This also applies to LED systems too. 
The minimum on-time is currently 100mS - feedback welcome as to whether this 
should be made LCD-only, or adjusted up/downwards. (This is necessary because 
once the min_cmd_delay has been taken into effect, rapidly fluttering LAN/IO 
lcd symbols cause the heartbeat to stutter slightly).

On my J5000, this gives a perfectly uniform heartbeat regardless of lan/io 
activity, and also prevents the lan/io symbols ghosting during continuous 
traffic.

As before, comments/suggestions welcome!

Cheers,

David

[-- Attachment #1.1.2: led_lcd_patch --]
[-- Type: text/x-diff, Size: 12928 bytes --]

diff -urpN linux-2.6.12-rc1-pa2/arch/parisc/kernel/time.c worktree/arch/parisc/kernel/time.c
--- linux-2.6.12-rc1-pa2/arch/parisc/kernel/time.c	2004-10-21 19:58:24.000000000 +0100
+++ worktree/arch/parisc/kernel/time.c	2005-03-24 01:04:32.000000000 +0000
@@ -89,14 +89,6 @@ irqreturn_t timer_interrupt(int irq, voi
 		}
 	}
     
-#ifdef CONFIG_CHASSIS_LCD_LED
-	/* Only schedule the led tasklet on cpu 0, and only if it
-	 * is enabled.
-	 */
-	if (cpu == 0 && !atomic_read(&led_tasklet.count))
-		tasklet_schedule(&led_tasklet);
-#endif
-
 	/* check soft power switch status */
 	if (cpu == 0 && !atomic_read(&power_tasklet.count))
 		tasklet_schedule(&power_tasklet);
diff -urpN linux-2.6.12-rc1-pa2/drivers/parisc/led.c worktree/drivers/parisc/led.c
--- linux-2.6.12-rc1-pa2/drivers/parisc/led.c	2005-03-18 13:17:10.000000000 +0000
+++ worktree/drivers/parisc/led.c	2005-03-24 01:07:08.000000000 +0000
@@ -18,6 +18,9 @@
  * Changes:
  *      - Audit copy_from_user in led_proc_write.
  *                                Daniele Bellucci <bellucda@tiscali.it>
+ *	- Switch from using a tasklet to a work queue, so the led_LCD_driver
+ *	  	can sleep.
+ *	  			  David Pye <dmp@davidmpye.dyndns.org>
  */
 
 #include <linux/config.h>
@@ -37,6 +40,7 @@
 #include <linux/proc_fs.h>
 #include <linux/ctype.h>
 #include <linux/blkdev.h>
+#include <linux/workqueue.h>
 #include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/hardware.h>
@@ -46,9 +50,9 @@
 #include <asm/uaccess.h>
 
 /* The control of the LEDs and LCDs on PARISC-machines have to be done 
-   completely in software. The necessary calculations are done in a tasklet
-   which is scheduled at every timer interrupt and since the calculations 
-   may consume relatively much CPU-time some of the calculations can be 
+   completely in software. The necessary calculations are done in a work queue
+   task which is scheduled regularly, and since the calculations may consume a 
+   relatively large amount of CPU time, some of the calculations can be 
    turned off with the following variables (controlled via procfs) */
 
 static int led_type = -1;
@@ -58,13 +62,16 @@ static int led_lanrxtx = 1;
 static char lcd_text[32];
 static char lcd_text_default[32];
 
+static struct workqueue_struct *led_wq;
+static void led_work_func(void *);
+static DECLARE_WORK(led_task, led_work_func, NULL);
+
 #if 0
 #define DPRINTK(x)	printk x
 #else
 #define DPRINTK(x)
 #endif
 
-
 struct lcd_block {
 	unsigned char command;	/* stores the command byte      */
 	unsigned char on;	/* value for turning LED on     */
@@ -115,12 +122,24 @@ lcd_info __attribute__((aligned(8))) =
 #define LCD_DATA_REG	lcd_info.lcd_data_reg_addr	 
 #define LED_DATA_REG	lcd_info.lcd_cmd_reg_addr	/* LASI & ASP only */
 
+#define LED_HASLCD 1
+#define LED_NOLCD  0
+
+/* The workqueue must be created at init-time */
+static int start_task(void) 
+{	
+	/* Create the work queue and queue the LED task */
+	led_wq = create_singlethread_workqueue("led_wq");	
+	queue_work(led_wq, &led_task);
+
+	return 0;
+}
+
+device_initcall(start_task);
 
 /* ptr to LCD/LED-specific function */
 static void (*led_func_ptr) (unsigned char);
 
-#define LED_HASLCD 1
-#define LED_NOLCD  0
 #ifdef CONFIG_PROC_FS
 static int led_proc_read(char *page, char **start, off_t off, int count, 
 	int *eof, void *data)
@@ -285,53 +304,41 @@ static void led_LASI_driver(unsigned cha
 /*
    ** 
    ** led_LCD_driver()
-   ** 
-   ** The logic of the LCD driver is, that we write at every scheduled call
-   ** only to one of LCD_CMD_REG _or_ LCD_DATA_REG - registers.
-   ** That way we don't need to let this tasklet busywait for min_cmd_delay
-   ** milliseconds.
-   **
-   ** TODO: check the value of "min_cmd_delay" against the value of HZ.
    **   
  */
 static void led_LCD_driver(unsigned char leds)
 {
-	static int last_index;	/* 0:heartbeat, 1:disk, 2:lan_in, 3:lan_out */
-	static int last_was_cmd;/* 0: CMD was written last, 1: DATA was last */
-	struct lcd_block *block_ptr;
-	int value;
-
-	switch (last_index) {
-	    case 0:	block_ptr = &lcd_info.heartbeat;
-			value = leds & LED_HEARTBEAT;
-			break;
-	    case 1:	block_ptr = &lcd_info.disk_io;
-			value = leds & LED_DISK_IO;
-			break;					
-	    case 2:	block_ptr = &lcd_info.lan_rcv;
-			value = leds & LED_LAN_RCV;
-			break;					
-	    case 3:	block_ptr = &lcd_info.lan_tx;
-			value = leds & LED_LAN_TX;
-			break;
-	    default:	/* should never happen: */
-			return;
-	}
+	static int lastleds, i;
+	static unsigned char mask[4] = { LED_HEARTBEAT, LED_DISK_IO,
+		LED_LAN_RCV, LED_LAN_TX };
+	
+	static struct lcd_block * blockp[4] = {
+		&lcd_info.heartbeat,
+		&lcd_info.disk_io,
+		&lcd_info.lan_rcv,
+		&lcd_info.lan_tx
+	};
 
-	if (last_was_cmd) {
-	    /* write the value to the LCD data port */
-    	    gsc_writeb( value ? block_ptr->on : block_ptr->off, LCD_DATA_REG );
-	} else {
-	    /* write the command-byte to the LCD command register */
-    	    gsc_writeb( block_ptr->command, LCD_CMD_REG );
-	}    
-	
-	/* now update the vars for the next interrupt iteration */ 
-	if (++last_was_cmd == 2) { /* switch between cmd & data */
-	    last_was_cmd = 0;
-	    if (++last_index == 4) 
-		last_index = 0;	 /* switch back to heartbeat index */
+	/* Convert min_cmd_delay (uS) to jiffies */
+	unsigned long wait_jiffies = 1 + (lcd_info.min_cmd_delay * HZ) / 1000000;
+
+	for (i=0; i<4; ++i) 
+	{
+		if ((leds & mask[i]) != (lastleds & mask[i])) 
+		{
+			gsc_writeb( blockp[i]->command, LCD_CMD_REG );
+			
+			set_current_state(TASK_UNINTERRUPTIBLE);		
+			schedule_timeout(wait_jiffies);	
+
+			gsc_writeb( leds & mask[i] ? blockp[i]->on : 
+					blockp[i]->off, LCD_DATA_REG );
+			
+			set_current_state(TASK_UNINTERRUPTIBLE);		
+			schedule_timeout(wait_jiffies);	
+		}
 	}
+	lastleds = leds;
 }
 
 
@@ -355,7 +362,7 @@ static __inline__ int led_get_net_activi
 
 	rx_total = tx_total = 0;
 	
-	/* we are running as tasklet, so locking dev_base 
+	/* we are running as a workqueue task, so locking dev_base 
 	 * for reading should be OK */
 	read_lock(&dev_base_lock);
 	for (dev = dev_base; dev; dev = dev->next) {
@@ -417,10 +424,9 @@ static __inline__ int led_get_diskio_act
 
 
 /*
-   ** led_tasklet_func()
+   ** led_work_func()
    ** 
-   ** is scheduled at every timer interrupt from time.c and
-   ** updates the chassis LCD/LED 
+   ** is scheduled every 5 jiffies and updates the chassis LCD/LED 
 
     TODO:
     - display load average (older machines like 715/64 have 4 "free" LED's for that)
@@ -431,66 +437,102 @@ static __inline__ int led_get_diskio_act
 #define HEARTBEAT_2ND_RANGE_START (HZ*22/100)
 #define HEARTBEAT_2ND_RANGE_END   (HEARTBEAT_2ND_RANGE_START + HEARTBEAT_LEN)
 
-#define NORMALIZED_COUNT(count) (count/(HZ/100))
+#define LED_MINIMUM_ON_TIME (HZ*10/100)
 
-static void led_tasklet_func(unsigned long unused)
+static void led_work_func (void *unused)
 {
 	static unsigned char lastleds;
-	unsigned char currentleds; /* stores current value of the LEDs */
-	static unsigned long count; /* static incremented value, not wrapped */
 	static unsigned long count_HZ; /* counter in range 0..HZ */
 
+	static unsigned long diskio_on_jiffies;
+	static unsigned long lantx_on_jiffies;
+	static unsigned long lanrcv_on_jiffies;
+
+	unsigned char currentleds = 0; /* stores current value of the LEDs */
+	
 	/* exit if not initialized */
 	if (!led_func_ptr)
 	    return;
 
-	/* increment the local counters */
-	++count;
-	if (++count_HZ == HZ)
+	/* increment the local counter */
+	count_HZ += 5;
+	if (count_HZ == HZ)
 	    count_HZ = 0;
 
-	currentleds = lastleds;
-
 	if (led_heartbeat)
 	{
 		/* flash heartbeat-LED like a real heart (2 x short then a long delay) */
 		if (count_HZ<HEARTBEAT_LEN || 
 		    (count_HZ>=HEARTBEAT_2ND_RANGE_START && count_HZ<HEARTBEAT_2ND_RANGE_END)) 
 		    currentleds |= LED_HEARTBEAT;
-		else
-		    currentleds &= ~LED_HEARTBEAT;
 	}
 
-	/* look for network activity and flash LEDs respectively */
-	if (led_lanrxtx && ((NORMALIZED_COUNT(count)+(8/2)) & 7) == 0)
+	/* If the leds are on, but should be turned off, make sure they have been on for
+	 * the minimum time, to prevent 'flutter' 
+	 */
+
+	/* look for network activity and flash LEDs accordingly */
+	if (led_lanrxtx)
 	{
-		currentleds &= ~(LED_LAN_RCV | LED_LAN_TX);
-		currentleds |= led_get_net_activity();
+		unsigned char new_lan_leds = led_get_net_activity();
+	
+		/* TX LED */
+		if (new_lan_leds & LED_LAN_TX) 
+		{
+			currentleds |= LED_LAN_TX;
+			lantx_on_jiffies = jiffies;
+		}
+		else if (lastleds & LED_LAN_TX && time_before(jiffies, lantx_on_jiffies + LED_MINIMUM_ON_TIME))
+		{
+			currentleds |= LED_LAN_TX;
+		}
+		
+		/* RCV LED */
+
+		if (new_lan_leds & LED_LAN_RCV) 
+		{
+			currentleds |= LED_LAN_RCV;
+			lanrcv_on_jiffies = jiffies;
+		}
+		else if (lastleds & LED_LAN_RCV && time_before(jiffies, lanrcv_on_jiffies + LED_MINIMUM_ON_TIME)) 
+		{
+			currentleds |= LED_LAN_RCV;
+		}
 	}
 
-	/* avoid to calculate diskio-stats at same irq  as netio-stats */
-	if (led_diskio && (NORMALIZED_COUNT(count) & 7) == 0)
+	/* Calculate disk IO */
+	if (led_diskio)
 	{
-		currentleds &= ~LED_DISK_IO;
-		currentleds |= led_get_diskio_activity();
+		unsigned char new_io_led = led_get_diskio_activity();
+		
+		if (new_io_led & LED_DISK_IO) 
+		{
+			currentleds |= LED_DISK_IO;
+			diskio_on_jiffies = jiffies;
+		}
+		else if (lastleds & LED_DISK_IO && time_before(jiffies, diskio_on_jiffies + LED_MINIMUM_ON_TIME)) 
+		{
+			currentleds |= LED_DISK_IO;
+		}
 	}
 
 	/* blink all LEDs twice a second if we got an Oops (HPMC) */
-	if (oops_in_progress) {
+	if (oops_in_progress) 
+	{
 		currentleds = (count_HZ<=(HZ/2)) ? 0 : 0xff;
 	}
 	
 	/* update the LCD/LEDs */
-	if (currentleds != lastleds) {
-	    led_func_ptr(currentleds);
-	    lastleds = currentleds;
+	if (currentleds != lastleds)
+	{
+		led_func_ptr(currentleds);
+		lastleds = currentleds;
 	}
+	
+	/* Requeue ourself to run in 5 jiffies */	
+	queue_delayed_work(led_wq, &led_task, 5);
 }
 
-/* main led tasklet struct (scheduled from time.c) */
-DECLARE_TASKLET_DISABLED(led_tasklet, led_tasklet_func, 0);
-
-
 /*
    ** led_halt()
    ** 
@@ -519,9 +561,13 @@ static int led_halt(struct notifier_bloc
 	default:		return NOTIFY_DONE;
 	}
 	
-	/* completely stop the LED/LCD tasklet */
-	tasklet_disable(&led_tasklet);
-
+	/* Cancel the work item and delete the queue */
+	if (led_wq) {
+		if (cancel_delayed_work(&led_task) == 0) 
+			flush_workqueue(led_wq);
+		destroy_workqueue(led_wq);
+	}
+ 
 	if (lcd_info.model == DISPLAY_MODEL_LCD)
 		lcd_print(txt);
 	else
@@ -586,9 +632,6 @@ int __init register_led_driver(int model
 	initialized++;
 	register_reboot_notifier(&led_notifier);
 
-	/* start the led tasklet for the first time */
-	tasklet_enable(&led_tasklet);
-	
 	return 0;
 }
 
@@ -623,7 +666,7 @@ void __init register_led_regions(void)
    ** lcd_print()
    ** 
    ** Displays the given string on the LCD-Display of newer machines.
-   ** lcd_print() disables the timer-based led tasklet during its 
+   ** lcd_print() disables the timer-based led work task during its 
    ** execution and enables it afterwards again.
    **
  */
@@ -634,8 +677,13 @@ int lcd_print( char *str )
 	if (!led_func_ptr || lcd_info.model != DISPLAY_MODEL_LCD)
 	    return 0;
 	
-	/* temporarily disable the led tasklet */
-	tasklet_disable(&led_tasklet);
+	/* temporarily disable the led work task */
+	if (led_wq) {
+		if (cancel_delayed_work(&led_task) == 0) 
+			flush_workqueue(led_wq);
+	}
+ 	/* copy display string to buffer for procfs */
+ 	strlcpy(lcd_text, str, sizeof(lcd_text));
 
 	/* copy display string to buffer for procfs */
 	strlcpy(lcd_text, str, sizeof(lcd_text));
@@ -653,8 +701,10 @@ int lcd_print( char *str )
 	    udelay(lcd_info.min_cmd_delay);
 	}
 	
-	/* re-enable the led tasklet */
-	tasklet_enable(&led_tasklet);
+	/* re-queue the work */
+	if (led_wq) {
+		queue_work(led_wq, &led_task);
+	}
 
 	return lcd_info.lcd_width;
 }
diff -urpN linux-2.6.12-rc1-pa2/include/asm-parisc/led.h worktree/include/asm-parisc/led.h
--- linux-2.6.12-rc1-pa2/include/asm-parisc/led.h	2004-12-30 08:07:48.000000000 +0000
+++ worktree/include/asm-parisc/led.h	2005-03-24 01:04:54.000000000 +0000
@@ -23,9 +23,6 @@
 
 #define LED_CMD_REG_NONE 0		/* NULL == no addr for the cmd register */
 
-/* led tasklet struct */
-extern struct tasklet_struct led_tasklet;
-
 /* register_led_driver() */
 int __init register_led_driver(int model, unsigned long cmd_reg, unsigned long data_reg);
 

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] A second LCD patch

[David Pye]

[-- Attachment #1.1.1: Type: text/plain, Size: 470 bytes --]

Hi,

Attached is another patch to the LED LCD code, which prints the initial 
'default lcd text' to the display slightly later than previously.

This avoids it being overwritten by the chassis code which is displayed on my 
J5000 shortly after kernel init (INI CC01). The device_init function created 
for the workqueue is an ideal place to do the initial lcd_print now, afaics.

My previous patch needs to be applied first, then this one.

Thanks,

David

[-- Attachment #1.1.2: led_lcd_patch2 --]
[-- Type: text/x-diff, Size: 964 bytes --]

diff -urpN oldtree/drivers/parisc/led.c newtree/drivers/parisc/led.c
--- oldtree/drivers/parisc/led.c	2005-03-24 19:17:25.000000000 +0000
+++ newtree/drivers/parisc/led.c	2005-03-24 19:19:07.000000000 +0000
@@ -128,6 +128,10 @@ lcd_info __attribute__((aligned(8))) =
 /* The workqueue must be created at init-time */
 static int start_task(void) 
 {	
+	/* Print the display text to the lcd if present now. This avoids it
+	 * being overwritten with any boot-time chassis codes */
+	if (led_type == LED_HASLCD) lcd_print( lcd_text_default );
+
 	/* Create the work queue and queue the LED task */
 	led_wq = create_singlethread_workqueue("led_wq");	
 	queue_work(led_wq, &led_task);
@@ -602,7 +606,6 @@ int __init register_led_driver(int model
 		printk(KERN_INFO "LCD display at %lx,%lx registered\n", 
 			LCD_CMD_REG , LCD_DATA_REG);
 		led_func_ptr = led_LCD_driver;
-		lcd_print( lcd_text_default );
 		led_type = LED_HASLCD;
 		break;
 

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[James Bottomley]

On Thu, 2005-03-24 at 18:48 +0000, David Pye wrote:
> +                       set_current_state(TASK_UNINTERRUPTIBLE);                
> +                       schedule_timeout(wait_jiffies); 

The two uses of this should become msleep().  schedule_timeout() is
deprecated in the kernel now.

> +       /* Requeue ourself to run in 5 jiffies */       
> +       queue_delayed_work(led_wq, &led_task, 5);

This is wrong too.  The value of HZ (the rate at which jiffies tick) has
been tampered with several times over the course of the years.  All
timing related quantites need to be expressed in terms of HZ to ensure
they remain constant.

We actually have a class of machines for whom HZ=100 so you'll be
waiting ten times longer than you expect on these...


> +               if (cancel_delayed_work(&led_task) == 0) 
> +                       flush_workqueue(led_wq);

This should really be done using 

cancel_rearming_delayed_workqueue()

Unfortunately, someone needs to get that symbol exported.

This added strlcpy is redundant:


> +       /* copy display string to buffer for procfs */
> +       strlcpy(lcd_text, str, sizeof(lcd_text));
>  
>         /* copy display string to buffer for procfs */
>         strlcpy(lcd_text, str, sizeof(lcd_text));
> 

James



_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[David Pye]

[-- Attachment #1.1.1: Type: text/plain, Size: 431 bytes --]

Hi,

Thanks for your feedback.

I wish the kernel driver development book clearly marked what was deprecated 
these days </whinge>

I believe the attached patch fixes the points you noted, with the exception of 
not yet using cancel_rearming_delayed_workqueue().   

The second patch I submitted still applies okay after this one. (offset -2 
lines thanks to the removal of the duplicated strlcpy).

Thanks,

David

[-- Attachment #1.1.2: led_lcd_patch --]
[-- Type: text/x-diff, Size: 12843 bytes --]

diff -urpN oldtree/arch/parisc/kernel/time.c newtree/arch/parisc/kernel/time.c
--- oldtree/arch/parisc/kernel/time.c	2004-10-21 19:58:24.000000000 +0100
+++ newtree/arch/parisc/kernel/time.c	2005-03-24 20:31:59.000000000 +0000
@@ -89,14 +89,6 @@ irqreturn_t timer_interrupt(int irq, voi
 		}
 	}
     
-#ifdef CONFIG_CHASSIS_LCD_LED
-	/* Only schedule the led tasklet on cpu 0, and only if it
-	 * is enabled.
-	 */
-	if (cpu == 0 && !atomic_read(&led_tasklet.count))
-		tasklet_schedule(&led_tasklet);
-#endif
-
 	/* check soft power switch status */
 	if (cpu == 0 && !atomic_read(&power_tasklet.count))
 		tasklet_schedule(&power_tasklet);
diff -urpN oldtree/drivers/parisc/led.c newtree/drivers/parisc/led.c
--- oldtree/drivers/parisc/led.c	2005-03-18 13:17:10.000000000 +0000
+++ newtree/drivers/parisc/led.c	2005-03-24 20:32:49.000000000 +0000
@@ -18,6 +18,9 @@
  * Changes:
  *      - Audit copy_from_user in led_proc_write.
  *                                Daniele Bellucci <bellucda@tiscali.it>
+ *	- Switch from using a tasklet to a work queue, so the led_LCD_driver
+ *	  	can sleep.
+ *	  			  David Pye <dmp@davidmpye.dyndns.org>
  */
 
 #include <linux/config.h>
@@ -37,6 +40,7 @@
 #include <linux/proc_fs.h>
 #include <linux/ctype.h>
 #include <linux/blkdev.h>
+#include <linux/workqueue.h>
 #include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/hardware.h>
@@ -46,9 +50,9 @@
 #include <asm/uaccess.h>
 
 /* The control of the LEDs and LCDs on PARISC-machines have to be done 
-   completely in software. The necessary calculations are done in a tasklet
-   which is scheduled at every timer interrupt and since the calculations 
-   may consume relatively much CPU-time some of the calculations can be 
+   completely in software. The necessary calculations are done in a work queue
+   task which is scheduled regularly, and since the calculations may consume a 
+   relatively large amount of CPU time, some of the calculations can be 
    turned off with the following variables (controlled via procfs) */
 
 static int led_type = -1;
@@ -58,13 +62,16 @@ static int led_lanrxtx = 1;
 static char lcd_text[32];
 static char lcd_text_default[32];
 
+static struct workqueue_struct *led_wq;
+static void led_work_func(void *);
+static DECLARE_WORK(led_task, led_work_func, NULL);
+
 #if 0
 #define DPRINTK(x)	printk x
 #else
 #define DPRINTK(x)
 #endif
 
-
 struct lcd_block {
 	unsigned char command;	/* stores the command byte      */
 	unsigned char on;	/* value for turning LED on     */
@@ -115,12 +122,24 @@ lcd_info __attribute__((aligned(8))) =
 #define LCD_DATA_REG	lcd_info.lcd_data_reg_addr	 
 #define LED_DATA_REG	lcd_info.lcd_cmd_reg_addr	/* LASI & ASP only */
 
+#define LED_HASLCD 1
+#define LED_NOLCD  0
+
+/* The workqueue must be created at init-time */
+static int start_task(void) 
+{	
+	/* Create the work queue and queue the LED task */
+	led_wq = create_singlethread_workqueue("led_wq");	
+	queue_work(led_wq, &led_task);
+
+	return 0;
+}
+
+device_initcall(start_task);
 
 /* ptr to LCD/LED-specific function */
 static void (*led_func_ptr) (unsigned char);
 
-#define LED_HASLCD 1
-#define LED_NOLCD  0
 #ifdef CONFIG_PROC_FS
 static int led_proc_read(char *page, char **start, off_t off, int count, 
 	int *eof, void *data)
@@ -285,53 +304,37 @@ static void led_LASI_driver(unsigned cha
 /*
    ** 
    ** led_LCD_driver()
-   ** 
-   ** The logic of the LCD driver is, that we write at every scheduled call
-   ** only to one of LCD_CMD_REG _or_ LCD_DATA_REG - registers.
-   ** That way we don't need to let this tasklet busywait for min_cmd_delay
-   ** milliseconds.
-   **
-   ** TODO: check the value of "min_cmd_delay" against the value of HZ.
    **   
  */
 static void led_LCD_driver(unsigned char leds)
 {
-	static int last_index;	/* 0:heartbeat, 1:disk, 2:lan_in, 3:lan_out */
-	static int last_was_cmd;/* 0: CMD was written last, 1: DATA was last */
-	struct lcd_block *block_ptr;
-	int value;
-
-	switch (last_index) {
-	    case 0:	block_ptr = &lcd_info.heartbeat;
-			value = leds & LED_HEARTBEAT;
-			break;
-	    case 1:	block_ptr = &lcd_info.disk_io;
-			value = leds & LED_DISK_IO;
-			break;					
-	    case 2:	block_ptr = &lcd_info.lan_rcv;
-			value = leds & LED_LAN_RCV;
-			break;					
-	    case 3:	block_ptr = &lcd_info.lan_tx;
-			value = leds & LED_LAN_TX;
-			break;
-	    default:	/* should never happen: */
-			return;
-	}
+	static int lastleds, i;
+	static unsigned char mask[4] = { LED_HEARTBEAT, LED_DISK_IO,
+		LED_LAN_RCV, LED_LAN_TX };
+	
+	static struct lcd_block * blockp[4] = {
+		&lcd_info.heartbeat,
+		&lcd_info.disk_io,
+		&lcd_info.lan_rcv,
+		&lcd_info.lan_tx
+	};
 
-	if (last_was_cmd) {
-	    /* write the value to the LCD data port */
-    	    gsc_writeb( value ? block_ptr->on : block_ptr->off, LCD_DATA_REG );
-	} else {
-	    /* write the command-byte to the LCD command register */
-    	    gsc_writeb( block_ptr->command, LCD_CMD_REG );
-	}    
-	
-	/* now update the vars for the next interrupt iteration */ 
-	if (++last_was_cmd == 2) { /* switch between cmd & data */
-	    last_was_cmd = 0;
-	    if (++last_index == 4) 
-		last_index = 0;	 /* switch back to heartbeat index */
+	/* Convert min_cmd_delay to milliseconds */
+	unsigned int msec_cmd_delay = 1 + (lcd_info.min_cmd_delay / 1000);
+	
+	for (i=0; i<4; ++i) 
+	{
+		if ((leds & mask[i]) != (lastleds & mask[i])) 
+		{
+			gsc_writeb( blockp[i]->command, LCD_CMD_REG );
+			msleep(msec_cmd_delay);
+			
+			gsc_writeb( leds & mask[i] ? blockp[i]->on : 
+					blockp[i]->off, LCD_DATA_REG );
+			msleep(msec_cmd_delay);
+		}
 	}
+	lastleds = leds;
 }
 
 
@@ -355,7 +358,7 @@ static __inline__ int led_get_net_activi
 
 	rx_total = tx_total = 0;
 	
-	/* we are running as tasklet, so locking dev_base 
+	/* we are running as a workqueue task, so locking dev_base 
 	 * for reading should be OK */
 	read_lock(&dev_base_lock);
 	for (dev = dev_base; dev; dev = dev->next) {
@@ -417,10 +420,9 @@ static __inline__ int led_get_diskio_act
 
 
 /*
-   ** led_tasklet_func()
+   ** led_work_func()
    ** 
-   ** is scheduled at every timer interrupt from time.c and
-   ** updates the chassis LCD/LED 
+   ** is scheduled every 5 milliseconds and updates the chassis LCD/LED 
 
     TODO:
     - display load average (older machines like 715/64 have 4 "free" LED's for that)
@@ -431,66 +433,104 @@ static __inline__ int led_get_diskio_act
 #define HEARTBEAT_2ND_RANGE_START (HZ*22/100)
 #define HEARTBEAT_2ND_RANGE_END   (HEARTBEAT_2ND_RANGE_START + HEARTBEAT_LEN)
 
-#define NORMALIZED_COUNT(count) (count/(HZ/100))
+#define LED_MINIMUM_ON_TIME (HZ*10/100)
 
-static void led_tasklet_func(unsigned long unused)
+#define LED_UPDATE_INTERVAL (1 + (HZ*4/1000))
+
+static void led_work_func (void *unused)
 {
 	static unsigned char lastleds;
-	unsigned char currentleds; /* stores current value of the LEDs */
-	static unsigned long count; /* static incremented value, not wrapped */
 	static unsigned long count_HZ; /* counter in range 0..HZ */
 
+	static unsigned long diskio_on_jiffies;
+	static unsigned long lantx_on_jiffies;
+	static unsigned long lanrcv_on_jiffies;
+
+	unsigned char currentleds = 0; /* stores current value of the LEDs */
+	
 	/* exit if not initialized */
 	if (!led_func_ptr)
 	    return;
 
-	/* increment the local counters */
-	++count;
-	if (++count_HZ == HZ)
+	/* increment the local counter */
+	count_HZ += LED_UPDATE_INTERVAL;
+	if (count_HZ >= HZ)
 	    count_HZ = 0;
 
-	currentleds = lastleds;
-
 	if (led_heartbeat)
 	{
 		/* flash heartbeat-LED like a real heart (2 x short then a long delay) */
 		if (count_HZ<HEARTBEAT_LEN || 
 		    (count_HZ>=HEARTBEAT_2ND_RANGE_START && count_HZ<HEARTBEAT_2ND_RANGE_END)) 
 		    currentleds |= LED_HEARTBEAT;
-		else
-		    currentleds &= ~LED_HEARTBEAT;
 	}
 
-	/* look for network activity and flash LEDs respectively */
-	if (led_lanrxtx && ((NORMALIZED_COUNT(count)+(8/2)) & 7) == 0)
+	/* If the leds are on, but should be turned off, make sure they have been on for
+	 * the minimum time, to prevent 'flutter' 
+	 */
+
+	/* look for network activity and flash LEDs accordingly */
+	if (led_lanrxtx)
 	{
-		currentleds &= ~(LED_LAN_RCV | LED_LAN_TX);
-		currentleds |= led_get_net_activity();
+		unsigned char new_lan_leds = led_get_net_activity();
+	
+		/* TX LED */
+		if (new_lan_leds & LED_LAN_TX) 
+		{
+			currentleds |= LED_LAN_TX;
+			lantx_on_jiffies = jiffies;
+		}
+		else if (lastleds & LED_LAN_TX && time_before(jiffies, lantx_on_jiffies + LED_MINIMUM_ON_TIME))
+		{
+			currentleds |= LED_LAN_TX;
+		}
+		
+		/* RCV LED */
+
+		if (new_lan_leds & LED_LAN_RCV) 
+		{
+			currentleds |= LED_LAN_RCV;
+			lanrcv_on_jiffies = jiffies;
+		}
+		else if (lastleds & LED_LAN_RCV && time_before(jiffies, lanrcv_on_jiffies + LED_MINIMUM_ON_TIME)) 
+		{
+			currentleds |= LED_LAN_RCV;
+		}
 	}
 
-	/* avoid to calculate diskio-stats at same irq  as netio-stats */
-	if (led_diskio && (NORMALIZED_COUNT(count) & 7) == 0)
+	/* Calculate disk IO */
+	if (led_diskio)
 	{
-		currentleds &= ~LED_DISK_IO;
-		currentleds |= led_get_diskio_activity();
+		unsigned char new_io_led = led_get_diskio_activity();
+		
+		if (new_io_led & LED_DISK_IO) 
+		{
+			currentleds |= LED_DISK_IO;
+			diskio_on_jiffies = jiffies;
+		}
+		else if (lastleds & LED_DISK_IO && time_before(jiffies, diskio_on_jiffies + LED_MINIMUM_ON_TIME)) 
+		{
+			currentleds |= LED_DISK_IO;
+		}
 	}
 
 	/* blink all LEDs twice a second if we got an Oops (HPMC) */
-	if (oops_in_progress) {
+	if (oops_in_progress) 
+	{
 		currentleds = (count_HZ<=(HZ/2)) ? 0 : 0xff;
 	}
 	
 	/* update the LCD/LEDs */
-	if (currentleds != lastleds) {
-	    led_func_ptr(currentleds);
-	    lastleds = currentleds;
+	if (currentleds != lastleds)
+	{
+		led_func_ptr(currentleds);
+		lastleds = currentleds;
 	}
+	
+	/* Requeue ourself to run in 5 jiffies */	
+	queue_delayed_work(led_wq, &led_task, LED_UPDATE_INTERVAL);
 }
 
-/* main led tasklet struct (scheduled from time.c) */
-DECLARE_TASKLET_DISABLED(led_tasklet, led_tasklet_func, 0);
-
-
 /*
    ** led_halt()
    ** 
@@ -519,9 +559,13 @@ static int led_halt(struct notifier_bloc
 	default:		return NOTIFY_DONE;
 	}
 	
-	/* completely stop the LED/LCD tasklet */
-	tasklet_disable(&led_tasklet);
-
+	/* Cancel the work item and delete the queue */
+	if (led_wq) {
+		if (cancel_delayed_work(&led_task) == 0) 
+			flush_workqueue(led_wq);
+		destroy_workqueue(led_wq);
+	}
+ 
 	if (lcd_info.model == DISPLAY_MODEL_LCD)
 		lcd_print(txt);
 	else
@@ -586,9 +630,6 @@ int __init register_led_driver(int model
 	initialized++;
 	register_reboot_notifier(&led_notifier);
 
-	/* start the led tasklet for the first time */
-	tasklet_enable(&led_tasklet);
-	
 	return 0;
 }
 
@@ -623,7 +664,7 @@ void __init register_led_regions(void)
    ** lcd_print()
    ** 
    ** Displays the given string on the LCD-Display of newer machines.
-   ** lcd_print() disables the timer-based led tasklet during its 
+   ** lcd_print() disables the timer-based led work task during its 
    ** execution and enables it afterwards again.
    **
  */
@@ -634,12 +675,14 @@ int lcd_print( char *str )
 	if (!led_func_ptr || lcd_info.model != DISPLAY_MODEL_LCD)
 	    return 0;
 	
-	/* temporarily disable the led tasklet */
-	tasklet_disable(&led_tasklet);
-
+	/* temporarily disable the led work task */
+	if (led_wq) {
+		if (cancel_delayed_work(&led_task) == 0) 
+			flush_workqueue(led_wq);
+	}
 	/* copy display string to buffer for procfs */
 	strlcpy(lcd_text, str, sizeof(lcd_text));
-	
+
 	/* Set LCD Cursor to 1st character */
 	gsc_writeb(lcd_info.reset_cmd1, LCD_CMD_REG);
 	udelay(lcd_info.min_cmd_delay);
@@ -653,8 +696,10 @@ int lcd_print( char *str )
 	    udelay(lcd_info.min_cmd_delay);
 	}
 	
-	/* re-enable the led tasklet */
-	tasklet_enable(&led_tasklet);
+	/* re-queue the work */
+	if (led_wq) {
+		queue_work(led_wq, &led_task);
+	}
 
 	return lcd_info.lcd_width;
 }
diff -urpN oldtree/include/asm-parisc/led.h newtree/include/asm-parisc/led.h
--- oldtree/include/asm-parisc/led.h	2004-12-30 08:07:48.000000000 +0000
+++ newtree/include/asm-parisc/led.h	2005-03-24 20:31:59.000000000 +0000
@@ -23,9 +23,6 @@
 
 #define LED_CMD_REG_NONE 0		/* NULL == no addr for the cmd register */
 
-/* led tasklet struct */
-extern struct tasklet_struct led_tasklet;
-
 /* register_led_driver() */
 int __init register_led_driver(int model, unsigned long cmd_reg, unsigned long data_reg);
 

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[Grant Grundler]

On Thu, Mar 24, 2005 at 09:00:05PM +0000, David Pye wrote:
...
> +	for (i=0; i<4; ++i) 
> +	{
> +		if ((leds & mask[i]) != (lastleds & mask[i])) 
> +		{
> +			gsc_writeb( blockp[i]->command, LCD_CMD_REG );
> +			msleep(msec_cmd_delay);
> +			
> +			gsc_writeb( leds & mask[i] ? blockp[i]->on : 
> +					blockp[i]->off, LCD_DATA_REG );
> +			msleep(msec_cmd_delay);
> +		}
>  	}
> +	lastleds = leds;

I still *really* like this bit. Good work!

> -   ** updates the chassis LCD/LED 
> +   ** is scheduled every 5 milliseconds and updates the chassis LCD/LED 
...
> -#define NORMALIZED_COUNT(count) (count/(HZ/100))
> +#define LED_MINIMUM_ON_TIME (HZ*10/100)

Why not just update every 10ms and ditch all the "minimum on time" stuff?
Can someone tell if an LED/LCD came on 5ms later than it should have been?

> +	static unsigned long diskio_on_jiffies;
> +	static unsigned long lantx_on_jiffies;
> +	static unsigned long lanrcv_on_jiffies;

If not, then this bit and...

> +	/* If the leds are on, but should be turned off, make sure they have been on for
> +	 * the minimum time, to prevent 'flutter' 
> +	 */

...and...

> +		else if (lastleds & LED_LAN_TX && time_before(jiffies, lantx_on_jiffies + LED_MINIMUM_ON_TIME))
> +		{
> +			currentleds |= LED_LAN_TX;
> +		}

can go away. Ditto for DiskIO.

We already know heartbeat stays on/off longer than that (about).

>  	/* blink all LEDs twice a second if we got an Oops (HPMC) */
> -	if (oops_in_progress) {
> +	if (oops_in_progress) 
> +	{
>  		currentleds = (count_HZ<=(HZ/2)) ? 0 : 0xff;
>  	}

Will this work for LCD?
Will the itimer continue to run and workq continue to get serviced?
Even if it does, I'm nervous about it.
Maybe stop all activity at that point?

The panic chassis log will get printed and that should be a good indicator.
Anyway, this is a seperate issue from the rest of the patch.

thanks,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[David Pye]

[-- Attachment #1.1: Type: text/plain, Size: 2446 bytes --]

On Friday 25 March 2005 06:25, you wrote:
> On Thu, Mar 24, 2005 at 09:00:05PM +0000, David Pye wrote:
> ...
>
> > + for (i=0; i<4; ++i)
> > + {
> > +  if ((leds & mask[i]) != (lastleds & mask[i]))
> > +  {
> > +   gsc_writeb( blockp[i]->command, LCD_CMD_REG );
> > +   msleep(msec_cmd_delay);
> > +
> > +   gsc_writeb( leds & mask[i] ? blockp[i]->on :
> > +     blockp[i]->off, LCD_DATA_REG );
> > +   msleep(msec_cmd_delay);
> > +  }
> >   }
> > + lastleds = leds;
>
> I still *really* like this bit. Good work!

Thankyou :)

> > -   ** updates the chassis LCD/LED
> > +   ** is scheduled every 5 milliseconds and updates the chassis LCD/LED
>
> ...
>
> > -#define NORMALIZED_COUNT(count) (count/(HZ/100))
> > +#define LED_MINIMUM_ON_TIME (HZ*10/100)
>
> Why not just update every 10ms and ditch all the "minimum on time" stuff?
> Can someone tell if an LED/LCD came on 5ms later than it should have been?

There is a reason for this, though there may be a better way of doing it.  

If you remove it, and set the update interval to 10mS, what tends to happen is 
that because each time you flip a symbol on the lcd you have to wait 2 
jiffies (once before cmd reg, once before writing data reg), and sometimes 
more, because the kernel may put you to sleep a bit longer than the time you 
asked for,  is that the heartbeat starts skipping beats when the io/lan leds 
are changing on/off all the time.

If you really don't like the minimum_on_time, another solution is to leave the 
work queued every five secs, and make sure you service the heartbeat on one 
cycle, and the led/diskio on the other.  If you think that's cleaner, I can 
change it to do that.

> >   /* blink all LEDs twice a second if we got an Oops (HPMC) */
> > - if (oops_in_progress) {
> > + if (oops_in_progress)
> > + {
> >    currentleds = (count_HZ<=(HZ/2)) ? 0 : 0xff;
> >   }
>
> Will this work for LCD?
> Will the itimer continue to run and workq continue to get serviced?
> Even if it does, I'm nervous about it.
> Maybe stop all activity at that point?
> The panic chassis log will get printed and that should be a good indicator.
> Anyway, this is a seperate issue from the rest of the patch.

I think it'd be better to just rewrite that so that the work only requeues 
itself if an oops ISNT in progress, but as you say, it wasn't something I 
changed!

Cheers,

David

>
> thanks,
> grant

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[David Pye]

[-- Attachment #1.1: Type: text/plain, Size: 1733 bytes --]

On Friday 25 March 2005 17:05, David Pye wrote:
<snip>
> >
> > > -#define NORMALIZED_COUNT(count) (count/(HZ/100))
> > > +#define LED_MINIMUM_ON_TIME (HZ*10/100)
> >
> > Why not just update every 10ms and ditch all the "minimum on time" stuff?
> > Can someone tell if an LED/LCD came on 5ms later than it should have
> > been?

Ah, I remember now.

That's 100mS, not 10mS for a start ;)  You can't make the whole update 
interval 100mS, of course, or you'll miss the heartbeats completely.

> There is a reason for this, though there may be a better way of doing it.
>
> If you remove it, and set the update interval to 10mS, what tends to happen
> is that because each time you flip a symbol on the lcd you have to wait 2
> jiffies (once before cmd reg, once before writing data reg), and sometimes
> more, because the kernel may put you to sleep a bit longer than the time
> you asked for,  is that the heartbeat starts skipping beats when the io/lan
> leds are changing on/off all the time.
>
> If you really don't like the minimum_on_time, another solution is to leave
> the work queued every five secs, and make sure you service the heartbeat on
> one cycle, and the led/diskio on the other.  If you think that's cleaner, I
> can change it to do that.

Having spent the evening trying that, I've found that the 100mS minimum 
'flutter stop' interval patch gives the most consistent heartbeat of the 
ideas that I've tried. The solution I suggested above causes the heartbeat to 
become uneven when LAN/DISK IO is present still.

If the 100mS minimum LED illumination bothers people who have 'real' LEDs as 
opposed to LCD indicators, we can make it LCD only, of course.


Cheers,

David

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[Grant Grundler]

On Fri, Mar 25, 2005 at 09:50:26PM +0000, David Pye wrote:
> On Friday 25 March 2005 17:05, David Pye wrote:
> > > Why not just update every 10ms and ditch all the "minimum on time" stuff?
> > > Can someone tell if an LED/LCD came on 5ms later than it should have
> > > been?
> 
> Ah, I remember now.
> 
> That's 100mS, not 10mS for a start ;)  You can't make the whole update 
> interval 100mS, of course, or you'll miss the heartbeats completely.

Sorry, I wasn't clear. I meant check if LED/LCD status needs to change
every 10ms. The msleep would still let us bang out 8 register writes,
one per HZ. Ie make the "lets see if activity is different now" check
less frequent so LED/LCD is either on or off long enough to be visible.

100ms seems a bit longer than I would like. But if that works, go for it.

> > There is a reason for this, though there may be a better way of doing it.
> >
> > If you remove it, and set the update interval to 10mS, what tends to happen
> > is that because each time you flip a symbol on the lcd you have to wait 2
> > jiffies (once before cmd reg, once before writing data reg), and sometimes
> > more, because the kernel may put you to sleep a bit longer than the time
> > you asked for,  is that the heartbeat starts skipping beats when the io/lan
> > leds are changing on/off all the time.

My point was
1) don't update LED/LCD state again until the previous update has completed.
   ie all the register writes have been sent.

2) we shouldn't touch Heartbeat register for LCD unless heartbeat needs to be updated.

I'm still really not clear why there is "flutter".  THe only reasonable case I can think
of is flutter is an artifact of the CPU busy doing "real" work and not servicing the
LED/LCD wq. i.e. the stress test to excercise networking/disk activity is causing the
CPU to work on lots of other tasks and it's taking more than a few jiffies to get back
to the LED/LCD wq.

> 
> Having spent the evening trying that, I've found that the 100mS minimum 
> 'flutter stop' interval patch gives the most consistent heartbeat of the 
> ideas that I've tried. The solution I suggested above causes the heartbeat to 
> become uneven when LAN/DISK IO is present still.

Yeah, 100ms is well within someone noticing that LED/LCD activity isn't
sync'd with LEDs on NIC or Disk. But I'm ok with that. If they got the real
LED, the display panel can be ignored.

> If the 100mS minimum LED illumination bothers people who have 'real' LEDs as 
> opposed to LCD indicators, we can make it LCD only, of course.

Sure. throw another patch out there! You can enumerate them...this is #3 or #4...

thanks,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[David Pye]

[-- Attachment #1.1.1: Type: text/plain, Size: 3590 bytes --]

On Saturday 26 March 2005 07:13, Grant Grundler wrote:

> Sorry, I wasn't clear. I meant check if LED/LCD status needs to change
> every 10ms. The msleep would still let us bang out 8 register writes,
> one per HZ. Ie make the "lets see if activity is different now" check
> less frequent so LED/LCD is either on or off long enough to be visible.
>
> 100ms seems a bit longer than I would like. But if that works, go for it.

I did try reducing it to 50, and found that it caused heartbeat to become 
irregular under heavy load.

> > > There is a reason for this, though there may be a better way of doing
> > > it.
> > >
> > > If you remove it, and set the update interval to 10mS, what tends to
> > > happen is that because each time you flip a symbol on the lcd you have
> > > to wait 2 jiffies (once before cmd reg, once before writing data reg),
> > > and sometimes more, because the kernel may put you to sleep a bit
> > > longer than the time you asked for,  is that the heartbeat starts
> > > skipping beats when the io/lan leds are changing on/off all the time.
>
> My point was
> 1) don't update LED/LCD state again until the previous update has
> completed. ie all the register writes have been sent.

Yep, we don't do that.

> 2) we shouldn't touch Heartbeat register for LCD unless heartbeat needs to
> be updated.

It already does that too.

> I'm still really not clear why there is "flutter".  THe only reasonable
> case I can think of is flutter is an artifact of the CPU busy doing "real"
> work and not servicing the LED/LCD wq. i.e. the stress test to excercise
> networking/disk activity is causing the CPU to work on lots of other tasks
> and it's taking more than a few jiffies to get back to the LED/LCD wq.

Yeah, it's a little mysterious. What I think is happening is that when we ask 
to msleep between the writes, because we're not busy-waiting, we might be put 
to sleep for far longer than we asked for.  Ditto again, for when we ask to 
be requeued - we ask to run every 10mS but we might end up waiting for far 
longer than that.  That's about the best explanation I've come up with so 
far.  By ensuring that we do not update the io/net symbols more often than 
necessary, we can save ourselves some of these sleeps, so giving the kernel 
less opportunity to put us to sleep!

> > Having spent the evening trying that, I've found that the 100mS minimum
> > 'flutter stop' interval patch gives the most consistent heartbeat of the
> > ideas that I've tried. The solution I suggested above causes the
> > heartbeat to become uneven when LAN/DISK IO is present still.
>
> Yeah, 100ms is well within someone noticing that LED/LCD activity isn't
> sync'd with LEDs on NIC or Disk. But I'm ok with that. If they got the real
> LED, the display panel can be ignored.

Very true.

> > If the 100mS minimum LED illumination bothers people who have 'real' LEDs
> > as opposed to LCD indicators, we can make it LCD only, of course.
>
> Sure. throw another patch out there! You can enumerate them...this is #3 or
> #4...

Attached! The only real change now is that the minimum on-time is now LCD 
only, so hopefully this gets us the best of both worlds. LED people won't 
notice anything different, and the LCD should now be nicely responsive.

Cheers,

David

> thanks,
> grant

-- 
-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCS d- s-: a-- C++ UL++++ P L+++ E--- W++ N+ o+ K- w---
O M V- PS+ PE+ Y+ PGP t 5- X+ R- tv+ b+ DI++ D+
G+ e++ h--- r++ y++
------END GEEK CODE BLOCK------

[-- Attachment #1.1.2: lcd_patch4 --]
[-- Type: text/x-diff, Size: 13189 bytes --]

diff -urpN linux-2.6.12-rc1-pa2/arch/parisc/kernel/time.c patched/arch/parisc/kernel/time.c
--- linux-2.6.12-rc1-pa2/arch/parisc/kernel/time.c	2004-10-21 19:58:24.000000000 +0100
+++ patched/arch/parisc/kernel/time.c	2005-03-26 14:30:39.000000000 +0000
@@ -89,14 +89,6 @@ irqreturn_t timer_interrupt(int irq, voi
 		}
 	}
     
-#ifdef CONFIG_CHASSIS_LCD_LED
-	/* Only schedule the led tasklet on cpu 0, and only if it
-	 * is enabled.
-	 */
-	if (cpu == 0 && !atomic_read(&led_tasklet.count))
-		tasklet_schedule(&led_tasklet);
-#endif
-
 	/* check soft power switch status */
 	if (cpu == 0 && !atomic_read(&power_tasklet.count))
 		tasklet_schedule(&power_tasklet);
diff -urpN linux-2.6.12-rc1-pa2/drivers/parisc/led.c patched/drivers/parisc/led.c
--- linux-2.6.12-rc1-pa2/drivers/parisc/led.c	2005-03-18 13:17:10.000000000 +0000
+++ patched/drivers/parisc/led.c	2005-03-26 14:42:35.000000000 +0000
@@ -18,6 +18,9 @@
  * Changes:
  *      - Audit copy_from_user in led_proc_write.
  *                                Daniele Bellucci <bellucda@tiscali.it>
+ *	- Switch from using a tasklet to a work queue, so the led_LCD_driver
+ *	  	can sleep.
+ *	  			  David Pye <dmp@davidmpye.dyndns.org>
  */
 
 #include <linux/config.h>
@@ -37,6 +40,7 @@
 #include <linux/proc_fs.h>
 #include <linux/ctype.h>
 #include <linux/blkdev.h>
+#include <linux/workqueue.h>
 #include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/hardware.h>
@@ -46,9 +50,9 @@
 #include <asm/uaccess.h>
 
 /* The control of the LEDs and LCDs on PARISC-machines have to be done 
-   completely in software. The necessary calculations are done in a tasklet
-   which is scheduled at every timer interrupt and since the calculations 
-   may consume relatively much CPU-time some of the calculations can be 
+   completely in software. The necessary calculations are done in a work queue
+   task which is scheduled regularly, and since the calculations may consume a 
+   relatively large amount of CPU time, some of the calculations can be 
    turned off with the following variables (controlled via procfs) */
 
 static int led_type = -1;
@@ -58,13 +62,16 @@ static int led_lanrxtx = 1;
 static char lcd_text[32];
 static char lcd_text_default[32];
 
+static struct workqueue_struct *led_wq;
+static void led_work_func(void *);
+static DECLARE_WORK(led_task, led_work_func, NULL);
+
 #if 0
 #define DPRINTK(x)	printk x
 #else
 #define DPRINTK(x)
 #endif
 
-
 struct lcd_block {
 	unsigned char command;	/* stores the command byte      */
 	unsigned char on;	/* value for turning LED on     */
@@ -115,12 +122,24 @@ lcd_info __attribute__((aligned(8))) =
 #define LCD_DATA_REG	lcd_info.lcd_data_reg_addr	 
 #define LED_DATA_REG	lcd_info.lcd_cmd_reg_addr	/* LASI & ASP only */
 
+#define LED_HASLCD 1
+#define LED_NOLCD  0
+
+/* The workqueue must be created at init-time */
+static int start_task(void) 
+{	
+	/* Create the work queue and queue the LED task */
+	led_wq = create_singlethread_workqueue("led_wq");	
+	queue_work(led_wq, &led_task);
+
+	return 0;
+}
+
+device_initcall(start_task);
 
 /* ptr to LCD/LED-specific function */
 static void (*led_func_ptr) (unsigned char);
 
-#define LED_HASLCD 1
-#define LED_NOLCD  0
 #ifdef CONFIG_PROC_FS
 static int led_proc_read(char *page, char **start, off_t off, int count, 
 	int *eof, void *data)
@@ -285,53 +304,37 @@ static void led_LASI_driver(unsigned cha
 /*
    ** 
    ** led_LCD_driver()
-   ** 
-   ** The logic of the LCD driver is, that we write at every scheduled call
-   ** only to one of LCD_CMD_REG _or_ LCD_DATA_REG - registers.
-   ** That way we don't need to let this tasklet busywait for min_cmd_delay
-   ** milliseconds.
-   **
-   ** TODO: check the value of "min_cmd_delay" against the value of HZ.
    **   
  */
 static void led_LCD_driver(unsigned char leds)
 {
-	static int last_index;	/* 0:heartbeat, 1:disk, 2:lan_in, 3:lan_out */
-	static int last_was_cmd;/* 0: CMD was written last, 1: DATA was last */
-	struct lcd_block *block_ptr;
-	int value;
-
-	switch (last_index) {
-	    case 0:	block_ptr = &lcd_info.heartbeat;
-			value = leds & LED_HEARTBEAT;
-			break;
-	    case 1:	block_ptr = &lcd_info.disk_io;
-			value = leds & LED_DISK_IO;
-			break;					
-	    case 2:	block_ptr = &lcd_info.lan_rcv;
-			value = leds & LED_LAN_RCV;
-			break;					
-	    case 3:	block_ptr = &lcd_info.lan_tx;
-			value = leds & LED_LAN_TX;
-			break;
-	    default:	/* should never happen: */
-			return;
-	}
+	static int lastleds, i;
+	static unsigned char mask[4] = { LED_HEARTBEAT, LED_DISK_IO,
+		LED_LAN_RCV, LED_LAN_TX };
+	
+	static struct lcd_block * blockp[4] = {
+		&lcd_info.heartbeat,
+		&lcd_info.disk_io,
+		&lcd_info.lan_rcv,
+		&lcd_info.lan_tx
+	};
 
-	if (last_was_cmd) {
-	    /* write the value to the LCD data port */
-    	    gsc_writeb( value ? block_ptr->on : block_ptr->off, LCD_DATA_REG );
-	} else {
-	    /* write the command-byte to the LCD command register */
-    	    gsc_writeb( block_ptr->command, LCD_CMD_REG );
-	}    
-	
-	/* now update the vars for the next interrupt iteration */ 
-	if (++last_was_cmd == 2) { /* switch between cmd & data */
-	    last_was_cmd = 0;
-	    if (++last_index == 4) 
-		last_index = 0;	 /* switch back to heartbeat index */
+	/* Convert min_cmd_delay to milliseconds */
+	unsigned int msec_cmd_delay = 1 + (lcd_info.min_cmd_delay / 1000);
+	
+	for (i=0; i<4; ++i) 
+	{
+		if ((leds & mask[i]) != (lastleds & mask[i])) 
+		{
+			gsc_writeb( blockp[i]->command, LCD_CMD_REG );
+			msleep(msec_cmd_delay);
+			
+			gsc_writeb( leds & mask[i] ? blockp[i]->on : 
+					blockp[i]->off, LCD_DATA_REG );
+			msleep(msec_cmd_delay);
+		}
 	}
+	lastleds = leds;
 }
 
 
@@ -355,7 +358,7 @@ static __inline__ int led_get_net_activi
 
 	rx_total = tx_total = 0;
 	
-	/* we are running as tasklet, so locking dev_base 
+	/* we are running as a workqueue task, so locking dev_base 
 	 * for reading should be OK */
 	read_lock(&dev_base_lock);
 	for (dev = dev_base; dev; dev = dev->next) {
@@ -417,10 +420,9 @@ static __inline__ int led_get_diskio_act
 
 
 /*
-   ** led_tasklet_func()
+   ** led_work_func()
    ** 
-   ** is scheduled at every timer interrupt from time.c and
-   ** updates the chassis LCD/LED 
+   ** is scheduled every 5 milliseconds and updates the chassis LCD/LED 
 
     TODO:
     - display load average (older machines like 715/64 have 4 "free" LED's for that)
@@ -431,66 +433,106 @@ static __inline__ int led_get_diskio_act
 #define HEARTBEAT_2ND_RANGE_START (HZ*22/100)
 #define HEARTBEAT_2ND_RANGE_END   (HEARTBEAT_2ND_RANGE_START + HEARTBEAT_LEN)
 
-#define NORMALIZED_COUNT(count) (count/(HZ/100))
+#define LCD_LED_MINIMUM_ON_TIME (HZ*10/100)
 
-static void led_tasklet_func(unsigned long unused)
+#define LED_UPDATE_INTERVAL (1 + (HZ*4/1000))
+
+static void led_work_func (void *unused)
 {
 	static unsigned char lastleds;
-	unsigned char currentleds; /* stores current value of the LEDs */
-	static unsigned long count; /* static incremented value, not wrapped */
 	static unsigned long count_HZ; /* counter in range 0..HZ */
 
+	static unsigned long diskio_on_jiffies;
+	static unsigned long lantx_on_jiffies;
+	static unsigned long lanrcv_on_jiffies;
+
+	unsigned char currentleds = 0; /* stores current value of the LEDs */
+	
 	/* exit if not initialized */
 	if (!led_func_ptr)
 	    return;
 
-	/* increment the local counters */
-	++count;
-	if (++count_HZ == HZ)
+	/* increment the local counter */
+	count_HZ += LED_UPDATE_INTERVAL;
+	if (count_HZ >= HZ)
 	    count_HZ = 0;
 
-	currentleds = lastleds;
-
 	if (led_heartbeat)
 	{
 		/* flash heartbeat-LED like a real heart (2 x short then a long delay) */
 		if (count_HZ<HEARTBEAT_LEN || 
 		    (count_HZ>=HEARTBEAT_2ND_RANGE_START && count_HZ<HEARTBEAT_2ND_RANGE_END)) 
 		    currentleds |= LED_HEARTBEAT;
-		else
-		    currentleds &= ~LED_HEARTBEAT;
 	}
 
-	/* look for network activity and flash LEDs respectively */
-	if (led_lanrxtx && ((NORMALIZED_COUNT(count)+(8/2)) & 7) == 0)
+	/* look for network activity and flash LEDs accordingly */
+	if (led_lanrxtx)
 	{
-		currentleds &= ~(LED_LAN_RCV | LED_LAN_TX);
-		currentleds |= led_get_net_activity();
+		unsigned char new_lan_leds = led_get_net_activity();
+		/* For LCD based systems, enforce a 'minimum on time' to prevent fluttering. */
+		if (led_type == LED_HASLCD) {
+			/* TX LED */
+			if (new_lan_leds & LED_LAN_TX) 
+			{
+				currentleds |= LED_LAN_TX;
+				lantx_on_jiffies = jiffies;
+			}
+			else if (lastleds & LED_LAN_TX && time_before(jiffies, lantx_on_jiffies + LCD_LED_MINIMUM_ON_TIME))
+			{
+				currentleds |= LED_LAN_TX;
+			}
+			
+			/* RCV LED */
+
+			if (new_lan_leds & LED_LAN_RCV) 
+			{
+				currentleds |= LED_LAN_RCV;
+				lanrcv_on_jiffies = jiffies;
+			}
+			else if (lastleds & LED_LAN_RCV && time_before(jiffies, lanrcv_on_jiffies + LCD_LED_MINIMUM_ON_TIME)) 
+			{
+				currentleds |= LED_LAN_RCV;
+			}
+		}
+		else currentleds |= new_lan_leds;
 	}
 
-	/* avoid to calculate diskio-stats at same irq  as netio-stats */
-	if (led_diskio && (NORMALIZED_COUNT(count) & 7) == 0)
+	/* Calculate disk IO */
+	if (led_diskio)
 	{
-		currentleds &= ~LED_DISK_IO;
-		currentleds |= led_get_diskio_activity();
+		unsigned char new_io_led = led_get_diskio_activity();
+		/* For LCD based systems, enforce a 'minimum on time' to prevent fluttering. */
+		if (led_type == LED_HASLCD) {
+			if (new_io_led & LED_DISK_IO) 
+			{
+				currentleds |= LED_DISK_IO;
+					diskio_on_jiffies = jiffies;
+			}
+			else if (lastleds & LED_DISK_IO && time_before(jiffies, diskio_on_jiffies + LCD_LED_MINIMUM_ON_TIME)) 
+			{
+				currentleds |= LED_DISK_IO;
+			}
+		}
+		else currentleds |= new_io_led;
 	}
 
 	/* blink all LEDs twice a second if we got an Oops (HPMC) */
-	if (oops_in_progress) {
+	if (oops_in_progress) 
+	{
 		currentleds = (count_HZ<=(HZ/2)) ? 0 : 0xff;
 	}
 	
-	/* update the LCD/LEDs */
-	if (currentleds != lastleds) {
-	    led_func_ptr(currentleds);
-	    lastleds = currentleds;
+	/* Update the LCD/LEDs if they've changed */
+	if (currentleds != lastleds)
+	{
+		led_func_ptr(currentleds);
+		lastleds = currentleds;
 	}
+	
+	/* Requeue ourself to run in 5 jiffies */	
+	queue_delayed_work(led_wq, &led_task, LED_UPDATE_INTERVAL);
 }
 
-/* main led tasklet struct (scheduled from time.c) */
-DECLARE_TASKLET_DISABLED(led_tasklet, led_tasklet_func, 0);
-
-
 /*
    ** led_halt()
    ** 
@@ -519,9 +561,13 @@ static int led_halt(struct notifier_bloc
 	default:		return NOTIFY_DONE;
 	}
 	
-	/* completely stop the LED/LCD tasklet */
-	tasklet_disable(&led_tasklet);
-
+	/* Cancel the work item and delete the queue */
+	if (led_wq) {
+		if (cancel_delayed_work(&led_task) == 0) 
+			flush_workqueue(led_wq);
+		destroy_workqueue(led_wq);
+	}
+ 
 	if (lcd_info.model == DISPLAY_MODEL_LCD)
 		lcd_print(txt);
 	else
@@ -586,9 +632,6 @@ int __init register_led_driver(int model
 	initialized++;
 	register_reboot_notifier(&led_notifier);
 
-	/* start the led tasklet for the first time */
-	tasklet_enable(&led_tasklet);
-	
 	return 0;
 }
 
@@ -623,7 +666,7 @@ void __init register_led_regions(void)
    ** lcd_print()
    ** 
    ** Displays the given string on the LCD-Display of newer machines.
-   ** lcd_print() disables the timer-based led tasklet during its 
+   ** lcd_print() disables the timer-based led work task during its 
    ** execution and enables it afterwards again.
    **
  */
@@ -634,12 +677,14 @@ int lcd_print( char *str )
 	if (!led_func_ptr || lcd_info.model != DISPLAY_MODEL_LCD)
 	    return 0;
 	
-	/* temporarily disable the led tasklet */
-	tasklet_disable(&led_tasklet);
-
+	/* temporarily disable the led work task */
+	if (led_wq) {
+		if (cancel_delayed_work(&led_task) == 0) 
+			flush_workqueue(led_wq);
+	}
 	/* copy display string to buffer for procfs */
 	strlcpy(lcd_text, str, sizeof(lcd_text));
-	
+
 	/* Set LCD Cursor to 1st character */
 	gsc_writeb(lcd_info.reset_cmd1, LCD_CMD_REG);
 	udelay(lcd_info.min_cmd_delay);
@@ -653,8 +698,10 @@ int lcd_print( char *str )
 	    udelay(lcd_info.min_cmd_delay);
 	}
 	
-	/* re-enable the led tasklet */
-	tasklet_enable(&led_tasklet);
+	/* re-queue the work */
+	if (led_wq) {
+		queue_work(led_wq, &led_task);
+	}
 
 	return lcd_info.lcd_width;
 }
diff -urpN linux-2.6.12-rc1-pa2/include/asm-parisc/led.h patched/include/asm-parisc/led.h
--- linux-2.6.12-rc1-pa2/include/asm-parisc/led.h	2004-12-30 08:07:48.000000000 +0000
+++ patched/include/asm-parisc/led.h	2005-03-26 14:30:39.000000000 +0000
@@ -23,9 +23,6 @@
 
 #define LED_CMD_REG_NONE 0		/* NULL == no addr for the cmd register */
 
-/* led tasklet struct */
-extern struct tasklet_struct led_tasklet;
-
 /* register_led_driver() */
 int __init register_led_driver(int model, unsigned long cmd_reg, unsigned long data_reg);
 

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[James Bottomley]

On Sat, 2005-03-26 at 15:14 +0000, David Pye wrote:
> > I'm still really not clear why there is "flutter".  THe only reasonable
> > case I can think of is flutter is an artifact of the CPU busy doing "real"
> > work and not servicing the LED/LCD wq. i.e. the stress test to excercise
> > networking/disk activity is causing the CPU to work on lots of other tasks
> > and it's taking more than a few jiffies to get back to the LED/LCD wq.
> 
> Yeah, it's a little mysterious. What I think is happening is that when we ask 
> to msleep between the writes, because we're not busy-waiting, we might be put 
> to sleep for far longer than we asked for.  Ditto again, for when we ask to 
> be requeued - we ask to run every 10mS but we might end up waiting for far 
> longer than that.  That's about the best explanation I've come up with so 
> far.  By ensuring that we do not update the io/net symbols more often than 
> necessary, we can save ourselves some of these sleeps, so giving the kernel 
> less opportunity to put us to sleep!

This is a natural consequence of converting from a tasklet to a
workqueue.  workqueues and msleep are controlled by the scheduler.  The
guarantee is not that the task will be awoken at the specified interval,
but that the task will be marked ready to run at the specified interval.
When it actually runs is within the gift of the scheduler and depends
entirely on how many other runnable tasks there are and what priorities
they have.

If you want absolute control of the timings then you have to use a
tasklet or timer (as per the original implementation)

James


_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[David Pye]

[-- Attachment #1.1: Type: text/plain, Size: 1748 bytes --]

On Saturday 26 March 2005 15:33, you wrote:

> This is a natural consequence of converting from a tasklet to a
> workqueue.  workqueues and msleep are controlled by the scheduler.  The
> guarantee is not that the task will be awoken at the specified interval,
> but that the task will be marked ready to run at the specified interval.
> When it actually runs is within the gift of the scheduler and depends
> entirely on how many other runnable tasks there are and what priorities
> they have.
>
> If you want absolute control of the timings then you have to use a
> tasklet or timer (as per the original implementation)

Yep, I agree entirely.  The problem with the previous solution arrived because 
of the fact that by nature of the way the LCD updates work, they're not 
atomic - ie there's a minimum write time between each write.  Because 
tasklets cannot sleep, it ends up turning into a can of worms to ensure that 
you waited long enough, etc. 

The old solution of writing the cmd reg one tasklet run, and the data reg the 
next generated some nasty race conditions that Grant spotted - specifically, 
with lcd_print.  It stops the tasklet, does its thing, and restarts it.  
Unfortunately, if the tasklet had written the cmd reg before, it now expects 
to write the data reg next. This seems to be the cause of the missing lcd 
characters you sometimes ended up with if you tried to make heavy use of 
lcd_print.

Personally, I think the workqueue idea is better, partly because of the fact 
that it greatly simplifies led_LCD_driver, and partly because of the fact 
that by running quite a bit less often, should impose lower CPU time usage.
(and not just because I wrote the patch ;)

Cheers,

David

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[Grant Grundler]

On Sat, Mar 26, 2005 at 03:14:41PM +0000, David Pye wrote:
> Attached! The only real change now is that the minimum on-time is now LCD 
> only, so hopefully this gets us the best of both worlds. LED people won't 
> notice anything different, and the LCD should now be nicely responsive.

I've attached the patch I'm willing to commit...works for me.

And I really like the work-queue implementation. It nicely
solves the cmd/data pair problem and consolidates more of the
support in led.c.  The only drawback I can see so far is the
scheduler may chose to not run the led_func when it needs to
be run. If a machine is really busy, only the hearbeat will
suffer and I expect users will get other clues that the machine
is just really busy.

Sorry - I can't get warm to the implementation of minimum on time.
It was really cluttering up the main routine.
Can I commit this patch for now and then we follow up with another
patch for minimum on-time?

I think I would be ok with it implemented inside the led_get*
activity routines. They have access to lastleds and jiffies and
that should be enough to figure out how long an LED should stay
on if it's already on.  The main routine can stay simple/readable.

I also just realized because led_get* routines are static inline,
led_lanrxtx and led_diskio tests can be moved to the respective
routines as well with no harm to performance. May be a bad idea
though since current construct is pretty obvious what they do.

cheers,
grant


Signed-off-by: Grant Grundler <grundler@parisc-linux.org>

Index: arch/parisc/kernel/time.c
===================================================================
RCS file: /var/cvs/linux-2.6/arch/parisc/kernel/time.c,v
retrieving revision 1.9
diff -u -p -r1.9 time.c
--- arch/parisc/kernel/time.c	21 Oct 2004 18:58:24 -0000	1.9
+++ arch/parisc/kernel/time.c	27 Mar 2005 08:33:00 -0000
@@ -89,14 +89,6 @@ irqreturn_t timer_interrupt(int irq, voi
 		}
 	}
     
-#ifdef CONFIG_CHASSIS_LCD_LED
-	/* Only schedule the led tasklet on cpu 0, and only if it
-	 * is enabled.
-	 */
-	if (cpu == 0 && !atomic_read(&led_tasklet.count))
-		tasklet_schedule(&led_tasklet);
-#endif
-
 	/* check soft power switch status */
 	if (cpu == 0 && !atomic_read(&power_tasklet.count))
 		tasklet_schedule(&power_tasklet);
Index: drivers/parisc/led.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/parisc/led.c,v
retrieving revision 1.12
diff -u -p -r1.12 led.c
--- drivers/parisc/led.c	18 Mar 2005 13:17:10 -0000	1.12
+++ drivers/parisc/led.c	27 Mar 2005 08:33:00 -0000
@@ -18,6 +18,9 @@
  * Changes:
  *      - Audit copy_from_user in led_proc_write.
  *                                Daniele Bellucci <bellucda@tiscali.it>
+ *	- Switch from using a tasklet to a work queue, so the led_LCD_driver
+ *	  	can sleep.
+ *	  			  David Pye <dmp@davidmpye.dyndns.org>
  */
 
 #include <linux/config.h>
@@ -37,6 +40,7 @@
 #include <linux/proc_fs.h>
 #include <linux/ctype.h>
 #include <linux/blkdev.h>
+#include <linux/workqueue.h>
 #include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/hardware.h>
@@ -46,25 +50,30 @@
 #include <asm/uaccess.h>
 
 /* The control of the LEDs and LCDs on PARISC-machines have to be done 
-   completely in software. The necessary calculations are done in a tasklet
-   which is scheduled at every timer interrupt and since the calculations 
-   may consume relatively much CPU-time some of the calculations can be 
+   completely in software. The necessary calculations are done in a work queue
+   task which is scheduled regularly, and since the calculations may consume a 
+   relatively large amount of CPU time, some of the calculations can be 
    turned off with the following variables (controlled via procfs) */
 
 static int led_type = -1;
-static int led_heartbeat = 1;
-static int led_diskio = 1;
-static int led_lanrxtx = 1;
+static unsigned char lastleds;	/* LED state from most recent update */
+static unsigned int led_heartbeat = 1;
+static unsigned int led_diskio = 1;
+static unsigned int led_lanrxtx = 1;
 static char lcd_text[32];
 static char lcd_text_default[32];
 
+
+static struct workqueue_struct *led_wq;
+static void led_work_func(void *);
+static DECLARE_WORK(led_task, led_work_func, NULL);
+
 #if 0
 #define DPRINTK(x)	printk x
 #else
 #define DPRINTK(x)
 #endif
 
-
 struct lcd_block {
 	unsigned char command;	/* stores the command byte      */
 	unsigned char on;	/* value for turning LED on     */
@@ -115,12 +124,24 @@ lcd_info __attribute__((aligned(8))) =
 #define LCD_DATA_REG	lcd_info.lcd_data_reg_addr	 
 #define LED_DATA_REG	lcd_info.lcd_cmd_reg_addr	/* LASI & ASP only */
 
+#define LED_HASLCD 1
+#define LED_NOLCD  0
+
+/* The workqueue must be created at init-time */
+static int start_task(void) 
+{	
+	/* Create the work queue and queue the LED task */
+	led_wq = create_singlethread_workqueue("led_wq");	
+	queue_work(led_wq, &led_task);
+
+	return 0;
+}
+
+device_initcall(start_task);
 
 /* ptr to LCD/LED-specific function */
 static void (*led_func_ptr) (unsigned char);
 
-#define LED_HASLCD 1
-#define LED_NOLCD  0
 #ifdef CONFIG_PROC_FS
 static int led_proc_read(char *page, char **start, off_t off, int count, 
 	int *eof, void *data)
@@ -285,52 +306,35 @@ static void led_LASI_driver(unsigned cha
 /*
    ** 
    ** led_LCD_driver()
-   ** 
-   ** The logic of the LCD driver is, that we write at every scheduled call
-   ** only to one of LCD_CMD_REG _or_ LCD_DATA_REG - registers.
-   ** That way we don't need to let this tasklet busywait for min_cmd_delay
-   ** milliseconds.
-   **
-   ** TODO: check the value of "min_cmd_delay" against the value of HZ.
    **   
  */
 static void led_LCD_driver(unsigned char leds)
 {
-	static int last_index;	/* 0:heartbeat, 1:disk, 2:lan_in, 3:lan_out */
-	static int last_was_cmd;/* 0: CMD was written last, 1: DATA was last */
-	struct lcd_block *block_ptr;
-	int value;
-
-	switch (last_index) {
-	    case 0:	block_ptr = &lcd_info.heartbeat;
-			value = leds & LED_HEARTBEAT;
-			break;
-	    case 1:	block_ptr = &lcd_info.disk_io;
-			value = leds & LED_DISK_IO;
-			break;					
-	    case 2:	block_ptr = &lcd_info.lan_rcv;
-			value = leds & LED_LAN_RCV;
-			break;					
-	    case 3:	block_ptr = &lcd_info.lan_tx;
-			value = leds & LED_LAN_TX;
-			break;
-	    default:	/* should never happen: */
-			return;
-	}
-
-	if (last_was_cmd) {
-	    /* write the value to the LCD data port */
-    	    gsc_writeb( value ? block_ptr->on : block_ptr->off, LCD_DATA_REG );
-	} else {
-	    /* write the command-byte to the LCD command register */
-    	    gsc_writeb( block_ptr->command, LCD_CMD_REG );
-	}    
-	
-	/* now update the vars for the next interrupt iteration */ 
-	if (++last_was_cmd == 2) { /* switch between cmd & data */
-	    last_was_cmd = 0;
-	    if (++last_index == 4) 
-		last_index = 0;	 /* switch back to heartbeat index */
+	static int i;
+	static unsigned char mask[4] = { LED_HEARTBEAT, LED_DISK_IO,
+		LED_LAN_RCV, LED_LAN_TX };
+	
+	static struct lcd_block * blockp[4] = {
+		&lcd_info.heartbeat,
+		&lcd_info.disk_io,
+		&lcd_info.lan_rcv,
+		&lcd_info.lan_tx
+	};
+
+	/* Convert min_cmd_delay to milliseconds */
+	unsigned int msec_cmd_delay = 1 + (lcd_info.min_cmd_delay / 1000);
+	
+	for (i=0; i<4; ++i) 
+	{
+		if ((leds & mask[i]) != (lastleds & mask[i])) 
+		{
+			gsc_writeb( blockp[i]->command, LCD_CMD_REG );
+			msleep(msec_cmd_delay);
+			
+			gsc_writeb( leds & mask[i] ? blockp[i]->on : 
+					blockp[i]->off, LCD_DATA_REG );
+			msleep(msec_cmd_delay);
+		}
 	}
 }
 
@@ -355,7 +359,7 @@ static __inline__ int led_get_net_activi
 
 	rx_total = tx_total = 0;
 	
-	/* we are running as tasklet, so locking dev_base 
+	/* we are running as a workqueue task, so locking dev_base 
 	 * for reading should be OK */
 	read_lock(&dev_base_lock);
 	for (dev = dev_base; dev; dev = dev->next) {
@@ -402,7 +406,7 @@ static __inline__ int led_get_diskio_act
 	static unsigned long last_pgpgin, last_pgpgout;
 	struct page_state pgstat;
 	int changed;
-	
+
 	get_full_page_state(&pgstat); /* get no of sectors in & out */
 
 	/* Just use a very simple calculation here. Do not care about overflow,
@@ -410,86 +414,68 @@ static __inline__ int led_get_diskio_act
 	changed = (pgstat.pgpgin != last_pgpgin) || (pgstat.pgpgout != last_pgpgout);
 	last_pgpgin  = pgstat.pgpgin;
 	last_pgpgout = pgstat.pgpgout;
-	
+
 	return (changed ? LED_DISK_IO : 0);
 }
 
 
 
 /*
-   ** led_tasklet_func()
+   ** led_work_func()
    ** 
-   ** is scheduled at every timer interrupt from time.c and
-   ** updates the chassis LCD/LED 
+   ** manages when and which chassis LCD/LED gets updated
 
     TODO:
     - display load average (older machines like 715/64 have 4 "free" LED's for that)
     - optimizations
  */
 
-#define HEARTBEAT_LEN (HZ*6/100)
-#define HEARTBEAT_2ND_RANGE_START (HZ*22/100)
+#define HEARTBEAT_LEN (HZ*10/100)
+#define HEARTBEAT_2ND_RANGE_START (HZ*28/100)
 #define HEARTBEAT_2ND_RANGE_END   (HEARTBEAT_2ND_RANGE_START + HEARTBEAT_LEN)
 
-#define NORMALIZED_COUNT(count) (count/(HZ/100))
+#define LED_UPDATE_INTERVAL (1 + (HZ*19/1000))
 
-static void led_tasklet_func(unsigned long unused)
+static void led_work_func (void *unused)
 {
-	static unsigned char lastleds;
-	unsigned char currentleds; /* stores current value of the LEDs */
-	static unsigned long count; /* static incremented value, not wrapped */
 	static unsigned long count_HZ; /* counter in range 0..HZ */
+	unsigned char currentleds = 0; /* stores current value of the LEDs */
 
 	/* exit if not initialized */
 	if (!led_func_ptr)
 	    return;
 
-	/* increment the local counters */
-	++count;
-	if (++count_HZ == HZ)
+	/* increment the local counter */
+	count_HZ += LED_UPDATE_INTERVAL;
+	if (count_HZ >= HZ)
 	    count_HZ = 0;
 
-	currentleds = lastleds;
-
 	if (led_heartbeat)
 	{
-		/* flash heartbeat-LED like a real heart (2 x short then a long delay) */
-		if (count_HZ<HEARTBEAT_LEN || 
-		    (count_HZ>=HEARTBEAT_2ND_RANGE_START && count_HZ<HEARTBEAT_2ND_RANGE_END)) 
-		    currentleds |= LED_HEARTBEAT;
-		else
-		    currentleds &= ~LED_HEARTBEAT;
+		/* flash heartbeat-LED like a real heart
+		 * (2 x short then a long delay)
+		 */
+		if (count_HZ < HEARTBEAT_LEN || 
+				(count_HZ >= HEARTBEAT_2ND_RANGE_START &&
+				count_HZ < HEARTBEAT_2ND_RANGE_END)) 
+			currentleds |= LED_HEARTBEAT;
 	}
 
-	/* look for network activity and flash LEDs respectively */
-	if (led_lanrxtx && ((NORMALIZED_COUNT(count)+(8/2)) & 7) == 0)
-	{
-		currentleds &= ~(LED_LAN_RCV | LED_LAN_TX);
-		currentleds |= led_get_net_activity();
-	}
-
-	/* avoid to calculate diskio-stats at same irq  as netio-stats */
-	if (led_diskio && (NORMALIZED_COUNT(count) & 7) == 0)
-	{
-		currentleds &= ~LED_DISK_IO;
-		currentleds |= led_get_diskio_activity();
-	}
+	if (led_lanrxtx)  currentleds |= led_get_net_activity();
+	if (led_diskio)   currentleds |= led_get_diskio_activity();
 
 	/* blink all LEDs twice a second if we got an Oops (HPMC) */
-	if (oops_in_progress) {
+	if (oops_in_progress) 
 		currentleds = (count_HZ<=(HZ/2)) ? 0 : 0xff;
-	}
-	
-	/* update the LCD/LEDs */
-	if (currentleds != lastleds) {
-	    led_func_ptr(currentleds);
-	    lastleds = currentleds;
-	}
-}
 
-/* main led tasklet struct (scheduled from time.c) */
-DECLARE_TASKLET_DISABLED(led_tasklet, led_tasklet_func, 0);
+	if (currentleds != lastleds)
+	{
+		led_func_ptr(currentleds);	/* Update the LCD/LEDs */
+		lastleds = currentleds;
+	}
 
+	queue_delayed_work(led_wq, &led_task, LED_UPDATE_INTERVAL);
+}
 
 /*
    ** led_halt()
@@ -519,9 +505,13 @@ static int led_halt(struct notifier_bloc
 	default:		return NOTIFY_DONE;
 	}
 	
-	/* completely stop the LED/LCD tasklet */
-	tasklet_disable(&led_tasklet);
-
+	/* Cancel the work item and delete the queue */
+	if (led_wq) {
+		if (cancel_delayed_work(&led_task) == 0) 
+			flush_workqueue(led_wq);
+		destroy_workqueue(led_wq);
+	}
+ 
 	if (lcd_info.model == DISPLAY_MODEL_LCD)
 		lcd_print(txt);
 	else
@@ -586,9 +576,6 @@ int __init register_led_driver(int model
 	initialized++;
 	register_reboot_notifier(&led_notifier);
 
-	/* start the led tasklet for the first time */
-	tasklet_enable(&led_tasklet);
-	
 	return 0;
 }
 
@@ -623,7 +610,7 @@ void __init register_led_regions(void)
    ** lcd_print()
    ** 
    ** Displays the given string on the LCD-Display of newer machines.
-   ** lcd_print() disables the timer-based led tasklet during its 
+   ** lcd_print() disables the timer-based led work task during its 
    ** execution and enables it afterwards again.
    **
  */
@@ -634,12 +621,14 @@ int lcd_print( char *str )
 	if (!led_func_ptr || lcd_info.model != DISPLAY_MODEL_LCD)
 	    return 0;
 	
-	/* temporarily disable the led tasklet */
-	tasklet_disable(&led_tasklet);
-
+	/* temporarily disable the led work task */
+	if (led_wq) {
+		if (cancel_delayed_work(&led_task) == 0) 
+			flush_workqueue(led_wq);
+	}
 	/* copy display string to buffer for procfs */
 	strlcpy(lcd_text, str, sizeof(lcd_text));
-	
+
 	/* Set LCD Cursor to 1st character */
 	gsc_writeb(lcd_info.reset_cmd1, LCD_CMD_REG);
 	udelay(lcd_info.min_cmd_delay);
@@ -653,8 +642,10 @@ int lcd_print( char *str )
 	    udelay(lcd_info.min_cmd_delay);
 	}
 	
-	/* re-enable the led tasklet */
-	tasklet_enable(&led_tasklet);
+	/* re-queue the work */
+	if (led_wq) {
+		queue_work(led_wq, &led_task);
+	}
 
 	return lcd_info.lcd_width;
 }
Index: include/asm-parisc/led.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/led.h,v
retrieving revision 1.3
diff -u -p -r1.3 led.h
--- include/asm-parisc/led.h	30 Dec 2004 08:07:48 -0000	1.3
+++ include/asm-parisc/led.h	27 Mar 2005 08:33:00 -0000
@@ -23,9 +23,6 @@
 
 #define LED_CMD_REG_NONE 0		/* NULL == no addr for the cmd register */
 
-/* led tasklet struct */
-extern struct tasklet_struct led_tasklet;

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[David Pye]

[-- Attachment #1.1: Type: text/plain, Size: 2660 bytes --]

On Sunday 27 March 2005 09:53, Grant Grundler wrote:
> On Sat, Mar 26, 2005 at 03:14:41PM +0000, David Pye wrote:
> > Attached! The only real change now is that the minimum on-time is now LCD
> > only, so hopefully this gets us the best of both worlds. LED people won't
> > notice anything different, and the LCD should now be nicely responsive.
>
> I've attached the patch I'm willing to commit...works for me.
>
> And I really like the work-queue implementation. It nicely
> solves the cmd/data pair problem and consolidates more of the
> support in led.c.  The only drawback I can see so far is the
> scheduler may chose to not run the led_func when it needs to
> be run. If a machine is really busy, only the hearbeat will
> suffer and I expect users will get other clues that the machine
> is just really busy.

Yep, that's what I'd guess too.

> Sorry - I can't get warm to the implementation of minimum on time.
> It was really cluttering up the main routine.
> Can I commit this patch for now and then we follow up with another
> patch for minimum on-time?

Sure.  It definitely cluttered up the main routine, and I wasn't particularly 
happy with having to include an if conditional testing led_type in the 
generic routine.  Hopefully a better idea on how to handle it will emerge 
shortly.

> I think I would be ok with it implemented inside the led_get*
> activity routines. They have access to lastleds and jiffies and
> that should be enough to figure out how long an LED should stay
> on if it's already on.  The main routine can stay simple/readable.

Yep, I hadn't really thought of doing it in there - I'll have a play with it 
and maybe annoy everyone with another patch in a bit.   Downside is that it's 
not really the right place to put it *if* you want it to be lcd-only.

The only other way to do it which you might not find as ugly is to move this 
into the drivers.  ie the drivers would need to be given the responsibility 
of deciding whether they need to update anything (ie each of them has to 
store currentleds rather than the generic calculator function). That way, 
they'll get called each time the work runs, but can decide for themselves 
whether to actually do anything.  It's only a couple of lines to add to the 
other functions e.g. lasi.   How does that sound?

> I also just realized because led_get* routines are static inline,
> led_lanrxtx and led_diskio tests can be moved to the respective
> routines as well with no harm to performance. May be a bad idea
> though since current construct is pretty obvious what they do.
>
> cheers,
> grant

Cheers,

David

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[David Pye]

[-- Attachment #1.1.1: Type: text/plain, Size: 511 bytes --]

Right, I've actually found and fixed what was the cause of the missing 
heartbeats.  Although led minimum ontime 'fixes' it, it's actually a bandaid 
for the real problem.

Could I ask you to have a glance at this patch? It's based on your previous 
one, and only has a very minor change to the way count_HZ is incremented - ie 
increment by the actual jiffies past, not the number we asked to be 
rescheduled in.

On my system, the heart now beats regularly under IO/LAN traffic!

Cheers,

David

[-- Attachment #1.1.2: led_lcd_patch5 --]
[-- Type: text/x-diff, Size: 12652 bytes --]

diff -urpN oldtree/arch/parisc/kernel/time.c newtree/arch/parisc/kernel/time.c
--- oldtree/arch/parisc/kernel/time.c	2004-10-21 19:58:24.000000000 +0100
+++ newtree/arch/parisc/kernel/time.c	2005-03-27 18:30:29.000000000 +0100
@@ -89,14 +89,6 @@ irqreturn_t timer_interrupt(int irq, voi
 		}
 	}
     
-#ifdef CONFIG_CHASSIS_LCD_LED
-	/* Only schedule the led tasklet on cpu 0, and only if it
-	 * is enabled.
-	 */
-	if (cpu == 0 && !atomic_read(&led_tasklet.count))
-		tasklet_schedule(&led_tasklet);
-#endif
-
 	/* check soft power switch status */
 	if (cpu == 0 && !atomic_read(&power_tasklet.count))
 		tasklet_schedule(&power_tasklet);
diff -urpN oldtree/drivers/parisc/led.c newtree/drivers/parisc/led.c
--- oldtree/drivers/parisc/led.c	2005-03-18 13:17:10.000000000 +0000
+++ newtree/drivers/parisc/led.c	2005-03-27 18:31:33.000000000 +0100
@@ -18,6 +18,9 @@
  * Changes:
  *      - Audit copy_from_user in led_proc_write.
  *                                Daniele Bellucci <bellucda@tiscali.it>
+ *	- Switch from using a tasklet to a work queue, so the led_LCD_driver
+ *	  	can sleep.
+ *	  			  David Pye <dmp@davidmpye.dyndns.org>
  */
 
 #include <linux/config.h>
@@ -37,6 +40,7 @@
 #include <linux/proc_fs.h>
 #include <linux/ctype.h>
 #include <linux/blkdev.h>
+#include <linux/workqueue.h>
 #include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/hardware.h>
@@ -46,25 +50,30 @@
 #include <asm/uaccess.h>
 
 /* The control of the LEDs and LCDs on PARISC-machines have to be done 
-   completely in software. The necessary calculations are done in a tasklet
-   which is scheduled at every timer interrupt and since the calculations 
-   may consume relatively much CPU-time some of the calculations can be 
+   completely in software. The necessary calculations are done in a work queue
+   task which is scheduled regularly, and since the calculations may consume a 
+   relatively large amount of CPU time, some of the calculations can be 
    turned off with the following variables (controlled via procfs) */
 
 static int led_type = -1;
-static int led_heartbeat = 1;
-static int led_diskio = 1;
-static int led_lanrxtx = 1;
+static unsigned char lastleds;	/* LED state from most recent update */
+static unsigned int led_heartbeat = 1;
+static unsigned int led_diskio = 1;
+static unsigned int led_lanrxtx = 1;
 static char lcd_text[32];
 static char lcd_text_default[32];
 
+
+static struct workqueue_struct *led_wq;
+static void led_work_func(void *);
+static DECLARE_WORK(led_task, led_work_func, NULL);
+
 #if 0
 #define DPRINTK(x)	printk x
 #else
 #define DPRINTK(x)
 #endif
 
-
 struct lcd_block {
 	unsigned char command;	/* stores the command byte      */
 	unsigned char on;	/* value for turning LED on     */
@@ -115,12 +124,24 @@ lcd_info __attribute__((aligned(8))) =
 #define LCD_DATA_REG	lcd_info.lcd_data_reg_addr	 
 #define LED_DATA_REG	lcd_info.lcd_cmd_reg_addr	/* LASI & ASP only */
 
+#define LED_HASLCD 1
+#define LED_NOLCD  0
+
+/* The workqueue must be created at init-time */
+static int start_task(void) 
+{	
+	/* Create the work queue and queue the LED task */
+	led_wq = create_singlethread_workqueue("led_wq");	
+	queue_work(led_wq, &led_task);
+
+	return 0;
+}
+
+device_initcall(start_task);
 
 /* ptr to LCD/LED-specific function */
 static void (*led_func_ptr) (unsigned char);
 
-#define LED_HASLCD 1
-#define LED_NOLCD  0
 #ifdef CONFIG_PROC_FS
 static int led_proc_read(char *page, char **start, off_t off, int count, 
 	int *eof, void *data)
@@ -285,52 +306,35 @@ static void led_LASI_driver(unsigned cha
 /*
    ** 
    ** led_LCD_driver()
-   ** 
-   ** The logic of the LCD driver is, that we write at every scheduled call
-   ** only to one of LCD_CMD_REG _or_ LCD_DATA_REG - registers.
-   ** That way we don't need to let this tasklet busywait for min_cmd_delay
-   ** milliseconds.
-   **
-   ** TODO: check the value of "min_cmd_delay" against the value of HZ.
    **   
  */
 static void led_LCD_driver(unsigned char leds)
 {
-	static int last_index;	/* 0:heartbeat, 1:disk, 2:lan_in, 3:lan_out */
-	static int last_was_cmd;/* 0: CMD was written last, 1: DATA was last */
-	struct lcd_block *block_ptr;
-	int value;
-
-	switch (last_index) {
-	    case 0:	block_ptr = &lcd_info.heartbeat;
-			value = leds & LED_HEARTBEAT;
-			break;
-	    case 1:	block_ptr = &lcd_info.disk_io;
-			value = leds & LED_DISK_IO;
-			break;					
-	    case 2:	block_ptr = &lcd_info.lan_rcv;
-			value = leds & LED_LAN_RCV;
-			break;					
-	    case 3:	block_ptr = &lcd_info.lan_tx;
-			value = leds & LED_LAN_TX;
-			break;
-	    default:	/* should never happen: */
-			return;
-	}
+	static int i;
+	static unsigned char mask[4] = { LED_HEARTBEAT, LED_DISK_IO,
+		LED_LAN_RCV, LED_LAN_TX };
+	
+	static struct lcd_block * blockp[4] = {
+		&lcd_info.heartbeat,
+		&lcd_info.disk_io,
+		&lcd_info.lan_rcv,
+		&lcd_info.lan_tx
+	};
 
-	if (last_was_cmd) {
-	    /* write the value to the LCD data port */
-    	    gsc_writeb( value ? block_ptr->on : block_ptr->off, LCD_DATA_REG );
-	} else {
-	    /* write the command-byte to the LCD command register */
-    	    gsc_writeb( block_ptr->command, LCD_CMD_REG );
-	}    
-	
-	/* now update the vars for the next interrupt iteration */ 
-	if (++last_was_cmd == 2) { /* switch between cmd & data */
-	    last_was_cmd = 0;
-	    if (++last_index == 4) 
-		last_index = 0;	 /* switch back to heartbeat index */
+	/* Convert min_cmd_delay to milliseconds */
+	unsigned int msec_cmd_delay = 1 + (lcd_info.min_cmd_delay / 1000);
+	
+	for (i=0; i<4; ++i) 
+	{
+		if ((leds & mask[i]) != (lastleds & mask[i])) 
+		{
+			gsc_writeb( blockp[i]->command, LCD_CMD_REG );
+			msleep(msec_cmd_delay);
+			
+			gsc_writeb( leds & mask[i] ? blockp[i]->on : 
+					blockp[i]->off, LCD_DATA_REG );
+			msleep(msec_cmd_delay);
+		}
 	}
 }
 
@@ -355,7 +359,7 @@ static __inline__ int led_get_net_activi
 
 	rx_total = tx_total = 0;
 	
-	/* we are running as tasklet, so locking dev_base 
+	/* we are running as a workqueue task, so locking dev_base 
 	 * for reading should be OK */
 	read_lock(&dev_base_lock);
 	for (dev = dev_base; dev; dev = dev->next) {
@@ -402,7 +406,7 @@ static __inline__ int led_get_diskio_act
 	static unsigned long last_pgpgin, last_pgpgout;
 	struct page_state pgstat;
 	int changed;
-	
+
 	get_full_page_state(&pgstat); /* get no of sectors in & out */
 
 	/* Just use a very simple calculation here. Do not care about overflow,
@@ -410,86 +414,70 @@ static __inline__ int led_get_diskio_act
 	changed = (pgstat.pgpgin != last_pgpgin) || (pgstat.pgpgout != last_pgpgout);
 	last_pgpgin  = pgstat.pgpgin;
 	last_pgpgout = pgstat.pgpgout;
-	
+
 	return (changed ? LED_DISK_IO : 0);
 }
 
 
 
 /*
-   ** led_tasklet_func()
+   ** led_work_func()
    ** 
-   ** is scheduled at every timer interrupt from time.c and
-   ** updates the chassis LCD/LED 
+   ** manages when and which chassis LCD/LED gets updated
 
     TODO:
     - display load average (older machines like 715/64 have 4 "free" LED's for that)
     - optimizations
  */
 
-#define HEARTBEAT_LEN (HZ*6/100)
-#define HEARTBEAT_2ND_RANGE_START (HZ*22/100)
+#define HEARTBEAT_LEN (HZ*10/100)
+#define HEARTBEAT_2ND_RANGE_START (HZ*28/100)
 #define HEARTBEAT_2ND_RANGE_END   (HEARTBEAT_2ND_RANGE_START + HEARTBEAT_LEN)
 
-#define NORMALIZED_COUNT(count) (count/(HZ/100))
+#define LED_UPDATE_INTERVAL (1 + (HZ*19/1000))
 
-static void led_tasklet_func(unsigned long unused)
+static void led_work_func (void *unused)
 {
-	static unsigned char lastleds;
-	unsigned char currentleds; /* stores current value of the LEDs */
-	static unsigned long count; /* static incremented value, not wrapped */
+	static unsigned long last_jiffies = 0;
 	static unsigned long count_HZ; /* counter in range 0..HZ */
+	unsigned char currentleds = 0; /* stores current value of the LEDs */
 
 	/* exit if not initialized */
 	if (!led_func_ptr)
 	    return;
 
-	/* increment the local counters */
-	++count;
-	if (++count_HZ == HZ)
+	/* increment the local counter */
+	count_HZ += jiffies - last_jiffies;
+	if (count_HZ >= HZ)
 	    count_HZ = 0;
-
-	currentleds = lastleds;
+	last_jiffies = jiffies;
 
 	if (led_heartbeat)
 	{
-		/* flash heartbeat-LED like a real heart (2 x short then a long delay) */
-		if (count_HZ<HEARTBEAT_LEN || 
-		    (count_HZ>=HEARTBEAT_2ND_RANGE_START && count_HZ<HEARTBEAT_2ND_RANGE_END)) 
-		    currentleds |= LED_HEARTBEAT;
-		else
-		    currentleds &= ~LED_HEARTBEAT;
-	}
-
-	/* look for network activity and flash LEDs respectively */
-	if (led_lanrxtx && ((NORMALIZED_COUNT(count)+(8/2)) & 7) == 0)
-	{
-		currentleds &= ~(LED_LAN_RCV | LED_LAN_TX);
-		currentleds |= led_get_net_activity();
+		/* flash heartbeat-LED like a real heart
+		 * (2 x short then a long delay)
+		 */
+		if (count_HZ < HEARTBEAT_LEN || 
+				(count_HZ >= HEARTBEAT_2ND_RANGE_START &&
+				count_HZ < HEARTBEAT_2ND_RANGE_END)) 
+			currentleds |= LED_HEARTBEAT;
 	}
 
-	/* avoid to calculate diskio-stats at same irq  as netio-stats */
-	if (led_diskio && (NORMALIZED_COUNT(count) & 7) == 0)
-	{
-		currentleds &= ~LED_DISK_IO;
-		currentleds |= led_get_diskio_activity();
-	}
+	if (led_lanrxtx)  currentleds |= led_get_net_activity();
+	if (led_diskio)   currentleds |= led_get_diskio_activity();
 
 	/* blink all LEDs twice a second if we got an Oops (HPMC) */
-	if (oops_in_progress) {
+	if (oops_in_progress) 
 		currentleds = (count_HZ<=(HZ/2)) ? 0 : 0xff;
-	}
-	
-	/* update the LCD/LEDs */
-	if (currentleds != lastleds) {
-	    led_func_ptr(currentleds);
-	    lastleds = currentleds;
-	}
-}
 
-/* main led tasklet struct (scheduled from time.c) */
-DECLARE_TASKLET_DISABLED(led_tasklet, led_tasklet_func, 0);
+	if (currentleds != lastleds)
+	{
+		led_func_ptr(currentleds);	/* Update the LCD/LEDs */
+		lastleds = currentleds;
+	}
 
+	queue_delayed_work(led_wq, &led_task, LED_UPDATE_INTERVAL);
+}
 
 /*
    ** led_halt()
@@ -519,9 +507,13 @@ static int led_halt(struct notifier_bloc
 	default:		return NOTIFY_DONE;
 	}
 	
-	/* completely stop the LED/LCD tasklet */
-	tasklet_disable(&led_tasklet);
-
+	/* Cancel the work item and delete the queue */
+	if (led_wq) {
+		if (cancel_delayed_work(&led_task) == 0) 
+			flush_workqueue(led_wq);
+		destroy_workqueue(led_wq);
+	}
+ 
 	if (lcd_info.model == DISPLAY_MODEL_LCD)
 		lcd_print(txt);
 	else
@@ -586,9 +578,6 @@ int __init register_led_driver(int model
 	initialized++;
 	register_reboot_notifier(&led_notifier);
 
-	/* start the led tasklet for the first time */
-	tasklet_enable(&led_tasklet);
-	
 	return 0;
 }
 
@@ -623,7 +612,7 @@ void __init register_led_regions(void)
    ** lcd_print()
    ** 
    ** Displays the given string on the LCD-Display of newer machines.
-   ** lcd_print() disables the timer-based led tasklet during its 
+   ** lcd_print() disables the timer-based led work task during its 
    ** execution and enables it afterwards again.
    **
  */
@@ -634,12 +623,14 @@ int lcd_print( char *str )
 	if (!led_func_ptr || lcd_info.model != DISPLAY_MODEL_LCD)
 	    return 0;
 	
-	/* temporarily disable the led tasklet */
-	tasklet_disable(&led_tasklet);
-
+	/* temporarily disable the led work task */
+	if (led_wq) {
+		if (cancel_delayed_work(&led_task) == 0) 
+			flush_workqueue(led_wq);
+	}
 	/* copy display string to buffer for procfs */
 	strlcpy(lcd_text, str, sizeof(lcd_text));
-	
+
 	/* Set LCD Cursor to 1st character */
 	gsc_writeb(lcd_info.reset_cmd1, LCD_CMD_REG);
 	udelay(lcd_info.min_cmd_delay);
@@ -653,8 +644,10 @@ int lcd_print( char *str )
 	    udelay(lcd_info.min_cmd_delay);
 	}
 	
-	/* re-enable the led tasklet */
-	tasklet_enable(&led_tasklet);
+	/* re-queue the work */
+	if (led_wq) {
+		queue_work(led_wq, &led_task);
+	}
 
 	return lcd_info.lcd_width;
 }
diff -urpN oldtree/include/asm-parisc/led.h newtree/include/asm-parisc/led.h
--- oldtree/include/asm-parisc/led.h	2004-12-30 08:07:48.000000000 +0000
+++ newtree/include/asm-parisc/led.h	2005-03-27 18:30:29.000000000 +0100
@@ -23,9 +23,6 @@
 
 #define LED_CMD_REG_NONE 0		/* NULL == no addr for the cmd register */
 
-/* led tasklet struct */
-extern struct tasklet_struct led_tasklet;
-
 /* register_led_driver() */
 int __init register_led_driver(int model, unsigned long cmd_reg, unsigned long data_reg);
 

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[Grant Grundler]

On Sun, Mar 27, 2005 at 05:55:53PM +0000, David Pye wrote:
> Could I ask you to have a glance at this patch? It's based on your previous 
> one, and only has a very minor change to the way count_HZ is incremented - ie 
> increment by the actual jiffies past, not the number we asked to be 
> rescheduled in.

Funny you point that out. I was thinking about it yesterday before I posted
the patch but was "too lazy" to figure out how to do it.

(TBH, I had several other things going on).

> On my system, the heart now beats regularly under IO/LAN traffic!

Cool!

> -	/* increment the local counters */
> -	++count;
> -	if (++count_HZ == HZ)
> +	/* increment the local counter */
> +	count_HZ += jiffies - last_jiffies;
> +	if (count_HZ >= HZ)
>  	    count_HZ = 0;
> -
> -	currentleds = lastleds;
> +	last_jiffies = jiffies;

Yeah, I'll add this bit, try it out and commit.

thanks,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] J5000 LED LCD patch (take 2)

[Grant Grundler]

On Sun, Mar 27, 2005 at 09:38:38AM +0000, David Pye wrote:
> > I think I would be ok with it implemented inside the led_get*
> > activity routines. They have access to lastleds and jiffies and
> > that should be enough to figure out how long an LED should stay
> > on if it's already on.  The main routine can stay simple/readable.
> 
> Yep, I hadn't really thought of doing it in there - I'll have a play with it 
> and maybe annoy everyone with another patch in a bit.   Downside is that it's 
> not really the right place to put it *if* you want it to be lcd-only.

I don't really care if it's LCD only.
LCD clearly has different response times than LED.

> The only other way to do it which you might not find as ugly is to move this 
> into the drivers.  ie the drivers would need to be given the responsibility 
> of deciding whether they need to update anything (ie each of them has to 
> store currentleds rather than the generic calculator function). That way, 
> they'll get called each time the work runs, but can decide for themselves 
> whether to actually do anything.  It's only a couple of lines to add to the 
> other functions e.g. lasi.   How does that sound?

Yeah - having the LCD_driver regulate minimum on probably makes just as
much sense since LED really doesn't need it.

Implementing it in led_get* routines was just my first thought.
led_get* controls state of the LEDs and could avoid extra calculations
if it knows about minimum-on time. 
Maybe minimum-on time should be a parameter that's different for LED/LCD
if a hardcoded value isn't acceptable for both.

thanks,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux