Simple Linux char driver

Question

Since the resources I found to learn are generally out-of-date, I'm having to read a lot of documentation, which makes the learning process somewhat haphazard. The module makes a simple character stack which can be read and written to by multiple processes.

#include<linux/init.h>
#include<linux/module.h>

#include<linux/fs.h>
#include<linux/types.h>
#include<linux/kdev_t.h>
#include<linux/cdev.h>

#include<linux/mutex.h>

#include<asm/uaccess.h>


#define MHELLO_STACK_SIZE 20

static int major;
static dev_t dev;
static struct cdev my_cdev;
static int count;
static DEFINE_MUTEX(lock);
static char stack[MHELLO_STACK_SIZE];
static int head;


static ssize_t hello_read(struct file* f,char* buf,size_t count,loff_t * offset)
{
    unsigned int res;
    printk(KERN_ALERT"R\n");

    mutex_lock(&lock);

    if(head<0)
    {
        mutex_unlock(&lock);
        return 0;
    }
    res=copy_to_user(buf,stack+head,1);
    head--;

    mutex_unlock(&lock);
    return 1;
}
static ssize_t hello_write(struct file* f,const char* buf,size_t count,loff_t * offset)
{
    unsigned int res;
    printk(KERN_ALERT"W\n");

    mutex_lock(&lock);

    if(head >= MHELLO_STACK_SIZE-1)
    {
        mutex_unlock(&lock);
        return -ENOSPC;
    }
    head++;
    res=copy_from_user(stack+head,buf,1);

    mutex_unlock(&lock);
    return 1;
}

static int hello_open(struct inode* inode,struct file* f)
{
    mutex_lock(&lock);
    count++;
    printk(KERN_ALERT"O %d\n",count);
    mutex_unlock(&lock);
    return 0;
}
static int hello_release(struct inode* inode,struct file* f)
{
    mutex_lock(&lock);
    count--;
    printk(KERN_ALERT"C %d\n",count);
    mutex_unlock(&lock);
    return 0;
}
const struct file_operations fops=
{
    .owner=THIS_MODULE,
    .read=hello_read,
    .write=hello_write,
    .open=hello_open,
    .release=hello_release
};

static int hello_init(void)
{
    int result=-1;

    mutex_init(&lock);
    mutex_lock(&lock);

    count=0;
    head=-1;

    result=alloc_chrdev_region(&dev,0,1,"mhello");

    if(result<0)
        printk(KERN_ALERT"Can't allocate major.\n");

    major=MAJOR(dev);
    printk(KERN_ALERT"Hello, my major no. is %d.\n",major);


    cdev_init(&my_cdev,&fops);
    my_cdev.owner=THIS_MODULE;


    cdev_add(&my_cdev,dev,1);

    mutex_unlock(&lock);
    return 0;
}
static void hello_exit(void)
{
    mutex_lock(&lock);
    unregister_chrdev_region(dev,1);
    cdev_del(&my_cdev);
    mutex_unlock(&lock);
    printk(KERN_ALERT"Goodbye world\n");
}
module_init(hello_init);
module_exit(hello_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("foo");

Answer 1

So, this isn't a full answer to your question, but maybe still relevant.

Included with the Linux sources is scripts/checkpatch.pl - which checks for things like coding style compliance, but also warns on use of outdated interfaces.

If you have a checked out (clean) git repository of the kernel, you can simply copy your source file into there and run

linux$ git diff HEAD | scripts/checkpatch.pl -

Doing that on the above code does point out a few things not just coding-style related.

Answer 2

My comments below are general C, not specific to Linux kernel development (of which I know nothing).

• When handling resources like mutexes, I usually think it better to release the resource in only one place. So I would modify your read function to the following:

  static ssize_t hello_read(struct file* f, char* buf, size_t count, loff_t * offset)
  {
      ssize_t res = 0;
      mutex_lock(&lock);
      if (head >= 0) {
          copy_to_user(buf, stack + head, 1);
          head--;
          res = 1;
      }
      mutex_unlock(&lock);
      return res;
  }
  

  As you can see, I have adjusted the condition and done the copy within the conditional block. Note that the return from copy_to_user should be checked (number of bytes not copied) and presumably the function should use count and offset, in which case these need to be checked before use.

  The same comments apply to the hello_write function.

• hello_init continues initialising even after an error in the alloc_chrdev_region call. It also doesn't check the return from cdev_init and cdev_add.

• Outside of kernel development, it is normal to put spaces after commas, around '=', and (for me) after keywords (if, else, for, while etc). Do kernel developers do things differently?