radio-mr800: fix unplug

This patch fixes problems(kernel oopses) with unplug of device while it's working.
Patch adds disconnect_lock mutex, changes usb_amradio_close and usb_amradio_disconnect functions and adds a lot of safety checks.

Signed-off-by Alexey Klimov <[email protected]>

---

diff -r 1536e16ffdf1 linux/drivers/media/radio/radio-mr800.c

linux/drivers/media/radio/radio-mr800.c Wed Nov 19 03:27:59 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Tue Nov 18 15:51:08 2008 -0200 +9 -0

@@ -142,6 +142,7 @@
 
       unsigned char *buffer;
       struct mutex lock;      /* buffer locking */
+       struct mutex disconnect_lock;
       int curfreq;
       int stereo;
       int users;

@@ -210,6 +211,10 @@
       int retval;
       int size;
 
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
+
       mutex_lock(&radio->lock);
 
       radio->buffer[0] = 0x00;

@@ -242,6 +247,10 @@
       int retval;
       int size;
       unsigned short freq_send = 0x13 + (freq >> 3) / 25;
+
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
 
       mutex_lock(&radio->lock);
 

linux/drivers/media/radio/radio-mr800.c Wed Nov 19 03:27:59 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Tue Nov 18 15:51:08 2008 -0200 +27 -0

       }
+       mutex_unlock(&radio->disconnect_lock);
 }
 
 /* vidioc_querycap - query device capabilities */

