Can someone help me replace "lock_kernel" on a block device driver?

Question

Thank you for looking at this post. I am trying to patch up a network block device driver. If you need to see the sources they are at http : / / code.ximeta.com.

I noticed that lock_kernel() seems deprecated as of linux 2.6.37. I read "The new way of ioctl()" and found that device drivers now should perform a specific lock before operating.

So I would like some advice replacing this if possible.

I have found two sections in the current code that I think are related, in the block folder section.

Source 
      block->io.c
           ->ctrldev.c

I put snippets from each for your consideration.

io.c contains one call to lock_kernel:

NDAS_SAL_API xbool     sal_file_get_size(sal_file file, xuint64* size)
{
    definitions and declarations etc..

lock_kernel();

#ifdef HAVE_UNLOCKED_IOCTL
    if (filp->f_op->unlocked_ioctl) {   
       some small statements

       error = filp->f_op->unlocked_ioctl(filp, BLKGETSIZE64, (unsigned long)size);

       actions if error or not etc.
   }
#endif

   unlock_kernel(); 
   return ret;
}

And ctrldev.c contains the main io function:

#include <linux/spinlock.h> // spinklock_t
#include <linux/semaphore.h> // struct semaphore
#include <asm/atomic.h> // atomic
#include <linux/interrupt.h>
#include <linux/fs.h>
#include <asm/uaccess.h>
#include <linux/ide.h>
#include <linux/smp_lock.h>
#include <linux/time.h>

......

int ndas_ctrldev_ioctl(struct inode *inode, struct file *filp, unsigned int cmd, unsigned long arg)
{
  lots of operations and functions. 

  return result;
}

Later ndas_ctrldev_ioctl function is set as the former .ioctl.

static struct file_operations ndasctrl_fops = {
    .write = ndas_ctrldev_write,
    .read = ndas_ctrldev_read,
    .open = ndas_ctrldev_open,
    .release = ndas_ctrldev_release,
    .ioctl = ndas_ctrldev_ioctl, 
};

Now I want to convert this to avoid using lock_kernel();

According to my understanding I will modified the former sections as below:

NDAS_SAL_API xbool     sal_file_get_size(sal_file file, xuint64* size)
{
    definitions and declarations etc..

#ifndef HAVE_UNLOCKED_IOCTL
    lock_kernel();
#endif

#ifdef HAVE_UNLOCKED_IOCTL
    if (filp->f_op->unlocked_ioctl) {   
       some small statements

       error = filp->f_op->unlocked_ioctl(filp, BLKGETSIZE64, (unsigned long)size);

       actions if error or not etc.
   }
#endif

#ifndef HAVE_UNLOCKED_IOCTL
   unlock_kernel(); 
#endif
   return ret;

}

#ifdef HAVE_UNLOCKED_IOCTL
long ndas_ctrldev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
#else
int ndas_ctrldev_ioctl(struct inode *inode, struct file *filp, unsigned int cmd, unsigned long arg)
#endif
{

#ifdef HAVE_UNLOCKED_IOCTL
  ! add some sort of lock here !
#endif

  lots of operations and functions. 

#ifdef HAVE_UNLOCKED_IOCTL
  ! add unlock statement here  !
#endif
  return result;
}

static struct file_operations ndasctrl_fops = {
    .write = ndas_ctrldev_write,
    .read = ndas_ctrldev_read,
    .open = ndas_ctrldev_open,
    .release = ndas_ctrldev_release,
#ifdef HAVE_UNLOCKED_IOCTL
    .unlocked_ioctl = ndas_ctrldev_ioctl, 
#else
    .ioctl = ndas_ctrldev_ioctl, 
#endif
};

So, I would ask the following advice.

1. Does this look like the right proceedure?

2. Do I understand correct to move the lock into the io function?

3. Based on the includes in crtrldev.c, can you recommend any lock off the top of your head? (I tried to research some other drivers dealing with filp and lock_kernel, but I am too much a noob to find the answer right away.)

Answer 1

The Big Kernel Lock (BKL) is more than deprecated - as of 2.6.39, it does not exist anymore.

The way the lock_kernel() conversion was done was to replace it by per-driver mutexes. If the driver is simple enough, you can simply create a mutex for the driver, and replace all uses of lock_kernel() and unlock_kernel() by the mutex lock/unlock calls. Note, however, that some functions used to be called with the BKL (the lock lock_kernel() used to lock) held; you will have to add lock/unlock calls to these functions too.

This will not work if the driver could acquire the BKL recursively; if that is the case, you would have to track it yourself to avoid deadlocks (this was done in the conversion of reiserfs, which depended somewhat heavily both in the recursive BKL behavior and in the fact that it was dropped when sleeping).

The next step after the conversion to a per-driver mutex would be to change it to use a per-device mutex instead of a per-driver mutex.