Re: [PATCH] support for Cobalt Networks (x86 only) systems (for real this

[Alan Cox]

> +		/* setup osb4 i/o regions */
> +		if ((reg = get_reg(OSB4_INDEX_PORT, OSB4_DATA_PORT, 0x20)))
> +			request_region(reg, 4, "OSB4 (pm1a_evt_blk)");

Check request_region worked

> +static int 
> +i2c_wait_for_smi(void)

Obvious question - why duplicate the i2c layer

> +/* device structure */
> +static struct miscdevice lcd_dev = {
> +	COBALT_LCD_MINOR,

Is this an officially allocated minor ?

> +	/* Get the current cursor position */
> +        case LCD_Get_Cursor_Pos:
> +                display.cursor_address = lcddev_read_inst();
> +                display.cursor_address = display.cursor_address & 0x07F;
> +                copy_to_user((struct lcd_display *)arg, &display, dlen);

		Sleeping holding a spinlock

> +        case LCD_Set_Cursor_Pos:
> +                copy_from_user(&display, (struct lcd_display *)arg, dlen);

Ditto. Also should check the return and return -EFAULT if so

> +/* LED daemon sits on this, we wake it up once a key is pressed */
> +static ssize_t 
> +cobalt_lcd_read(struct file *file, char *buf, size_t count, loff_t *ppos)
> +{
> +	int buttons_now;
> +	static atomic_t lcd_waiters = ATOMIC_INIT(0);
> +
> +	if (atomic_read(&lcd_waiters) > 0)
> +		return -EINVAL;
> +	atomic_inc(&lcd_waiters);

Unsafe. Two people can execute the atomic_read before anyone executes
the atomic_inc. You probably want test_and_set_bit() or a semaphore

> +	while (((buttons_now = button_pressed()) == 0) &&
> +	       !(signal_pending(current)))
> +	{
> +		current->state = TASK_INTERRUPTIBLE;

We have a set_ function for this.. ALso what stops an ioctl occuring at
the same instant ?