@@ -327,6 +334,10 @@
                               struct v4l2_tuner *v)
 {
       struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
 
       if (v->index > 0)
               return -EINVAL;

@@ -354,6 +365,12 @@
 static int vidioc_s_tuner(struct file *file, void *priv,
                               struct v4l2_tuner *v)
 {
+       struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
+
       if (v->index > 0)
               return -EINVAL;
       return 0;

@@ -364,6 +381,10 @@
                               struct v4l2_frequency *f)
 {
       struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
 
       radio->curfreq = f->frequency;
       if (amradio_setfreq(radio, radio->curfreq) < 0)

@@ -377,6 +398,10 @@
                               struct v4l2_frequency *f)
 {
       struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
 
       f->type = V4L2_TUNER_RADIO;
       f->frequency = radio->curfreq;

@@ -404,6 +429,10 @@
 {
       struct amradio_device *radio = video_get_drvdata(video_devdata(file));
 
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
+
       switch (ctrl->id) {
       case V4L2_CID_AUDIO_MUTE:
               ctrl->value = radio->muted;

@@ -417,6 +446,10 @@
                               struct v4l2_control *ctrl)
 {
       struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
 
       switch (ctrl->id) {
       case V4L2_CID_AUDIO_MUTE:

@@ -503,14 +536,26 @@
 static int usb_amradio_close(struct inode *inode, struct file *file)
 {

linux/drivers/media/radio/radio-mr800.c Wed Nov 19 03:27:59 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Tue Nov 18 15:51:08 2008 -0200 +8 -0

+
+       } else {
+               retval = amradio_stop(radio);
+               if (retval < 0)
+                       amradio_dev_warn(&radio->videodev->dev,
+                               "amradio_stop failed\n");
       }
+
+       mutex_unlock(&radio->disconnect_lock);
       return 0;
 }
 

linux/drivers/media/radio/radio-mr800.c Wed Nov 19 03:27:59 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Tue Nov 18 15:51:08 2008 -0200 +0 -1

       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

NACK

video_unregister_device should _always_ be called once the device is disconnect, no matter how many handles are still open.

linux/drivers/media/radio/radio-mr800.c line 315

Quoted 9 lines

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

-               radio->videodev = NULL;
-               if (radio->users) {
-                       kfree(radio->buffer);
-                       kfree(radio);
-               } else {
-                       radio->removed = 1;
-               }
+               kfree(radio->buffer);
+               kfree(radio);

You should not be freeing memory here. The video_device release callback should be used for this purpose. It is called once all open file handles are closed and after video_unregister_device has been called.

Again, video_unregister_device should always be called from the usb disconnect callback.

Quoted 2 lines

               kfree(radio->buffer);
               kfree(radio);

Again, memory should not be freed here. It should be freed by the video_device release callback for reasons stated above.

I suspect you'll find little need for this mutex once you have properly implemented the video_device release callback. You may however still need the removed flag as some usb calls obviously can't be made once the device has been removed. For reference, please review the stk-webcam driver as it implements this properly

Hello, David

Quoted 15 lines · David Ellingsworth

NACK

video_unregister_device should _always_ be called once the device is disconnect, no matter how many handles are still open.

linux/drivers/media/radio/radio-mr800.c line 315

Quoted 9 lines

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

-               radio->videodev = NULL;
-               if (radio->users) {
-                       kfree(radio->buffer);
-                       kfree(radio);
-               } else {
-                       radio->removed = 1;
-               }
+               kfree(radio->buffer);
+               kfree(radio);

Quoted 4 lines

You should not be freeing memory here. The video_device release callback should be used for this purpose. It is called once all open file handles are closed and after video_unregister_device has been called.

Well, things what you said make me feel ill at ease (feel uncomfortable). Looks like 3 usb radio drivers don't implement right disconnect and video release functions ?
Generaly, i took order of release/kfree-functions from dsbr100 and si470x.

Quoted 9 lines

Again, video_unregister_device should always be called from the usb disconnect callback.

Quoted 2 lines

               kfree(radio->buffer);
               kfree(radio);

Again, memory should not be freed here. It should be freed by the video_device release callback for reasons stated above.

Ok. I were in deep quest of finding video_device release callback. I had release function only in file_operations, but it wasn't right function. Then i found video_device_release in video_device amradio_videodev_template.
Looks like disconnect function called before video_device_release in all cases. And i need to call kfree(radio) after disconnect but before probe function(if device pluged in again).

Do this general examples below look right ?

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_am,

};

I need my own release function, right ? To free radio structure.

void video_device_release_am(struct video_device *videodev)

{
       struct amradio_device *radio = video_get_drvdata(videodev);
       printk("we are in video_device_release\n");
       video_device_release(videodev);
       kfree(radio->buffer);
       kfree(radio);
}

May be something like "container_of" to get *radio from *videodev ? Or it's okay ?

static void usb_amradio_disconnect(struct usb_interface *intf)

{
       struct amradio_device *radio = usb_get_intfdata(intf);

       printk("disconnect called\n");
//      mutex_lock(&radio->disconnect_lock);
       radio->removed = 1;

       usb_set_intfdata(intf, NULL);
       video_unregister_device(radio->videodev);

// mutex_unlock(&radio->disconnect_lock);
}

Quoted 5 lines

I suspect you'll find little need for this mutex once you have properly implemented the video_device release callback. You may however still need the removed flag as some usb calls obviously can't be made once the device has been removed. For reference, please review the stk-webcam driver as it implements this properly

Thanks for pointing this out. I think that disconnect lock is not necessarily.

Quoted 23 lines · Alexey Klimov

Hello, David

Quoted 15 lines · David Ellingsworth

NACK

video_unregister_device should _always_ be called once the device is disconnect, no matter how many handles are still open.

Review

Quoted 9 lines

(quoted diff) +2 -7

-               radio->videodev = NULL;
-               if (radio->users) {
-                       kfree(radio->buffer);
-                       kfree(radio);
-               } else {
-                       radio->removed = 1;
-               }
+               kfree(radio->buffer);
+               kfree(radio);

Quoted 4 lines

You should not be freeing memory here. The video_device release callback should be used for this purpose. It is called once all open file handles are closed and after video_unregister_device has been called.

Quoted 5 lines

Well, things what you said make me feel ill at ease (feel uncomfortable). Looks like 3 usb radio drivers don't implement right disconnect and video release functions ?
Generaly, i took order of release/kfree-functions from dsbr100 and si470x.

Yes, this is probably true. Not many drivers handled this properly since the video_device release callback used to occur immediately after calling video_unregister_device while the structure was potentially still being used. Some drivers implemented a kref object to work around this deficiency, others used a user count. Now that the v4l2-core correctly calls this release after video_unregister_device has been called and all open handles have been close, no work-arounds are needed by v4l2 sub-drivers.

The suggested method to simply set the video_device release callback to video_device_release was from the beginning flawed. The only instance where it was actually safe to do so was in the vivi driver since there was no way to abruptly remove the device while it was still being used. All usb and pci devices can be removed at will and have to properly handle that case. Sadly most do not. In my opinion, the video_device_alloc and video_device_release functions should be removed entirely, they only add unnecessary complexity to a v4l2 sub-driver.

Before the v4l2-core was fixed, if a driver set the release callback to video_device_release and _always_ called video_unregister_device in it's disconnect callback then the video_device struct used by the driver would be immediately freed. Once the video_device struct had been freed, any call to video_get_drvdata would fail and crash the driver. Sub-drivers could then do one of two things to avoid this crash, 1. not call video_unregister_device upon disconnect, or 2. set the release callback to a null function. The problem with the first method is that by not calling video_unregister_device the driver had to ensure the device was present during the fops open callback, aka handling a call to open after disconnect. The problem with the second method is that the sub-driver had to implement it's own reference count to determine when it was safe to free it's structure. Both methods had their own challenges and neither was something that most drivers got right.

Quoted 18 lines

Quoted 9 lines

Again, video_unregister_device should always be called from the usb disconnect callback.

Quoted 2 lines

               kfree(radio->buffer);
               kfree(radio);

Again, memory should not be freed here. It should be freed by the video_device release callback for reasons stated above.

Ok. I were in deep quest of finding video_device release callback. I had release function only in file_operations, but it wasn't right function. Then i found video_device_release in video_device amradio_videodev_template.
Looks like disconnect function called before video_device_release in all cases. And i need to call kfree(radio) after disconnect but before probe function(if device pluged in again).

Quoted 1 line

Do this general examples below look right ?

Yes, this looks correct.

Quoted 9 lines

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_am,

};

I need my own release function, right ? To free radio structure.

Correct. You'll need your own function.

Quoted 10 lines

void video_device_release_am(struct video_device *videodev)

{
       struct amradio_device *radio = video_get_drvdata(videodev);
       printk("we are in video_device_release\n");
       video_device_release(videodev);
       kfree(radio->buffer);
       kfree(radio);
}

May be something like "container_of" to get *radio from *videodev ? Or it's okay ?

Since the video_device struct is not embedded within the amradio_device structure you can't use container_of to get a pointer to the amradio_device structure. What you have here is correct. If you choose to do so, I advocate embedding the video_device struct into the amradio struct since it simplifies memory allocation. You would then be able to use container_of in many places to go from the video_device struct to the amradio_device struct.

Quoted 12 lines

static void usb_amradio_disconnect(struct usb_interface *intf)

{
       struct amradio_device *radio = usb_get_intfdata(intf);

       printk("disconnect called\n");
//      mutex_lock(&radio->disconnect_lock);
       radio->removed = 1;

       usb_set_intfdata(intf, NULL);
       video_unregister_device(radio->videodev);

Quoted 8 lines

// mutex_unlock(&radio->disconnect_lock);
}

Quoted 5 lines

I suspect you'll find little need for this mutex once you have properly implemented the video_device release callback. You may however still need the removed flag as some usb calls obviously can't be made once the device has been removed. For reference, please review the stk-webcam driver as it implements this properly

If you have a general mutex, which you should, for locking access to the amradio_device struct it should probably be used here while setting the removed member to 1. Admittedly, stk-webcam currently lacks the required mutex. The state change in it's disconnect callback is not safe and should be protected by a general mutex, as should operations in it's open, release, and read functions. I merely haven't had the time to submit a patch to correct it.

Hello, David

Do this patch looks correct ? (it is fix for current hg-tree) I'm still confused about part of probe-function where video_register_device failed and we should free memory. Will disconnect function be called if probe-function failed ?

It's in the end of this patch. David, if you have some free time - can you you review it ?

diff -r 602d3ac1f476 linux/drivers/media/radio/radio-mr800.c

linux/drivers/media/radio/radio-mr800.c Wed Nov 26 17:29:54 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Thu Nov 20 19:47:37 2008 -0200 +6 -17

@@ -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;
 }
 

linux/drivers/media/radio/radio-mr800.c Wed Nov 26 17:29:54 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Thu Nov 20 19:47:37 2008 -0200 +5 -1

+}
+
+
+
 /* 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,15 +662,12 @@
       radio->usbdev = interface_to_usbdev(intf);
       radio->curfreq = 95.16 * FREQ_MUL;
 

Quoted 4 lines

Do this patch looks correct ? (it is fix for current hg-tree) I'm still confused about part of probe-function where video_register_device failed and we should free memory. Will disconnect function be called if probe-function failed ?

The disconnect function will not be called if the probe method fails. The usb subsystem only calls the disconnect function for successfully probed devices.

Quoted 65 lines

diff -r 602d3ac1f476 linux/drivers/media/radio/radio-mr800.c

linux/drivers/media/radio/radio-mr800.c Wed Nov 26 17:29:54 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Thu Nov 20 19:47:37 2008 -0200 +6 -17

@@ -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;
 }
 

linux/drivers/media/radio/radio-mr800.c Wed Nov 26 17:29:54 2008 +0300 line 608

Quoted 8 lines

linux/drivers/media/radio/radio-mr800.c Wed Nov 26 17:29:54 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Thu Nov 20 19:47:37 2008 -0200 +5 -0

@@ -612,12 +601,30 @@
       .vidioc_s_input     = vidioc_s_input,
 };
 
+static void usb_amradio_device_release(struct video_device *videodev)
+{
+       struct amradio_device *radio = video_get_drvdata(videodev);
+
+       mutex_lock(&radio->lock);

The lock here is completely unnecessary for you are guaranteed the structure is not in use due to the reference counting done by the v4l2 core.

linux/drivers/media/radio/radio-mr800.c Wed Nov 26 17:29:54 2008 +0300 line 617

Quoted 9 lines

linux/drivers/media/radio/radio-mr800.c Wed Nov 26 17:29:54 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Thu Nov 20 19:47:37 2008 -0200 +9 -0

+
+       /* we call v4l to free radio->videodev */
+       video_device_release(videodev);
+
+       /* free rest memory */
+       kfree(radio->buffer);
+       kfree(radio);
+
+       mutex_unlock(&radio->lock);

