[patch review 1/6] radio-mr800: remove redundant lock/unlock_kernel

[patch review 1/6] radio-mr800: remove redundant lock/unlock_kernel

[Alexey Klimov]

Remove redundant lock/unlock_kernel() calls from usb_amradio_open/close
functions.

Signed-off-by: Alexey Klimov <klimov.linux@gmail.com>

--
diff -r ee6cf88cb5d3 linux/drivers/media/radio/radio-mr800.c
--- a/linux/drivers/media/radio/radio-mr800.c	Wed Jul 29 01:42:02 2009 -0300
+++ b/linux/drivers/media/radio/radio-mr800.c	Wed Jul 29 10:44:02 2009 +0400
@@ -540,8 +540,6 @@
 	struct amradio_device *radio = video_get_drvdata(video_devdata(file));
 	int retval;
 
-	lock_kernel();
-
 	radio->users = 1;
 	radio->muted = 1;
 
@@ -550,7 +548,6 @@
 		amradio_dev_warn(&radio->videodev->dev,
 			"radio did not start up properly\n");
 		radio->users = 0;
-		unlock_kernel();
 		return -EIO;
 	}
 
@@ -564,7 +561,6 @@
 		amradio_dev_warn(&radio->videodev->dev,
 			"set frequency failed\n");
 
-	unlock_kernel();
 	return 0;
 }
 


-- 
Best regards, Klimov Alexey

Re: [patch review 1/6] radio-mr800: remove redundant lock/unlock_kernel

[David Ellingsworth]

On Sat, Aug 8, 2009 at 1:45 PM, Alexey Klimov<klimov.linux@gmail.com> wrote:
> Remove redundant lock/unlock_kernel() calls from usb_amradio_open/close
> functions.
>
> Signed-off-by: Alexey Klimov <klimov.linux@gmail.com>
>
> --
> diff -r ee6cf88cb5d3 linux/drivers/media/radio/radio-mr800.c
> --- a/linux/drivers/media/radio/radio-mr800.c   Wed Jul 29 01:42:02 2009 -0300
> +++ b/linux/drivers/media/radio/radio-mr800.c   Wed Jul 29 10:44:02 2009 +0400
> @@ -540,8 +540,6 @@
>        struct amradio_device *radio = video_get_drvdata(video_devdata(file));
>        int retval;
>
> -       lock_kernel();
> -

Maybe I'm missing something here, but the lock_kernel() call seems
very necessary here since you're modifying the state of the driver
without taking the driver's lock. Last I checked the v4l2 subsystem
doesn't call open with it's lock held. So by removing these locks
you've introduced a race condition between open and close.

>        radio->users = 1;
>        radio->muted = 1;
>
> @@ -550,7 +548,6 @@
>                amradio_dev_warn(&radio->videodev->dev,
>                        "radio did not start up properly\n");
>                radio->users = 0;
> -               unlock_kernel();
>                return -EIO;
>        }
>
> @@ -564,7 +561,6 @@
>                amradio_dev_warn(&radio->videodev->dev,
>                        "set frequency failed\n");
>
> -       unlock_kernel();
>        return 0;
>  }
>
>
>
> --
> Best regards, Klimov Alexey
>

Regards,

David Ellingsworth