BKL in open functions in drivers

Hello,

Few days ago Alessio Igor Bogani<[email protected]> sent me patch that removes BKLs like lock/unlock_kernel() in open call and place mutex there in media/radio/radio-mr800.c.
This patch broke the driver, so we figured out new approah. We added one more mutex lock that was used in open call. The patch is below:

diff -r ffa5df73ebeb linux/drivers/media/radio/radio-mr800.c --- a/linux/drivers/media/radio/radio-mr800.c Fri Mar 13 00:43:34 2009 +0000
+++ b/linux/drivers/media/radio/radio-mr800.c Thu Apr 02 00:40:56 2009
+0400
@@ -163,6 +163,7 @@

unsigned char *buffer;
struct mutex lock; /* buffer locking */
+ struct mutex open;
int curfreq;
int stereo;
int users;
@@ -570,7 +571,7 @@
struct amradio_device *radio = video_get_drvdata(video_devdata(file)); int retval;

- lock_kernel();
+ mutex_lock(&radio->open);

radio->users = 1;
radio->muted = 1;
@@ -580,7 +581,7 @@
amradio_dev_warn(&radio->videodev->dev,
"radio did not start up properly\n");
radio->users = 0;
- unlock_kernel();
+ mutex_unlock(&radio->open);
return -EIO;
}

@@ -594,7 +595,7 @@
amradio_dev_warn(&radio->videodev->dev,
"set frequency failed\n");

- unlock_kernel();
+ mutex_unlock(&radio->open);
return 0;
}

@@ -735,6 +736,7 @@
radio->stereo = -1;

mutex_init(&radio->lock);
+ mutex_init(&radio->open);

video_set_drvdata(radio->videodev, radio);
retval = video_register_device(radio->videodev, VFL_TYPE_RADIO,
radio_nr);

I tested such approach using stress tool that tries to open /dev/radio0 few hundred times. Looks fine.

So, questions are:

1) What for is lock/unlock_kernel() used in open?
2) Can it be replaced by mutex, for example?

Please, comments, exaplanations are more than welcome.

Thanks,

best regards, Klimov Alexey

It's pointless. Just remove it.

No need.

But what is really wrong is the way the 'users' field is used: that should be an atomic counter: on the first-time-open you set up the device, and when the last user goes away you can close it down.

Currently if you open the device a second time and then close that second fh, the first gets muted by that close. Not what you want!

Actually, I don't see why this stuff is in the open/close at all, unless this saves some measurable amount of power consumption. I'd just move the setup code in the open() to the probe() and after that both the open() and close() functions become no-ops.

Regards,

Hans

Hans Verkuil - video4linux developer - sponsored by TANDBERG

On Thu, Apr 2, 2009 at 11:29 AM, Hans Verkuil <[email protected]> wrote:

[...]

Actually, i can see lock/unlock_kernel() in open in other V4L drivers too. What for is it used in other drivers?

Good, so we can remove it.

Agreed, thanks for explanations and suggestions.

Best regards, Klimov Alexey

If I remember correctly these lock/unlock_kernel() calls were originally handled in the part of the kernel that calls us in turn. An effort was made not too long ago to move it this out of the core kernel and into each driver, allowing each driver to replace these calls by proper mutexes, or remove it altogether if it isn't needed. I strongly suspect that in 80-90% of the cases the v4l driver does not actually need to use it and in the remaining 10-20% it can be replaced by a regular mutex.

Regards,

Hans

Hans Verkuil - video4linux developer - sponsored by TANDBERG