Again, wrong for the above reason but worse cause you are referencing freed memory.

Quoted 21 lines

linux/drivers/media/radio/radio-mr800.c Wed Nov 26 17:29:54 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Thu Nov 20 19:47:37 2008 -0200 +5 -2

+}
+
+
+
 /* 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,15 +662,12 @@
       radio->usbdev = interface_to_usbdev(intf);
       radio->curfreq = 95.16 * FREQ_MUL;
 
-       mutex_init(&radio->disconnect_lock);
       mutex_init(&radio->lock);
 

linux/drivers/media/radio/radio-mr800.c Wed Nov 26 17:29:54 2008 +0300 line 670

Quoted 7 lines

linux/drivers/media/radio/radio-mr800.c Wed Nov 26 17:29:54 2008 +0300 renamed from linux/drivers/media/radio/radio-mr800.c Thu Nov 20 19:47:37 2008 -0200 +1 -3

       video_set_drvdata(radio->videodev, radio);
       if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr)) {
               dev_warn(&intf->dev, "could not register video device\n");
-               video_device_release(radio->videodev);
-               kfree(radio->buffer);
-               kfree(radio);
+               video_unregister_device(radio->videodev);

This is wrong. When video_register_device fails, you should not call video_unregister_device. You should free any allocated memory and return an error code. The prior code here was correct.

Mauro, should i make patch for current hg-tree ?

Probably this thing should go to current upstream, because it will fix an oops. I start working to fix the same oops in dsbr100 module.

Hello, all

Patch adds new macros - videodev_to_radio. Make it sense ? Mutex lock added, a lot of safety checks. "struct video_device videodev" is embedded in dsbr100_device structure that works up in addition many address of operands. Memcpy removed.

Please, check if have free time..

---
diff -r 6bab82c6096e linux/drivers/media/radio/dsbr100.c

linux/drivers/media/radio/dsbr100.c Tue Dec 09 04:23:43 2008 +0300 renamed from linux/drivers/media/radio/dsbr100.c Wed Dec 03 15:32:11 2008 -0200 +4 -1

@@ -146,6 +146,7 @@
 #define FREQ_MAX 108.0
 #define FREQ_MUL 16000
 
+#define videodev_to_radio(d) container_of(d, struct dsbr100_device, videodev)
 
 static int usb_dsbr100_probe(struct usb_interface *intf,
                            const struct usb_device_id *id);

@@ -162,8 +163,9 @@
 /* Data for one (physical) device */
 struct dsbr100_device {
       struct usb_device *usbdev;
-       struct video_device *videodev;
+       struct video_device videodev;
       u8 *transfer_buffer;
+       struct mutex lock;      /* buffer locking */
       int curfreq;
       int stereo;
       int users;

@@ -198,6 +200,7 @@
 /* switch on radio */
 static int dsbr100_start(struct dsbr100_device *radio)
 {
+       mutex_lock(&radio->lock);
       if (usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
                       USB_REQ_GET_STATUS,
                       USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,

@@ -205,8 +208,12 @@
       usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),

linux/drivers/media/radio/dsbr100.c Tue Dec 09 04:23:43 2008 +0300 renamed from linux/drivers/media/radio/dsbr100.c Wed Dec 03 15:32:11 2008 -0200 +1 -0

       return (radio->transfer_buffer)[0];
 }

@@ -215,6 +222,7 @@
 /* switch off radio */
 static int dsbr100_stop(struct dsbr100_device *radio)
 {
+       mutex_lock(&radio->lock);
       if (usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
                       USB_REQ_GET_STATUS,
                       USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,

@@ -222,8 +230,12 @@
       usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),

linux/drivers/media/radio/dsbr100.c Tue Dec 09 04:23:43 2008 +0300 renamed from linux/drivers/media/radio/dsbr100.c Wed Dec 03 15:32:11 2008 -0200 +62 -27

       return (radio->transfer_buffer)[0];
 }

@@ -232,6 +244,7 @@
 static int dsbr100_setfreq(struct dsbr100_device *radio, int freq)
 {
       freq = (freq / 16 * 80) / 1000 + 856;
+       mutex_lock(&radio->lock);
       if (usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
                       DSB100_TUNE,
                       USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,

@@ -246,9 +259,12 @@
                       USB_TYPE_VENDOR | USB_RECIP_DEVICE |  USB_DIR_IN,
                       0x00, 0x24, radio->transfer_buffer, 8, 300) < 0) {
               radio->stereo = -1;
+               mutex_unlock(&radio->lock);
               return -1;
       }
+
       radio->stereo = !((radio->transfer_buffer)[0] & 0x01);
+       mutex_unlock(&radio->lock);
       return (radio->transfer_buffer)[0];
 }
 

