Linux Kernel & Device Driver Programming

Anti-Patterns: What's wrong with this code?

This file uses the W3C HTML Slidy format. The "a" key toggles between one-slide-at-a-time and single-page mode, and the "c" key toggles on and off the table of contents. The ← and → keys can be used to page forward and backward. For more help on controls see the "help?" link at the bottom.

 *

Image from http://www.gerfm.com/funny.htm.

Ethics

Do you consider the potential consequences of errors in code you write?

What can go wrong

if the following code is called concurrently by different threads?

void start_timer() {
   init_timer(&timer);
   timer.function = timer_handler;
   timer.expires = jiffies + HZ;
   add_timer(&timer);
}

How can this problem be corrected?

What is wrong with the following "solution"?

int timer_active = 0;
void start_timer() {
   if (!timer_active) {
      timer_active = 1;
      init_timer(&timer);
      timer.function = timer_handler;
      timer.expires = jiffies + HZ;
      add_timer(&timer);
}}

How can this problem be corrected?

What is likely to go wrong

if the following code is called concurrently by different threads to control a device?

outb(x, device_base); outb(y, device_base +1);

How can this problem be corrected?

What can go wrong?

register_chrdev(major, "mydevice", &myfops);
buffer = kmalloc(sizeof(* buffer), GFP_KERNEL);

Hint: There are at least three (3) problems!

What can go wrong?

kfree(buffer);
unregister_chrdev(major, "mydevice");

What is likely to go wrong

if this device is opened concurrently my multiple processes?

int mydev_open(struct inode *inode; struct file *filp) {
   if (mutex_lock_interruptible(&mydev->lock)) return -ERESTARTSYS;
   mydev->state = INITIAL_STATE;
   mutex_unlock(&mdev->lock);
}
int mydev_release(struct inode *inode; struct file *filp) {
   if (mutex_lock_interruptible(&mydev->lock)) return -ERESTARTSYS;
   mydev->state = FINAL_STATE;
   mutex_unlock(&mdev->lock);
}

What can go wrong?

int mydev_init (void) {

register_chrdev(major, "mydev", &myfops);
   server_task = kthread_create(mythread_function, NULL, "mydevserver");
   if (server_task == ERR_PTR(-ENOMEM)) return -ENOMEM;
   wake_up_process(server_task);
}

What's wrong with this?

void timer_handler () {
    ...
   mdelay (1);
    ...
}

What's wrong with this?

void timer_handler () {
    ...
   if (mutex_lock_interruptible(&mydev->lock)) return -ERESTARTSYS;
   ...
   mutex_unlock(&mdev->lock);
    ...
}

What can go wrong?

hrtimer_init(&mytimer, CLOCK_REALTIME, HRTIMER_MODE_ABS);
hrtimer_start(&mytimer, base_time, HRTIMER_MODE_ABS);
...
hrtimer_init(&mytimer, LCOKC_REALTIME, HRTIMER_MODE_ABS);

What can be improved?

void put_num(int num) {
   switch(num) {
   case -1:
     put(0); break;
   case 0:
     put (123); break;
   case 1:
     put (66); break;
   case 2:
     put (55); break;
   case 3:
     put (87); break;
   case 4:
     put (78); break;
   case 5:
     put (93); break;
   case 6:
     put (125); break;
   case 7:
     put (75); break;
   case 8:
     put (127); break;
   case 9:
     put (95); break;
   }
}

This slide has additional "handout" notes. You need to use the 'a' key to toggle into handout mode, in order to see them.

char code [] = {0, 123, 66, 55, 87, 78, 93, 125, 75, 127, 95};
...
put (code (num));