Re: [RFC/RFT] Convert i8042 input event handling to irq + tasklet

[Vojtech Pavlik]

On Thu, May 06, 2004 at 01:50:52AM -0500, Dmitry Torokhov wrote:

> Entire input event handling in i8042 is done from interrupt context. Given
> that there is mousedev emulation, evdev and what's not it seems way too much
> for my taste. The patch below moves most of the processing into a tasklet
> leaving only retrieving data from the KBC to interrup handler. What do you
> think?

Looks reasonable. The FIFO is a good thing, because before we
theoretically could deliver characters in the wrong order on multi-cpu
machines.

> --- linus-2.5/drivers/input/serio/i8042.c	2004-05-06 00:54:57.000000000 -0500
> +++ dtor/drivers/input/serio/i8042.c	2004-05-06 00:35:32.000000000 -0500
> @@ -74,6 +74,14 @@
>  	unsigned char *phys;
>  };
>  
> +#define I8042_QUEUE_LEN		64
> +struct {
> +	unsigned char str[I8042_QUEUE_LEN];
> +	unsigned char data[I8042_QUEUE_LEN];
> +	unsigned int read_pos;
> +	unsigned int write_pos;
> +} i8042_buf;
> +
>  static struct serio i8042_kbd_port;
>  static struct serio i8042_aux_port;
>  static unsigned char i8042_initial_ctr;
> @@ -82,7 +90,7 @@
>  static unsigned char i8042_mux_present;
>  static unsigned char i8042_sysdev_initialized;
>  static struct pm_dev *i8042_pm_dev;
> -struct timer_list i8042_timer;
> +static struct timer_list i8042_timer;
>  
>  /*
>   * Shared IRQ's require a device pointer, but this driver doesn't support
> @@ -374,77 +382,109 @@
>  static char i8042_mux_phys[4][32];
>  
>  /*
> - * i8042_interrupt() is the most important function in this driver -
> - * it handles the interrupts from the i8042, and sends incoming bytes
> - * to the upper layers.
> + * i8042_handle_data() is the most important function in this driver -
> + * it processes data received by i8042_interrupt and sends it to the
> + * upper layers.
>   */
>  
> -static irqreturn_t i8042_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> +static void i8042_handle_data(unsigned long notused)
>  {
> -	unsigned long flags;
>  	unsigned char str, data = 0;
>  	unsigned int dfl;
> -	int ret;
>  
> -	mod_timer(&i8042_timer, jiffies + I8042_POLL_PERIOD);
> +	/*
> +	 * No locking it required on i8042_buf as the tasklet is guaranteed
> +	 * to be serialized and if write_pos changes while comparing it with
> +	 * read_pos another run will be scheduled by i8042_interrupt.
> +	 */
> +	while (i8042_buf.read_pos != i8042_buf.write_pos) {
>  
> -	spin_lock_irqsave(&i8042_lock, flags);
> -	str = i8042_read_status();
> -	if (str & I8042_STR_OBF)
> -		data = i8042_read_data();
> -	spin_unlock_irqrestore(&i8042_lock, flags);
> +		str = i8042_buf.str[i8042_buf.read_pos];
> +		data = i8042_buf.data[i8042_buf.read_pos];
>  
> -	if (~str & I8042_STR_OBF) {
> -		if (irq) dbg("Interrupt %d, without any data", irq);
> -		ret = 0;
> -		goto out;
> +		i8042_buf.read_pos++;
> +		i8042_buf.read_pos %= I8042_QUEUE_LEN;
> +
> +		dfl = ((str & I8042_STR_PARITY)  ? SERIO_PARITY  : 0) |
> +	      	      ((str & I8042_STR_TIMEOUT) ? SERIO_TIMEOUT : 0);
> +
> +		if (i8042_mux_values[0].exists && (str & I8042_STR_AUXDATA)) {
> +
> +			if (str & I8042_STR_MUXERR) {
> +				switch (data) {
> +					case 0xfd:
> +					case 0xfe: dfl = SERIO_TIMEOUT; break;
> +					case 0xff: dfl = SERIO_PARITY; break;
> +				}
> +				data = 0xfe;
> +			} else dfl = 0;
> +
> +			dbg("%02x <- i8042 (interrupt, aux%d, %d%s%s)",
> +				data, (str >> 6), irq,
> +				dfl & SERIO_PARITY ? ", bad parity" : "",
> +				dfl & SERIO_TIMEOUT ? ", timeout" : "");
> +
> +			serio_interrupt(i8042_mux_port + ((str >> 6) & 3), data, dfl, NULL);
> +		} else {
> +
> +			dbg("%02x <- i8042 (interrupt, %s, %d%s%s)",
> +				data, (str & I8042_STR_AUXDATA) ? "aux" : "kbd", irq,
> +				dfl & SERIO_PARITY ? ", bad parity" : "",
> +				dfl & SERIO_TIMEOUT ? ", timeout" : "");
> +
> +			if (i8042_aux_values.exists && (str & I8042_STR_AUXDATA))
> +				serio_interrupt(&i8042_aux_port, data, dfl, NULL);
> +			else if (i8042_kbd_values.exists)
> +				serio_interrupt(&i8042_kbd_port, data, dfl, NULL);
> +		}
>  	}
> +}
>  
> -	dfl = ((str & I8042_STR_PARITY) ? SERIO_PARITY : 0) |
> -	      ((str & I8042_STR_TIMEOUT) ? SERIO_TIMEOUT : 0);
> +DECLARE_TASKLET(i8042_tasklet, i8042_handle_data, 0);
>  
> -	if (i8042_mux_values[0].exists && (str & I8042_STR_AUXDATA)) {
> +/*
> + * i8042_interrupt() handles the interrupts from i8042 and schedules
> + * i8042_handle_data to process and pass received bytes to the upper
> + * layers.
> + */
>  
> -		if (str & I8042_STR_MUXERR) {
> -			switch (data) {
> -				case 0xfd:
> -				case 0xfe: dfl = SERIO_TIMEOUT; break;
> -				case 0xff: dfl = SERIO_PARITY; break;
> -			}
> -			data = 0xfe;
> -		} else dfl = 0;
> +static irqreturn_t i8042_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> +{
> +	unsigned long flags;
> +	unsigned char str;
> +	unsigned int n_bytes = 0;
>  
> -		dbg("%02x <- i8042 (interrupt, aux%d, %d%s%s)",
> -			data, (str >> 6), irq, 
> -			dfl & SERIO_PARITY ? ", bad parity" : "",
> -			dfl & SERIO_TIMEOUT ? ", timeout" : "");
> +	mod_timer(&i8042_timer, jiffies + I8042_POLL_PERIOD);
>  
> -		serio_interrupt(i8042_mux_port + ((str >> 6) & 3), data, dfl, regs);
> -		
> -		goto irq_ret;
> -	}
> +	spin_lock_irqsave(&i8042_lock, flags);
> +
> +	while ((str = i8042_read_status()) & I8042_STR_OBF) {
>  
> -	dbg("%02x <- i8042 (interrupt, %s, %d%s%s)",
> -		data, (str & I8042_STR_AUXDATA) ? "aux" : "kbd", irq, 
> -		dfl & SERIO_PARITY ? ", bad parity" : "",
> -		dfl & SERIO_TIMEOUT ? ", timeout" : "");
> +		n_bytes++;
>  
> -	if (i8042_aux_values.exists && (str & I8042_STR_AUXDATA)) {
> -		serio_interrupt(&i8042_aux_port, data, dfl, regs);
> -		goto irq_ret;
> +		i8042_buf.str[i8042_buf.write_pos] = str;
> +		i8042_buf.data[i8042_buf.write_pos] = i8042_read_data();
> +
> +		i8042_buf.write_pos++;
> +		i8042_buf.write_pos %= I8042_QUEUE_LEN;
> +
> +		if (unlikely(i8042_buf.write_pos == i8042_buf.read_pos))
> +			printk(KERN_WARNING "i8042.c: ring buffer full\n");
>  	}
>  
> -	if (!i8042_kbd_values.exists)
> -		goto irq_ret;
> +	spin_unlock_irqrestore(&i8042_lock, flags);
> +
> +	if (unlikely(n_bytes == 0)) {
> +		if (irq) dbg("Interrupt %d, without any data", irq);
> +		return IRQ_NONE;
> +	}
>  
> -	serio_interrupt(&i8042_kbd_port, data, dfl, regs);
> +	tasklet_schedule(&i8042_tasklet);
>  
> -irq_ret:
> -	ret = 1;
> -out:
> -	return IRQ_RETVAL(ret);
> +	return IRQ_HANDLED;
>  }
>  
> +
>  /*
>   * i8042_enable_mux_mode checks whether the controller has an active
>   * multiplexor and puts the chip into Multiplexed (as opposed to
> @@ -1011,6 +1053,8 @@
>  		if (i8042_mux_values[i].exists)
>  			serio_unregister_port(i8042_mux_port + i);
>  
> +	tasklet_kill(&i8042_tasklet);
> +
>  	i8042_platform_exit();
>  }
>  
> 
> -- 
> Dmitry
> 

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/