@@ -256,6 +272,7 @@
 sees a stereo signal or not.  Pity. */
 static void dsbr100_getstat(struct dsbr100_device *radio)
 {
+       mutex_lock(&radio->lock);
       if (usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
               USB_REQ_GET_STATUS,
               USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,

@@ -263,6 +280,7 @@
               radio->stereo = -1;
       else
               radio->stereo = !(radio->transfer_buffer[0] & 0x01);
+       mutex_unlock(&radio->lock);
 }
 
 

@@ -277,16 +295,12 @@
       struct dsbr100_device *radio = usb_get_intfdata(intf);
 
       usb_set_intfdata (intf, NULL);
-       if (radio) {
-               video_unregister_device(radio->videodev);
-               radio->videodev = NULL;
-               if (radio->users) {
-                       kfree(radio->transfer_buffer);
-                       kfree(radio);
-               } else {
-                       radio->removed = 1;
-               }
-       }
+
+       mutex_lock(&radio->lock);
+       radio->removed = 1;
+       mutex_unlock(&radio->lock);
+
+       video_unregister_device(&radio->videodev);
 }
 
 

@@ -305,6 +319,10 @@
                               struct v4l2_tuner *v)
 {
       struct dsbr100_device *radio = video_drvdata(file);
+
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
 
       if (v->index > 0)
               return -EINVAL;

@@ -327,6 +345,12 @@
 static int vidioc_s_tuner(struct file *file, void *priv,
                               struct v4l2_tuner *v)
 {
+       struct dsbr100_device *radio = video_drvdata(file);
+
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
+
       if (v->index > 0)
               return -EINVAL;
 

@@ -338,6 +362,10 @@
 {
       struct dsbr100_device *radio = video_drvdata(file);
 
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
+
       radio->curfreq = f->frequency;
       if (dsbr100_setfreq(radio, radio->curfreq) == -1)
               dev_warn(&radio->usbdev->dev, "Set frequency failed\n");

@@ -348,6 +376,10 @@
                               struct v4l2_frequency *f)
 {
       struct dsbr100_device *radio = video_drvdata(file);
+
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
 
       f->type = V4L2_TUNER_RADIO;
       f->frequency = radio->curfreq;

@@ -373,6 +405,10 @@
 {
       struct dsbr100_device *radio = video_drvdata(file);
 
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
+
       switch (ctrl->id) {
       case V4L2_CID_AUDIO_MUTE:
               ctrl->value = radio->muted;

@@ -385,6 +421,10 @@
                               struct v4l2_control *ctrl)
 {
       struct dsbr100_device *radio = video_drvdata(file);
+
+       /* safety check */
+       if (radio->removed)
+               return -EIO;
 
       switch (ctrl->id) {
       case V4L2_CID_AUDIO_MUTE:

@@ -467,13 +507,19 @@
 static int usb_dsbr100_close(struct inode *inode, struct file *file)
 {
       struct dsbr100_device *radio = video_drvdata(file);
+       int retval;
 
       if (!radio)
               return -ENODEV;
+
       radio->users = 0;
-       if (radio->removed) {
-               kfree(radio->transfer_buffer);
-               kfree(radio);
+       if (!radio->removed) {
+               retval = dsbr100_stop(radio);
+               if (retval == -1) {
+                       dev_warn(&radio->usbdev->dev,
+                               "dsbr100_stop failed\n");
+               }
+
       }
       return 0;
 }

@@ -508,6 +554,14 @@
       return 0;
 }
 
+static void usb_dsbr100_video_device_release(struct video_device *videodev)
+{
+       struct dsbr100_device *radio = videodev_to_radio(videodev);
+
+       kfree(radio->transfer_buffer);
+       kfree(radio);
+}
+
 /* File system interface */
 static const struct file_operations usb_dsbr100_fops = {
       .owner          = THIS_MODULE,

@@ -536,11 +590,11 @@
 };
 
 /* V4L2 interface */
-static struct video_device dsbr100_videodev_template = {
+static struct video_device dsbr100_videodev_data = {
       .name           = "D-Link DSB-R 100",
       .fops           = &usb_dsbr100_fops,
       .ioctl_ops      = &usb_dsbr100_ioctl_ops,
-       .release        = video_device_release,
+       .release        = usb_dsbr100_video_device_release,
 };
 
 /* check if the device is present and register with v4l and

@@ -561,23 +615,17 @@
               kfree(radio);
               return -ENOMEM;
       }
-       radio->videodev = video_device_alloc();
 
-       if (!(radio->videodev)) {
-               kfree(radio->transfer_buffer);
-               kfree(radio);
-               return -ENOMEM;
-       }
-       memcpy(radio->videodev, &dsbr100_videodev_template,
-               sizeof(dsbr100_videodev_template));
+       mutex_init(&radio->lock);
+       radio->videodev = dsbr100_videodev_data;
+
       radio->removed = 0;
       radio->users = 0;
       radio->usbdev = interface_to_usbdev(intf);
       radio->curfreq = FREQ_MIN * FREQ_MUL;
-       video_set_drvdata(radio->videodev, radio);
-       if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr) < 0) {
+       video_set_drvdata(&radio->videodev, radio);
+       if (video_register_device(&radio->videodev, VFL_TYPE_RADIO, radio_nr) < 0) {
               dev_warn(&intf->dev, "Could not register video device\n");
-               video_device_release(radio->videodev);
               kfree(radio->transfer_buffer);
               kfree(radio);
               return -EIO;
 
 
--

Best regards, Klimov Alexey

This patch corrects unplug procedure. Patch adds usb_dsbr100_video_device_release, new macros - videodev_to_radio, mutex lock and a lot of safety checks.
Struct video_device videodev is embedded in dsbr100_device structure. Video_device_alloc and memcpy calls removed.

Signed-off-by Alexey Klimov <[email protected]>