* [PATCH 1/2] radio-mr800: correct unplug, fix to previous patch
@ 2008-12-08 16:14 Alexey Klimov
2008-12-08 22:03 ` David Ellingsworth
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Klimov @ 2008-12-08 16:14 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: video4linux-list, David Ellingsworth
This patch corrects unplug procedure, that was implemented wrong in
previous patch. New function usb_amradio_device_release added.
Disconnect lock removed.
Signed-off-by: Alexey Klimov <klimov.linux@gmail.com>
---
diff -r 602d3ac1f476 linux/drivers/media/radio/radio-mr800.c
--- a/linux/drivers/media/radio/radio-mr800.c Thu Nov 20 19:47:37 2008 -0200
+++ b/linux/drivers/media/radio/radio-mr800.c Mon Dec 08 17:42:06 2008 +0300
@@ -142,7 +142,6 @@
unsigned char *buffer;
struct mutex lock; /* buffer locking */
- struct mutex disconnect_lock;
int curfreq;
int stereo;
int users;
@@ -305,16 +304,12 @@
{
struct amradio_device *radio = usb_get_intfdata(intf);
- mutex_lock(&radio->disconnect_lock);
+ mutex_lock(&radio->lock);
radio->removed = 1;
+ mutex_unlock(&radio->lock);
+
usb_set_intfdata(intf, NULL);
-
- if (radio->users == 0) {
- video_unregister_device(radio->videodev);
- kfree(radio->buffer);
- kfree(radio);
- }
- mutex_unlock(&radio->disconnect_lock);
+ video_unregister_device(radio->videodev);
}
/* vidioc_querycap - query device capabilities */
@@ -532,7 +527,7 @@
return 0;
}
-/*close device - free driver structures */
+/*close device */
static int usb_amradio_close(struct inode *inode, struct file *file)
{
struct amradio_device *radio = video_get_drvdata(video_devdata(file));
@@ -541,21 +536,15 @@
if (!radio)
return -ENODEV;
- mutex_lock(&radio->disconnect_lock);
radio->users = 0;
- if (radio->removed) {
- video_unregister_device(radio->videodev);
- kfree(radio->buffer);
- kfree(radio);
- } else {
+ if (!radio->removed) {
retval = amradio_stop(radio);
if (retval < 0)
amradio_dev_warn(&radio->videodev->dev,
"amradio_stop failed\n");
}
- mutex_unlock(&radio->disconnect_lock);
return 0;
}
@@ -612,12 +601,24 @@
.vidioc_s_input = vidioc_s_input,
};
+static void usb_amradio_device_release(struct video_device *videodev)
+{
+ struct amradio_device *radio = video_get_drvdata(videodev);
+
+ /* we call v4l to free radio->videodev */
+ video_device_release(videodev);
+
+ /* free rest memory */
+ kfree(radio->buffer);
+ kfree(radio);
+}
+
/* V4L2 interface */
static struct video_device amradio_videodev_template = {
.name = "AverMedia MR 800 USB FM Radio",
.fops = &usb_amradio_fops,
.ioctl_ops = &usb_amradio_ioctl_ops,
- .release = video_device_release,
+ .release = usb_amradio_device_release,
};
/* check if the device is present and register with v4l and
@@ -655,7 +656,6 @@
radio->usbdev = interface_to_usbdev(intf);
radio->curfreq = 95.16 * FREQ_MUL;
- mutex_init(&radio->disconnect_lock);
mutex_init(&radio->lock);
video_set_drvdata(radio->videodev, radio);
--
Best regards, Klimov Alexey
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] radio-mr800: correct unplug, fix to previous patch
2008-12-08 16:14 [PATCH 1/2] radio-mr800: correct unplug, fix to previous patch Alexey Klimov
@ 2008-12-08 22:03 ` David Ellingsworth
2008-12-08 22:10 ` Alexey Klimov
0 siblings, 1 reply; 6+ messages in thread
From: David Ellingsworth @ 2008-12-08 22:03 UTC (permalink / raw)
To: Alexey Klimov; +Cc: video4linux-list, Mauro Carvalho Chehab
This patch looks good, but I think it should remove the now unused
user member of the amradio struct as well.
Regards,
David Ellingsworth
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] radio-mr800: correct unplug, fix to previous patch
2008-12-08 22:03 ` David Ellingsworth
@ 2008-12-08 22:10 ` Alexey Klimov
2008-12-09 0:20 ` David Ellingsworth
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Klimov @ 2008-12-08 22:10 UTC (permalink / raw)
To: David Ellingsworth; +Cc: video4linux-list, Mauro Carvalho Chehab
On Tue, Dec 9, 2008 at 1:03 AM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> This patch looks good, but I think it should remove the now unused
> user member of the amradio struct as well.
Should i remake this patch now ? Or make one more patch ?
--
Best regards, Klimov Alexey
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] radio-mr800: correct unplug, fix to previous patch
2008-12-08 22:10 ` Alexey Klimov
@ 2008-12-09 0:20 ` David Ellingsworth
2008-12-09 12:49 ` David Ellingsworth
0 siblings, 1 reply; 6+ messages in thread
From: David Ellingsworth @ 2008-12-09 0:20 UTC (permalink / raw)
To: Alexey Klimov; +Cc: video4linux-list, Mauro Carvalho Chehab
On Mon, Dec 8, 2008 at 5:10 PM, Alexey Klimov <klimov.linux@gmail.com> wrote:
> On Tue, Dec 9, 2008 at 1:03 AM, David Ellingsworth
> <david@identd.dyndns.org> wrote:
>> This patch looks good, but I think it should remove the now unused
>> user member of the amradio struct as well.
>
> Should i remake this patch now ? Or make one more patch ?
>
I'd make a separate patch. Leaving the users member variable doesn't
cause any harm; removing it is just a driver simplification/cleanup.
Regards,
David Ellingsworth
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] radio-mr800: correct unplug, fix to previous patch
2008-12-09 0:20 ` David Ellingsworth
@ 2008-12-09 12:49 ` David Ellingsworth
2008-12-13 2:41 ` Alexey Klimov
0 siblings, 1 reply; 6+ messages in thread
From: David Ellingsworth @ 2008-12-09 12:49 UTC (permalink / raw)
To: Alexey Klimov; +Cc: video4linux-list, Mauro Carvalho Chehab
On Mon, Dec 8, 2008 at 7:20 PM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> On Mon, Dec 8, 2008 at 5:10 PM, Alexey Klimov <klimov.linux@gmail.com> wrote:
>> On Tue, Dec 9, 2008 at 1:03 AM, David Ellingsworth
>> <david@identd.dyndns.org> wrote:
>>> This patch looks good, but I think it should remove the now unused
>>> user member of the amradio struct as well.
>>
>> Should i remake this patch now ? Or make one more patch ?
>>
>
> I'd make a separate patch. Leaving the users member variable doesn't
> cause any harm; removing it is just a driver simplification/cleanup.
>
I thought about this more and you might want to reconstitute the
meaning of the users member of the amradio struct. Currently
amradio_start is called whenever the device is opened and amradio_stop
is called whenever it is closed. Thus if the device were opened twice,
it would call the start routine twice. The other issue is that once it
has been opened more than one time, the first time it is closed,
amradio_stop is called. It may make more since to turn this member
into a counter that is incremented on open and decremented on close.
Then if the counter is 0 on open, before increment, call amradio_start
and if it's 0 on close, after decrement, call amradio_stop. Once this
is done, amradio_start will only be called once when the device is
initially opened and amradio_stop will be called once when during
close when all users all users have closed the device. I don't know if
it matters or not, but you may also want to take a look at the control
which mutes the device for it seems to call amradio_start and
amradio_stop as well. Ideally if the device is stopped by the user
before the final close amradio_stop should not be called, so you
should check if it's not muted as well.
Finally I noticed that the muted flag is not modified within the
context of the lock in amradio_start and amradio_stop. This should be
corrected to ensure a race condition doesn't exist between the two
states. I also think that amradio_start and amradio_stop should verify
the device's state before proceeding. For example if the device is not
muted during a call to amradio_start it should return an error since
it's already started. And if it's muted during a call to amradio_stop
it should return an error.
The suspend/resume functions needs to be fixed as well. amradio_start
should only be called if the device wasn't muted when it was
suspended. Likewise, amradio_stop should only be called during suspend
if the device isn't muted. Since the amradio_start and amradio_stop
manipulate the muted variable you may need another variable to
determine how to resume from suspend. You might be able to use the
muted variable, by resetting it to one to on suspend after calling
amradio_stop when it's not muted. And then resetting it to 0 in resume
if it's 1 before calling amradio_start in resume. None the less,
resume/suspend should return the device to it's prior state. IE. if it
was muted upon suspend, it should be muted upon resume and vice versa.
Regards,
David Ellingsworth
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] radio-mr800: correct unplug, fix to previous patch
2008-12-09 12:49 ` David Ellingsworth
@ 2008-12-13 2:41 ` Alexey Klimov
0 siblings, 0 replies; 6+ messages in thread
From: Alexey Klimov @ 2008-12-13 2:41 UTC (permalink / raw)
To: David Ellingsworth; +Cc: video4linux-list, Mauro Carvalho Chehab
Hello, David
On Tue, Dec 9, 2008 at 3:49 PM, David Ellingsworth
<david@identd.dyndns.org> wrote:
>
> I thought about this more and you might want to reconstitute the
> meaning of the users member of the amradio struct. Currently
> amradio_start is called whenever the device is opened and amradio_stop
> is called whenever it is closed. Thus if the device were opened twice,
> it would call the start routine twice. The other issue is that once it
> has been opened more than one time, the first time it is closed,
> amradio_stop is called. It may make more since to turn this member
> into a counter that is incremented on open and decremented on close.
> Then if the counter is 0 on open, before increment, call amradio_start
> and if it's 0 on close, after decrement, call amradio_stop. Once this
> is done, amradio_start will only be called once when the device is
> initially opened and amradio_stop will be called once when during
> close when all users all users have closed the device. I don't know if
> it matters or not, but you may also want to take a look at the control
> which mutes the device for it seems to call amradio_start and
> amradio_stop as well. Ideally if the device is stopped by the user
> before the final close amradio_stop should not be called, so you
> should check if it's not muted as well.
>
> Finally I noticed that the muted flag is not modified within the
> context of the lock in amradio_start and amradio_stop. This should be
> corrected to ensure a race condition doesn't exist between the two
> states. I also think that amradio_start and amradio_stop should verify
> the device's state before proceeding. For example if the device is not
> muted during a call to amradio_start it should return an error since
> it's already started. And if it's muted during a call to amradio_stop
> it should return an error.
>
> The suspend/resume functions needs to be fixed as well. amradio_start
> should only be called if the device wasn't muted when it was
> suspended. Likewise, amradio_stop should only be called during suspend
> if the device isn't muted. Since the amradio_start and amradio_stop
> manipulate the muted variable you may need another variable to
> determine how to resume from suspend. You might be able to use the
> muted variable, by resetting it to one to on suspend after calling
> amradio_stop when it's not muted. And then resetting it to 0 in resume
> if it's 1 before calling amradio_start in resume. None the less,
> resume/suspend should return the device to it's prior state. IE. if it
> was muted upon suspend, it should be muted upon resume and vice versa.
This works like a todo-list for next activity(work) about driver for
me. I'll be guided by your advices.
Thanks for reviewing and help, David. Your answers are always very
accurate and rational.
--
Best regards, Klimov Alexey
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-12-13 2:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-12-08 16:14 [PATCH 1/2] radio-mr800: correct unplug, fix to previous patch Alexey Klimov
2008-12-08 22:03 ` David Ellingsworth
2008-12-08 22:10 ` Alexey Klimov
2008-12-09 0:20 ` David Ellingsworth
2008-12-09 12:49 ` David Ellingsworth
2008-12-13 2:41 ` Alexey Klimov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox