> + /* 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 ?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Fri Jun 01 2001 - 04:52:36 EDT