[PATCH 1/1] radio-mr800: fix unplug

[PATCH 1/1] radio-mr800: fix unplug

[Alexey Klimov]

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 <klimov.linux@gmail.com>

---

diff -r 1536e16ffdf1 linux/drivers/media/radio/radio-mr800.c
--- a/linux/drivers/media/radio/radio-mr800.c	Tue Nov 18 15:51:08 2008 -0200
+++ b/linux/drivers/media/radio/radio-mr800.c	Wed Nov 19 03:27:59 2008 +0300
@@ -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);
 
@@ -296,18 +305,16 @@
 {
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
+	mutex_lock(&radio->disconnect_lock);
+	radio->removed = 1;
 	usb_set_intfdata(intf, NULL);
 
-	if (radio) {
+	if (radio->users == 0) {
 		video_unregister_device(radio->videodev);
-		radio->videodev = NULL;
-		if (radio->users) {
-			kfree(radio->buffer);
-			kfree(radio);
-		} else {
-			radio->removed = 1;
-		}
+		kfree(radio->buffer);
+		kfree(radio);
 	}
+	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)
 {
 	struct amradio_device *radio = video_get_drvdata(video_devdata(file));
+	int retval;
 
 	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 {
+		retval = amradio_stop(radio);
+		if (retval < 0)
+			amradio_dev_warn(&radio->videodev->dev,
+				"amradio_stop failed\n");
 	}
+
+	mutex_unlock(&radio->disconnect_lock);
 	return 0;
 }
 
@@ -610,6 +655,7 @@
 	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

Re: [PATCH 1/1] radio-mr800: fix unplug

[David Ellingsworth]

NACK

On Tue, Nov 18, 2008 at 7:36 PM, Alexey Klimov <klimov.linux@gmail.com> wrote:
>
> 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 <klimov.linux@gmail.com>
>
> ---
>
> diff -r 1536e16ffdf1 linux/drivers/media/radio/radio-mr800.c
> --- a/linux/drivers/media/radio/radio-mr800.c   Tue Nov 18 15:51:08 2008 -0200
> +++ b/linux/drivers/media/radio/radio-mr800.c   Wed Nov 19 03:27:59 2008 +0300
> @@ -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);
>
> @@ -296,18 +305,16 @@
>  {
>        struct amradio_device *radio = usb_get_intfdata(intf);
>
> +       mutex_lock(&radio->disconnect_lock);
> +       radio->removed = 1;
>        usb_set_intfdata(intf, NULL);
>
> -       if (radio) {
> +       if (radio->users == 0) {
>                video_unregister_device(radio->videodev);

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

> -               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.

>        }
> +       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)
>  {
>        struct amradio_device *radio = video_get_drvdata(video_devdata(file));
> +       int retval;
>
>        if (!radio)
>                return -ENODEV;
> +
> +       mutex_lock(&radio->disconnect_lock);
>        radio->users = 0;
>        if (radio->removed) {
> +               video_unregister_device(radio->videodev);

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

>                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.

> +
> +       } 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;
>  }
>
> @@ -610,6 +655,7 @@
>        radio->usbdev = interface_to_usbdev(intf);
>        radio->curfreq = 95.16 * FREQ_MUL;
>
> +       mutex_init(&radio->disconnect_lock);

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

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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 1/1] radio-mr800: fix unplug

[Alexey Klimov]

Hello, David

On Thu, 2008-11-20 at 10:53 -0500, David Ellingsworth wrote:
> NACK

> video_unregister_device should _always_ be called once the device is
> disconnect, no matter how many handles are still open.
> 
> > -               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.

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.

> Again, video_unregister_device should always be called from the usb
> disconnect callback.
> 
> >                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);
}

> 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.


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

Re: [PATCH 1/1] radio-mr800: fix unplug

[David Ellingsworth]

