[patch review 6/6] radio-mr800: redesign radio->users counter

[patch review 6/6] radio-mr800: redesign radio->users counter

[Alexey Klimov]

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@gmail.com>

--
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;
+
+	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) {
+		int retval;
+		retval = amradio_set_mute(radio, AMRADIO_START);
+		if (retval < 0)
+			dev_warn(&radio->videodev->dev,
+				"amradio_start failed\n");
+	}
+
 	return 0;
 }
 


-- 
Best regards, Klimov Alexey

Re: [patch review 6/6] radio-mr800: redesign radio->users counter

[Trent Piepho]

On Sat, 8 Aug 2009, Alexey Klimov wrote:
> Redesign radio->users counter. Don't allow more that 5 users on radio in

Why?

Re: [patch review 6/6] radio-mr800: redesign radio->users counter

[Alexey Klimov]

On Sat, Aug 8, 2009 at 10:01 PM, Trent Piepho<xyzzy@speakeasy.org> wrote:
> On Sat, 8 Aug 2009, Alexey Klimov wrote:
>> Redesign radio->users counter. Don't allow more that 5 users on radio in
>
> Why?

Well, v4l2 specs says that multiple opens are optional. Honestly, i
think that five userspace applications open /dev/radio is enough. Btw,
if too many userspace applications opened radio that means that
something wrong happened in userspace. And driver can handle such
situation by disallowing new open calls(returning EBUSY). I can't
imagine user that runs more than five mplayers or gnomeradios, or
kradios and so on.

Am i totally wrong here?

Thanks.
-- 
Best regards, Klimov Alexey

Re: [patch review 6/6] radio-mr800: redesign radio->users counter

[wk]

Alexey Klimov schrieb:
> On Sat, Aug 8, 2009 at 10:01 PM, Trent Piepho<xyzzy@speakeasy.org> wrote:
>   
>> On Sat, 8 Aug 2009, Alexey Klimov wrote:
>>     
>>> Redesign radio->users counter. Don't allow more that 5 users on radio in
>>>       
>> Why?
>>     
>
> Well, v4l2 specs says that multiple opens are optional. Honestly, i
> think that five userspace applications open /dev/radio is enough. Btw,
> if too many userspace applications opened radio that means that
> something wrong happened in userspace. And driver can handle such
> situation by disallowing new open calls(returning EBUSY). I can't
> imagine user that runs more than five mplayers or gnomeradios, or
> kradios and so on.
>
> Am i totally wrong here?
>
> Thanks.
>   
"I can't imagine.." Funny answer, reminds at the 640kB limit of old 
computers.. :)
But if there's no real technical restriction, the driver should not 
restrict access a device at all.

Re: [patch review 6/6] radio-mr800: redesign radio->users counter

[Hans Verkuil]

On Saturday 08 August 2009 22:16:28 wk wrote:
> Alexey Klimov schrieb:
> > On Sat, Aug 8, 2009 at 10:01 PM, Trent Piepho<xyzzy@speakeasy.org> wrote:
> >   
> >> On Sat, 8 Aug 2009, Alexey Klimov wrote:
> >>     
> >>> Redesign radio->users counter. Don't allow more that 5 users on radio in
> >>>       
> >> Why?
> >>     
> >
> > Well, v4l2 specs says that multiple opens are optional. Honestly, i
> > think that five userspace applications open /dev/radio is enough. Btw,
> > if too many userspace applications opened radio that means that
> > something wrong happened in userspace. And driver can handle such
> > situation by disallowing new open calls(returning EBUSY). I can't
> > imagine user that runs more than five mplayers or gnomeradios, or
> > kradios and so on.
> >
> > Am i totally wrong here?
> >
> > Thanks.
> >   
> "I can't imagine.." Funny answer, reminds at the 640kB limit of old 
> computers.. :)
> But if there's no real technical restriction, the driver should not 
> restrict access a device at all.

Exactly. It's an artificial restriction that serves no purpose. Also
remember that apps can open a radio device just to do e.g. a QUERYCAP
or something like that. It does not necessarily has to be an mplayer
or gnomeradio.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [patch review 6/6] radio-mr800: redesign radio->users counter

[Alexey Klimov]

On Sun, Aug 9, 2009 at 12:19 PM, Hans Verkuil<hverkuil@xs4all.nl> wrote:
> On Saturday 08 August 2009 22:16:28 wk wrote:
>> Alexey Klimov schrieb:
>> > On Sat, Aug 8, 2009 at 10:01 PM, Trent Piepho<xyzzy@speakeasy.org> wrote:
>> >
>> >> On Sat, 8 Aug 2009, Alexey Klimov wrote:
>> >>
>> >>> Redesign radio->users counter. Don't allow more that 5 users on radio in
>> >>>
>> >> Why?
>> >>
>> >
>> > Well, v4l2 specs says that multiple opens are optional. Honestly, i
>> > think that five userspace applications open /dev/radio is enough. Btw,
>> > if too many userspace applications opened radio that means that
>> > something wrong happened in userspace. And driver can handle such
>> > situation by disallowing new open calls(returning EBUSY). I can't
>> > imagine user that runs more than five mplayers or gnomeradios, or
>> > kradios and so on.
>> >
>> > Am i totally wrong here?
>> >
>> > Thanks.
>> >
>
> Exactly. It's an artificial restriction that serves no purpose. Also
> remember that apps can open a radio device just to do e.g. a QUERYCAP
> or something like that. It does not necessarily has to be an mplayer
> or gnomeradio.
>
> Regards,
>
>        Hans
>
> --
> Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Ok, i'll update the patch.
Idea initially came in mind from gspca.c.

-- 
Best regards, Klimov Alexey

Re: [patch review 6/6] radio-mr800: redesign radio->users counter

[David Ellingsworth]

On Sat, Aug 8, 2009 at 1:46 PM, Alexey Klimov<klimov.linux@gmail.com> 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@gmail.com>
>
> --
> 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