radio-mr800: correct unplug, fix to previous patch

This patch corrects unplug procedure, that was implemented wrong in previous patch. New function usb_amradio_device_release added. Disconnect lock removed.

---

diff -r 602d3ac1f476 linux/drivers/media/radio/radio-mr800.c
@@ -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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Should i remake this patch now ? Or make one more patch ?

Best regards, Klimov Alexey

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hello, David

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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list