radio-mr800: redesign radio->users counter

PATCH In review Alexey Klimov 5 participants 7 messages Aug 8, 2009 – Aug 10, 2009 Media (V4L/DVB)

Share & tools

Alexey Klimov

author

2009-08-08 17:46 UTC #0 lore raw

PATCH 6/6 radio-mr800: redesign radio->users counter

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 <[email protected]>

linux/drivers/media/radio/radio-mr800.c +19 -2

diff -r c2dd9da28106 linux/drivers/media/radio/radio-mr800.c
@@ -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;
 }

Signature · 1 line

Best regards, Klimov Alexey

Trent Piepho

2009-08-08 18:01 UTC #1 lore raw

Quoted 1 line · Alexey Klimov

Redesign radio->users counter. Don't allow more that 5 users on radio in

Why?

Alexey Klimov

author

2009-08-08 19:08 UTC #2 lore raw

Quoted 4 lines · Trent Piepho

Quoted 1 line · Alexey Klimov
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.

Signature · 1 line

Best regards, Klimov Alexey

wk

2009-08-08 20:16 UTC #3 lore raw

Quoted 17 lines · Alexey Klimov

Quoted 6 lines · Trent Piepho
Quoted 2 lines · Alexey Klimov
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.

Quoted 4 lines

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.

Hans Verkuil

2009-08-09 08:19 UTC #4 lore raw

Quoted 18 lines · On Saturday 08 August 2009 22:16:28 wk wrote:

Quoted 17 lines · Alexey Klimov
Quoted 6 lines · Trent Piepho
Quoted 2 lines · Alexey Klimov
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.

Quoted 8 lines

Quoted 4 lines
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

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Alexey Klimov

author

2009-08-09 11:04 UTC #5 lore raw

Quoted 29 lines · Hans Verkuil

Quoted 18 lines · On Saturday 08 August 2009 22:16:28 wk wrote:
Quoted 17 lines · Alexey Klimov
Quoted 6 lines · Trent Piepho
Quoted 2 lines · Alexey Klimov
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.
Quoted 4 lines
Quoted 4 lines
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.

Quoted 6 lines

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

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

Signature · 1 line

Best regards, Klimov Alexey

David Ellingsworth

2009-08-10 22:24 UTC #6 lore raw

In reply to Alexey Klimov

Quoted 11 lines · 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 <[email protected]>
--
diff -r c2dd9da28106 linux/drivers/media/radio/radio-mr800.c

linux/drivers/media/radio/radio-mr800.c Sat Aug 08 18:12:01 2009 +0400 line 545

Quoted 7 lines

linux/drivers/media/radio/radio-mr800.c Sat Aug 08 18:12:01 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 17:28:18 2009 +0400 +3 -1
@@ -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.

Quoted 9 lines

linux/drivers/media/radio/radio-mr800.c Sat Aug 08 18:12:01 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 17:28:18 2009 +0400 +4 -0
+
+       mutex_lock(&radio->lock);
+       radio->users++;
+       mutex_unlock(&radio->lock);
 
        return 0;
  }
@@ -554,9 +560,20 @@
                return -ENODEV;

linux/drivers/media/radio/radio-mr800.c Sat Aug 08 18:12:01 2009 +0400 line 569

Quoted 10 lines

linux/drivers/media/radio/radio-mr800.c Sat Aug 08 18:12:01 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 17:28:18 2009 +0400 +5 -1

 
        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.

linux/drivers/media/radio/radio-mr800.c Sat Aug 08 18:12:01 2009 +0400 line 579

Quoted 10 lines

linux/drivers/media/radio/radio-mr800.c Sat Aug 08 18:12:01 2009 +0400 renamed from linux/drivers/media/radio/radio-mr800.c Sat Aug 08 17:28:18 2009 +0400 +7 -0

+               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