[RFC] BKL in open functions in drivers

[RFC] BKL in open functions in drivers

[Alexey Klimov]

Hello,

Few days ago Alessio Igor Bogani<abogani@texware.it> 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

Re: [RFC] BKL in open functions in drivers

[Hans Verkuil]

On Wednesday 01 April 2009 23:00:56 Alexey Klimov wrote:
> Hello,
>
> Few days ago Alessio Igor Bogani<abogani@texware.it> 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?

It's pointless. Just remove it.

> 2) Can it be replaced by mutex, for example?

No need.

> Please, comments, exaplanations are more than welcome.

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

Re: [RFC] BKL in open functions in drivers

[Alexey Klimov]

On Thu, Apr 2, 2009 at 11:29 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:

[...]

>> So, questions are:
>>
>> 1) What for is lock/unlock_kernel() used in open?
>
> It's pointless. Just remove it.

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

>> 2) Can it be replaced by mutex, for example?
>
> No need.

Good, so we can remove it.

>> Please, comments, explanations are more than welcome.
>
> 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

Agreed, thanks for explanations and suggestions.

-- 
Best regards, Klimov Alexey

Re: [RFC] BKL in open functions in drivers

[Hans Verkuil]

On Thursday 02 April 2009 20:25:10 Alexey Klimov wrote:
> On Thu, Apr 2, 2009 at 11:29 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
> [...]
> 
> >> So, questions are:
> >>
> >> 1) What for is lock/unlock_kernel() used in open?
> >
> > It's pointless. Just remove it.
> 
> Actually, i can see lock/unlock_kernel() in open in other V4L drivers too.
> What for is it used in other drivers?

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