On Wednesday 01 April 2009 23:00:56 Alexey Klimov wrote:
> 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?
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