On Sat, Nov 22, 2008 at 10:19 PM, Alexey Klimov <klimov.linux@gmail.com> wrote:
> Hello, David
>
> On Thu, 2008-11-20 at 10:53 -0500, David Ellingsworth wrote:
>> NACK
>
>> video_unregister_device should _always_ be called once the device is
>> disconnect, no matter how many handles are still open.
>>
>> > -               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.
>
> 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.

>
>> Again, video_unregister_device should always be called from the usb
>> disconnect callback.
>>
>> >                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 ?

Yes, this looks correct.

>
> 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.

>
> 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.

>
> 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);
> }
>
>> 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.

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

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 1/1] radio-mr800: fix unplug

[Alexey Klimov]

Hello, David

On Mon, 2008-11-24 at 09:35 -0500, David Ellingsworth wrote:
> On Sat, Nov 22, 2008 at 10:19 PM, Alexey Klimov <klimov.linux@gmail.com> wrote:
> > Hello, David
> >
> > On Thu, 2008-11-20 at 10:53 -0500, David Ellingsworth wrote:
> >> NACK
> >
> >> video_unregister_device should _always_ be called once the device is
> >> disconnect, no matter how many handles are still open.
> >>
> >> > -               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.
> >
> > 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.
> 
> >
> >> Again, video_unregister_device should always be called from the usb
> >> disconnect callback.
> >>
> >> >                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 ?
> 
> Yes, this looks correct.
> 
> >
> > 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.
> 
> >
> > 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.
> 
> >
> > 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);
> > }
> >
> >> 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.

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
--- a/linux/drivers/media/radio/radio-mr800.c	Thu Nov 20 19:47:37 2008 -0200
+++ b/linux/drivers/media/radio/radio-mr800.c	Wed Nov 26 17:29:54 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,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);
+
+	/* we call v4l to free radio->videodev */
+	video_device_release(videodev);
+
+	/* free rest memory */
+	kfree(radio->buffer);
+	kfree(radio);
+
+	mutex_unlock(&radio->lock);
+}
+
+
+
 /* 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);
 
 	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);
 		return -EIO;
 	}
 

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

Re: [PATCH 1/1] radio-mr800: fix unplug

[David Ellingsworth]

On Thu, Nov 27, 2008 at 7:00 AM, Alexey Klimov <klimov.linux@gmail.com> wrote:
> 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 ?

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

> 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
> --- a/linux/drivers/media/radio/radio-mr800.c   Thu Nov 20 19:47:37 2008 -0200
> +++ b/linux/drivers/media/radio/radio-mr800.c   Wed Nov 26 17:29:54 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,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.

> +
> +       /* 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.

> +}
> +
> +
> +
>  /* 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);
>
>        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.

>                return -EIO;
>        }
>

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 1/1] radio-mr800: fix unplug

[Alexey Klimov]

Hello, all

On Sun, Nov 30, 2008 at 8:19 AM, David Ellingsworth
<david@identd.dyndns.org> wrote:
>> 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.
>> 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   Wed Nov 26 17:29:54 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,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.
>
>> +
>> +       /* 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.

Yes, you are right!
I removed them.

>> +}
>> +
>> +
>> +
>>  /* 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);
>>
>>        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.

Okay, i didn't know about when disconnect-function called, so i asked.
I return former code.

Thanks for reviewing again. It's really help me.

If there is no more mistakes..

Mauro, should i make patch for current hg-tree ? Or you want revert
previous patch ?
What is the options here?
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.

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

Re: [PATCH 1/1] radio-mr800: fix unplug

[Mauro Carvalho Chehab]

On Sat, 6 Dec 2008 03:04:49 +0300
"Alexey Klimov" <klimov.linux@gmail.com> wrote:


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

Yes.

> 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.

So, you need to hurry! We are already at -rc7.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[please review patch] dsbr100: fix unplug oops

[Alexey Klimov]

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
--- a/linux/drivers/media/radio/dsbr100.c	Wed Dec 03 15:32:11 2008 -0200
+++ b/linux/drivers/media/radio/dsbr100.c	Tue Dec 09 04:23:43 2008 +0300
@@ -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),
 			DSB100_ONOFF,
 			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-			0x01, 0x00, radio->transfer_buffer, 8, 300) < 0)
+			0x01, 0x00, radio->transfer_buffer, 8, 300) < 0) {
+		mutex_unlock(&radio->lock);
 		return -1;
+	}
+
+	mutex_unlock(&radio->lock);
 	radio->muted=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),
 			DSB100_ONOFF,
 			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-			0x00, 0x00, radio->transfer_buffer, 8, 300) < 0)
+			0x00, 0x00, radio->transfer_buffer, 8, 300) < 0) {
+		mutex_unlock(&radio->lock);
 		return -1;
+	}
+
+	mutex_unlock(&radio->lock);
 	radio->muted=1;
 	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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [please review patch] dsbr100: fix unplug oops

[David Ellingsworth]

On Mon, Dec 8, 2008 at 8:35 PM, Alexey Klimov <klimov.linux@gmail.com> wrote:
> 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
> --- a/linux/drivers/media/radio/dsbr100.c       Wed Dec 03 15:32:11 2008 -0200
> +++ b/linux/drivers/media/radio/dsbr100.c       Tue Dec 09 04:23:43 2008 +0300
> @@ -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),
>                        DSB100_ONOFF,
>                        USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> -                       0x01, 0x00, radio->transfer_buffer, 8, 300) < 0)
> +                       0x01, 0x00, radio->transfer_buffer, 8, 300) < 0) {
> +               mutex_unlock(&radio->lock);
>                return -1;
> +       }
> +
> +       mutex_unlock(&radio->lock);
>        radio->muted=0;

The muted member should be set within the context of the lock.

>        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),
>                        DSB100_ONOFF,
>                        USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> -                       0x00, 0x00, radio->transfer_buffer, 8, 300) < 0)
> +                       0x00, 0x00, radio->transfer_buffer, 8, 300) < 0) {
> +               mutex_unlock(&radio->lock);
>                return -1;
> +       }
> +
> +       mutex_unlock(&radio->lock);
>        radio->muted=1;

The muted member should be set within the context of the lock.

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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH] dsbr100: fix unplug oops

[Alexey Klimov]

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 <klimov.linux@gmail.com>

---
diff -r 6bab82c6096e linux/drivers/media/radio/dsbr100.c
--- a/linux/drivers/media/radio/dsbr100.c	Wed Dec 03 15:32:11 2008 -0200
+++ b/linux/drivers/media/radio/dsbr100.c	Tue Dec 09 23:37:18 2008 +0300
@@ -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,9 +208,13 @@
 	usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
 			DSB100_ONOFF,
 			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-			0x01, 0x00, radio->transfer_buffer, 8, 300) < 0)
+			0x01, 0x00, radio->transfer_buffer, 8, 300) < 0) {
+		mutex_unlock(&radio->lock);
 		return -1;
+	}
+
 	radio->muted=0;
+	mutex_unlock(&radio->lock);
 	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,9 +230,13 @@
 	usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
 			DSB100_ONOFF,
 			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-			0x00, 0x00, radio->transfer_buffer, 8, 300) < 0)
+			0x00, 0x00, radio->transfer_buffer, 8, 300) < 0) {
+		mutex_unlock(&radio->lock);
 		return -1;
+	}
+
 	radio->muted=1;
+	mutex_unlock(&radio->lock);
 	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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] dsbr100: fix unplug oops

[Douglas Schilling Landgraf]

Hello Alexey,

On Tue, 09 Dec 2008 23:55:43 +0300
Alexey Klimov <klimov.linux@gmail.com> wrote:

> 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 <klimov.linux@gmail.com>
> 

  I've double check your patch and tested several times. Worked fine.
  Congrats to you and David.

  Acked-by: Douglas Schilling Landgraf <dougsland@linuxtv.org>

Thanks,
Douglas

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list