On Sat, Aug 8, 2009 at 1:46 PM, Alexey Klimov<klimov.linux@xxxxxxxxx> wrote:
> Redesign radio->users counter. Don't allow more that 5 users on radio in
> usb_amradio_open() and don't stop radio device if other userspace
> application uses it in usb_amradio_close().
>
> Signed-off-by: Alexey Klimov <klimov.linux@xxxxxxxxx>
>
> --
> diff -r c2dd9da28106 linux/drivers/media/radio/radio-mr800.c
> --- a/linux/drivers/media/radio/radio-mr800.c Sat Aug 08 17:28:18 2009 +0400
> +++ b/linux/drivers/media/radio/radio-mr800.c Sat Aug 08 18:12:01 2009 +0400
> @@ -540,7 +540,13 @@
> {
> struct amradio_device *radio = video_get_drvdata(video_devdata(file));
>
> - radio->users = 1;
> + /* don't allow more than 5 users on radio */
> + if (radio->users > 4)
> + return -EBUSY;
I agree with what the others have said regarding this.. there should
be no such restriction here. The code is broken anyway as it doesn't
acquire the lock before checking the number of active users. So
technically, while you've tried to limit it to five, six, seven, or
more users could gain access to it under the right conditions.
> +
> + mutex_lock(&radio->lock);
> + radio->users++;
> + mutex_unlock(&radio->lock);
>
> return 0;
> }
> @@ -554,9 +560,20 @@
> return -ENODEV;
>
> mutex_lock(&radio->lock);
> - radio->users = 0;
> + radio->users--;
> mutex_unlock(&radio->lock);
>
> + /* In case several userspace applications opened the radio
> + * and one of them closes and stops it,
> + * we check if others use it and if they do we start the radio again. */
> + if (radio->users && radio->status == AMRADIO_STOP) {
More locking issues here as well. Two competing opens might both see
the status as stopped and both try to start the device.
> + int retval;
> + retval = amradio_set_mute(radio, AMRADIO_START);
> + if (retval < 0)
> + dev_warn(&radio->videodev->dev,
> + "amradio_start failed\n");
> + }
> +
> return 0;
> }
>
Regards,
David Ellingsworth
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
