Message ID | 1249753576.15160.251.camel@tux.localhost (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@infradead.org Delivery-date: Sat, 08 Aug 2009 17:46:19 +0000 Received: from bombadil.infradead.org [18.85.46.34] by pedra.chehab.org with IMAP (fetchmail-6.3.6) for <mchehab@localhost> (single-drop); Sat, 08 Aug 2009 14:46:08 -0300 (BRT) Received: from vger.kernel.org ([209.132.176.167]) by bombadil.infradead.org with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1MZpzm-0005kZ-Uc; Sat, 08 Aug 2009 17:46:19 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752716AbZHHRqP (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Sat, 8 Aug 2009 13:46:15 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752741AbZHHRqP (ORCPT <rfc822;linux-media-outgoing>); Sat, 8 Aug 2009 13:46:15 -0400 Received: from mail-ew0-f214.google.com ([209.85.219.214]:51190 "EHLO mail-ew0-f214.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752716AbZHHRqO (ORCPT <rfc822; linux-media@vger.kernel.org>); Sat, 8 Aug 2009 13:46:14 -0400 Received: by mail-ew0-f214.google.com with SMTP id 10so2174216ewy.37 for <linux-media@vger.kernel.org>; Sat, 08 Aug 2009 10:46:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:subject:from:to:cc :content-type:date:message-id:mime-version:x-mailer :content-transfer-encoding; bh=4P9HSVdOZvKPs9A6GS7nFSOHwNepcQmIvPx+HV5FKl0=; b=bgJadQO54PnRvPiiB4ks3hl3InouYNXe5N+i/nUYBoDwYIyzpGSdnR7sH9XghBGW8c skqhLDLSLx90D94bXaMB+Kx+yqRjzQwQXAD0dkVhIE/+DNBhcRu7cLi6uW9AwGdN+A8z zBYdYRk6kCiXxlW4QfufIUlgKdTvXKOYPCAo0= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:to:cc:content-type:date:message-id:mime-version :x-mailer:content-transfer-encoding; b=b+idGK1Dx6PK9WjsZ320s/6cXFkOkV9hgcRd9YbImwbrjJNgXBz7ZIyoykIO37rWHR 4VAXXHsYA6xS3YS+EsE7EIid6SFxyQXejcM7FWEwdyLweBUoVL6tAc425zIGS3SYt1j6 IElrwJePSrUPihCPRdKLYE98c9E0nEodUTwRc= Received: by 10.210.57.15 with SMTP id f15mr902675eba.54.1249753575839; Sat, 08 Aug 2009 10:46:15 -0700 (PDT) Received: from ?95.78.132.126? ([95.78.132.126]) by mx.google.com with ESMTPS id 10sm6282676eyz.31.2009.08.08.10.46.15 (version=SSLv3 cipher=RC4-MD5); Sat, 08 Aug 2009 10:46:15 -0700 (PDT) Subject: [patch review 6/6] radio-mr800: redesign radio->users counter From: Alexey Klimov <klimov.linux@gmail.com> To: Douglas Schilling Landgraf <dougsland@gmail.com> Cc: linux-media@vger.kernel.org Content-Type: text/plain Date: Sat, 08 Aug 2009 21:46:16 +0400 Message-Id: <1249753576.15160.251.camel@tux.localhost> Mime-Version: 1.0 X-Mailer: Evolution 2.26.3 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Alexey Klimov
Aug. 8, 2009, 5:46 p.m. UTC
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>
--
Comments
On Sat, 8 Aug 2009, Alexey Klimov wrote:
> Redesign radio->users counter. Don't allow more that 5 users on radio in
Why?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }