[PATCH] ov511.c: video_register_device() return zero on success

[PATCH] ov511.c: video_register_device() return zero on success

[Figo.zhang]

video_register_device() return zero on success, it would not return a positive integer.

Signed-off-by: Figo.zhang <figo1802@gmail.com>
--- 
 drivers/media/video/ov511.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/ov511.c b/drivers/media/video/ov511.c
index 9af5532..816427e 100644
--- a/drivers/media/video/ov511.c
+++ b/drivers/media/video/ov511.c
@@ -5851,7 +5851,7 @@ ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
 			break;
 
 		if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
-			unit_video[i]) >= 0) {
+			unit_video[i]) == 0) {
 			break;
 		}
 	}

Re: [PATCH] ov511.c: video_register_device() return zero on success

[Mauro Carvalho Chehab]

Em Sun, 31 May 2009 14:41:52 +0800
"Figo.zhang" <figo1802@gmail.com> escreveu:

> video_register_device() return zero on success, it would not return a positive integer.
> 
> Signed-off-by: Figo.zhang <figo1802@gmail.com>
> --- 
>  drivers/media/video/ov511.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/ov511.c b/drivers/media/video/ov511.c
> index 9af5532..816427e 100644
> --- a/drivers/media/video/ov511.c
> +++ b/drivers/media/video/ov511.c
> @@ -5851,7 +5851,7 @@ ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  			break;
>  
>  		if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
> -			unit_video[i]) >= 0) {
> +			unit_video[i]) == 0) {
>  			break;
>  		}
>  	}

Nack.

Errors are always negative. So, any zero or positive value indicates that no error occurred.

Yet, the logic for forcing ov51x to specific minor number seems broken: it will
end by registering the device twice, if used.

So, that part of the function needs a rewrite. I'll fix it.

-- 

Cheers,
Mauro

Re: [PATCH] ov511.c: video_register_device() return zero on success

[Mauro Carvalho Chehab]

Em Wed, 10 Jun 2009 22:39:51 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:

> Em Sun, 31 May 2009 14:41:52 +0800
> "Figo.zhang" <figo1802@gmail.com> escreveu:
> 
> > video_register_device() return zero on success, it would not return a positive integer.
> > 
> > Signed-off-by: Figo.zhang <figo1802@gmail.com>
> > --- 
> >  drivers/media/video/ov511.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/media/video/ov511.c b/drivers/media/video/ov511.c
> > index 9af5532..816427e 100644
> > --- a/drivers/media/video/ov511.c
> > +++ b/drivers/media/video/ov511.c
> > @@ -5851,7 +5851,7 @@ ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
> >  			break;
> >  
> >  		if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
> > -			unit_video[i]) >= 0) {
> > +			unit_video[i]) == 0) {
> >  			break;
> >  		}
> >  	}
> 
> Nack.
> 
> Errors are always negative. So, any zero or positive value indicates that no error occurred.
> 
> Yet, the logic for forcing ov51x to specific minor number seems broken: it will
> end by registering the device twice, if used.
> 
> So, that part of the function needs a rewrite. I'll fix it.
> 

Hmm... the issue seems more complex than I've imagined...

When ov511 were written, it was assumed that video_register_device(dev, v, nr)
would have the following behavior:

	if nr = -1, it would do dynamic minor allocation;
	if nr >= 0, it would allocate to 'nr' minor or fail.

With the original behavior.

The ov511_probe registering loop is doing something like this (I did a cleanup
to allow a better understand of the logic):

<snip>
#define OV511_MAX_UNIT_VIDEO 16
...
static int unit_video[OV511_MAX_UNIT_VIDEO];
...
module_param_array(unit_video, int, &num_uv, 0);
MODULE_PARM_DESC(unit_video,
  "Force use of specific minor number(s). 0 is not allowed.");
...
static int
ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
{
...
        for (i = 0; i < OV511_MAX_UNIT_VIDEO; i++) {
                /* Minor 0 cannot be specified; assume user wants autodetect */
                if (unit_video[i] == 0)
                        break;

                if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
                                        unit_video[i]) >= 0)
                        goto register_succeded;
        }

        /* Use the next available one */
        if (video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
                err("video_register_device failed");
                goto error;

register_succeeded:
        dev_info(&intf->dev, "Device at %s registered to minor %d\n",
                 ov->usb_path, ov->vdev->minor);
</snip>

So, if you probe ov511 with a list of device numbers, for example:

modprobe ov511 4,1,3

And assuming that you have 5 ov511 devices, and connect they one by one,
they'll gain the following device numbers (at the connection order):
/dev/video4
/dev/video1
/dev/video3
/dev/video0
/dev/video2

However, changeset 9133:

changeset:   9133:64aed7485a43
parent:      9073:4db9722caf4f
user:        Hans Verkuil <hverkuil@xs4all.nl>
date:        Sat Oct 04 13:36:54 2008 +0200
summary:     v4l: disconnect kernel number from minor

Changed the behavior video_register_device(dev, v, nr):

+ nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);

So, instead of accepting just -1 or nr, it will now do:
	if nr = -1, it will do dynamic minor allocation (as before);
	if nr >= 0, it will also do dynamic minor allocation, but starting from nr.

So, the new code won't fail if nr is already allocated, but it will return the
next unallocated nr.

With the ov511 code, this means that you'll have, instead:

/dev/video5
/dev/video6
/dev/video7
/dev/video8
/dev/video9

So, changeset 9133 actually broke the ov511 probe original behavior.

In order to restore the original behavior, the probe logic should be replaced
by something else (like the approach taken by em28xx).

Also, changeset 9133 may also cause other side effects on other drivers that
were expecting the original behavior.

To make things worse, the function comment doesn't properly explain the current
behavior.

---

Figo,

Since we are in the middle of a merge window, it is unlikely that I'll have
enough time to properly fix it right now (since my priority right now is to
merge the existing patches).

As you started proposing a patch for it, maybe you could try to fix it and
check about similar troubles on other drivers.

Cheers,
Mauro

Re: [PATCH] ov511.c: video_register_device() return zero on success

[Hans Verkuil]

On Thursday 11 June 2009 06:40:14 Mauro Carvalho Chehab wrote:
> Em Wed, 10 Jun 2009 22:39:51 -0300
> Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:
> 
> > Em Sun, 31 May 2009 14:41:52 +0800
> > "Figo.zhang" <figo1802@gmail.com> escreveu:
> > 
> > > video_register_device() return zero on success, it would not return a positive integer.
> > > 
> > > Signed-off-by: Figo.zhang <figo1802@gmail.com>
> > > --- 
> > >  drivers/media/video/ov511.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/ov511.c b/drivers/media/video/ov511.c
> > > index 9af5532..816427e 100644
> > > --- a/drivers/media/video/ov511.c
> > > +++ b/drivers/media/video/ov511.c
> > > @@ -5851,7 +5851,7 @@ ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > >  			break;
> > >  
> > >  		if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
> > > -			unit_video[i]) >= 0) {
> > > +			unit_video[i]) == 0) {
> > >  			break;
> > >  		}
> > >  	}
> > 
> > Nack.
> > 
> > Errors are always negative. So, any zero or positive value indicates that no error occurred.
> > 
> > Yet, the logic for forcing ov51x to specific minor number seems broken: it will
> > end by registering the device twice, if used.
> > 
> > So, that part of the function needs a rewrite. I'll fix it.
> > 
> 
> Hmm... the issue seems more complex than I've imagined...
> 
> When ov511 were written, it was assumed that video_register_device(dev, v, nr)
> would have the following behavior:
> 
> 	if nr = -1, it would do dynamic minor allocation;
> 	if nr >= 0, it would allocate to 'nr' minor or fail.
> 
> With the original behavior.
> 
> The ov511_probe registering loop is doing something like this (I did a cleanup
> to allow a better understand of the logic):
> 
> <snip>
> #define OV511_MAX_UNIT_VIDEO 16
> ...
> static int unit_video[OV511_MAX_UNIT_VIDEO];
> ...
> module_param_array(unit_video, int, &num_uv, 0);
> MODULE_PARM_DESC(unit_video,
>   "Force use of specific minor number(s). 0 is not allowed.");
> ...
> static int
> ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
> {
> ...
>         for (i = 0; i < OV511_MAX_UNIT_VIDEO; i++) {
>                 /* Minor 0 cannot be specified; assume user wants autodetect */
>                 if (unit_video[i] == 0)
>                         break;
> 
>                 if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
>                                         unit_video[i]) >= 0)
>                         goto register_succeded;
>         }
> 
>         /* Use the next available one */
>         if (video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
>                 err("video_register_device failed");
>                 goto error;
> 
> register_succeeded:
>         dev_info(&intf->dev, "Device at %s registered to minor %d\n",
>                  ov->usb_path, ov->vdev->minor);
> </snip>
> 
> So, if you probe ov511 with a list of device numbers, for example:
> 
> modprobe ov511 4,1,3
> 
> And assuming that you have 5 ov511 devices, and connect they one by one,
> they'll gain the following device numbers (at the connection order):
> /dev/video4
> /dev/video1
> /dev/video3
> /dev/video0
> /dev/video2
> 
> However, changeset 9133:
> 
> changeset:   9133:64aed7485a43
> parent:      9073:4db9722caf4f
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Sat Oct 04 13:36:54 2008 +0200
> summary:     v4l: disconnect kernel number from minor
> 
> Changed the behavior video_register_device(dev, v, nr):
> 
> + nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);
> 
> So, instead of accepting just -1 or nr, it will now do:
> 	if nr = -1, it will do dynamic minor allocation (as before);
> 	if nr >= 0, it will also do dynamic minor allocation, but starting from nr.
> 
> So, the new code won't fail if nr is already allocated, but it will return the
> next unallocated nr.
> 
> With the ov511 code, this means that you'll have, instead:
> 
> /dev/video5
> /dev/video6
> /dev/video7
> /dev/video8
> /dev/video9
> 
> So, changeset 9133 actually broke the ov511 probe original behavior.
> 
> In order to restore the original behavior, the probe logic should be replaced
> by something else (like the approach taken by em28xx).
> 
> Also, changeset 9133 may also cause other side effects on other drivers that
> were expecting the original behavior.
> 
> To make things worse, the function comment doesn't properly explain the current
> behavior.
> 
> ---
> 
> Figo,
> 
> Since we are in the middle of a merge window, it is unlikely that I'll have
> enough time to properly fix it right now (since my priority right now is to
> merge the existing patches).
> 
> As you started proposing a patch for it, maybe you could try to fix it and
> check about similar troubles on other drivers.
> 
> Cheers,
> Mauro
> 

Since I made that change I'm willing to look at this. Some comments definitely
need improving at the least. Also ivtv and cx18 rely on the current behavior,
so any changes need to be done carefully.

But one question first: isn't the current approach not better anyway than the
old approach? In the past device creation would fail if you specified an
explicit device kernel number that was already in use. Now it will attempt
to fulfill the request and skip to the next free number otherwise. Seems a
pretty good approach to me.

There haven't been any complaints about this (probably also because nobody is
using it).

Let me know what you want and I can implement it. It's not that hard.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCH] ov511.c: video_register_device() return zero on success

[Figo.zhang]

On Thu, 2009-06-11 at 01:40 -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 10 Jun 2009 22:39:51 -0300
> Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:
> 
> > Em Sun, 31 May 2009 14:41:52 +0800
> > "Figo.zhang" <figo1802@gmail.com> escreveu:
> > 
> > > video_register_device() return zero on success, it would not return a positive integer.
> > > 
> > > Signed-off-by: Figo.zhang <figo1802@gmail.com>
> > > --- 
> > >  drivers/media/video/ov511.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/ov511.c b/drivers/media/video/ov511.c
> > > index 9af5532..816427e 100644
> > > --- a/drivers/media/video/ov511.c
> > > +++ b/drivers/media/video/ov511.c
> > > @@ -5851,7 +5851,7 @@ ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > >  			break;
> > >  
> > >  		if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
> > > -			unit_video[i]) >= 0) {
> > > +			unit_video[i]) == 0) {
> > >  			break;
> > >  		}
> > >  	}
> > 
> > Nack.
> > 
> > Errors are always negative. So, any zero or positive value indicates that no error occurred.
> > 
> > Yet, the logic for forcing ov51x to specific minor number seems broken: it will
> > end by registering the device twice, if used.
> > 
> > So, that part of the function needs a rewrite. I'll fix it.
> > 
> 
> Hmm... the issue seems more complex than I've imagined...
> 
> When ov511 were written, it was assumed that video_register_device(dev, v, nr)
> would have the following behavior:
> 
> 	if nr = -1, it would do dynamic minor allocation;
> 	if nr >= 0, it would allocate to 'nr' minor or fail.
> 
> With the original behavior.
> 
> The ov511_probe registering loop is doing something like this (I did a cleanup
> to allow a better understand of the logic):
> 
> <snip>
> #define OV511_MAX_UNIT_VIDEO 16
> ...
> static int unit_video[OV511_MAX_UNIT_VIDEO];
> ...
> module_param_array(unit_video, int, &num_uv, 0);
> MODULE_PARM_DESC(unit_video,
>   "Force use of specific minor number(s). 0 is not allowed.");
> ...
> static int
> ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
> {
> ...
>         for (i = 0; i < OV511_MAX_UNIT_VIDEO; i++) {
>                 /* Minor 0 cannot be specified; assume user wants autodetect */
>                 if (unit_video[i] == 0)
>                         break;
> 
>                 if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
>                                         unit_video[i]) >= 0)
>                         goto register_succeded;
>         }
> 
>         /* Use the next available one */
>         if (video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
>                 err("video_register_device failed");
>                 goto error;
> 
> register_succeeded:
>         dev_info(&intf->dev, "Device at %s registered to minor %d\n",
>                  ov->usb_path, ov->vdev->minor);
> </snip>
> 
> So, if you probe ov511 with a list of device numbers, for example:
> 
> modprobe ov511 4,1,3
> 

you means specify the minior video
device: /dev/vidoe4,/dev/video1, /dev/video3 ? 

it maybe : modprobe ov511 unit_video=4,1,3
  
> And assuming that you have 5 ov511 devices, and connect they one by one,
> they'll gain the following device numbers (at the connection order):
> /dev/video4
> /dev/video1
> /dev/video3
> /dev/video0
> /dev/video2
> 
> However, changeset 9133:
> 
> changeset:   9133:64aed7485a43
> parent:      9073:4db9722caf4f
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Sat Oct 04 13:36:54 2008 +0200
> summary:     v4l: disconnect kernel number from minor
> 
> Changed the behavior video_register_device(dev, v, nr):
> 
> + nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);
> 
> So, instead of accepting just -1 or nr, it will now do:
> 	if nr = -1, it will do dynamic minor allocation (as before);
> 	if nr >= 0, it will also do dynamic minor allocation, but starting from nr.
> 
> So, the new code won't fail if nr is already allocated, but it will return the
> next unallocated nr.
> 
> With the ov511 code, this means that you'll have, instead:
> 
> /dev/video5
> /dev/video6
> /dev/video7
> /dev/video8
> /dev/video9
> 
> So, changeset 9133 actually broke the ov511 probe original behavior.
> 
> In order to restore the original behavior, the probe logic should be replaced
> by something else (like the approach taken by em28xx).
> 
> Also, changeset 9133 may also cause other side effects on other drivers that
> were expecting the original behavior.
> 
> To make things worse, the function comment doesn't properly explain the current
> behavior.
> 
> ---
> 
> Figo,
> 
> Since we are in the middle of a merge window, it is unlikely that I'll have
> enough time to properly fix it right now (since my priority right now is to
> merge the existing patches).
> 
> As you started proposing a patch for it, maybe you could try to fix it and
> check about similar troubles on other drivers.
> 
> Cheers,
> Mauro


in v2, if insmod without specify 'unit_video', it use autodetect video device.
if specify the 'unit_video', it will try to detect start from nr.

Signed-off-by: Figo.zhang <figo1802@gmail.com>
--- 
 drivers/media/video/ov511.c |   32 +++++++++++++++++---------------
 1 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/media/video/ov511.c b/drivers/media/video/ov511.c
index 9af5532..293695f 100644
--- a/drivers/media/video/ov511.c
+++ b/drivers/media/video/ov511.c
@@ -180,7 +180,7 @@ module_param(force_palette, int, 0);
 MODULE_PARM_DESC(force_palette, "Force the palette to a specific value");
 module_param(backlight, int, 0);
 MODULE_PARM_DESC(backlight, "For objects that are lit from behind");
-static unsigned int num_uv;
+static unsigned int num_uv = 0;
 module_param_array(unit_video, int, &num_uv, 0);
 MODULE_PARM_DESC(unit_video,
   "Force use of specific minor number(s). 0 is not allowed.");
@@ -5845,22 +5845,24 @@ ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	ov->vdev->parent = &intf->dev;
 	video_set_drvdata(ov->vdev, ov);
 
-	for (i = 0; i < OV511_MAX_UNIT_VIDEO; i++) {
-		/* Minor 0 cannot be specified; assume user wants autodetect */
-		if (unit_video[i] == 0)
-			break;
-
-		if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
-			unit_video[i]) >= 0) {
-			break;
+	/*if num_uv is zero, it autodetect*/
+	if(num_uv == 0){
+		if ((ov->vdev->minor == -1) &&
+	    		video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
+			err("video_register_device failed");
+			goto error;
 		}
-	}
+	}else {
+		for (i = 0; i < num_uv; i++) {
+			/* Minor 0 cannot be specified; assume user wants autodetect */
+			if (unit_video[i] == 0)
+				break;
 
-	/* Use the next available one */
-	if ((ov->vdev->minor == -1) &&
-	    video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
-		err("video_register_device failed");
-		goto error;
+			if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
+				unit_video[i]) == 0) {
+				break;
+			}
+		}
 	}
 
 	dev_info(&intf->dev, "Device at %s registered to minor %d\n",

Re: [PATCH] ov511.c: video_register_device() return zero on success

[Mauro Carvalho Chehab]

Em Thu, 11 Jun 2009 08:36:00 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:


> Since I made that change I'm willing to look at this. Some comments definitely
> need improving at the least.

Thanks! Since the behavior changed, it is important to better document it.

> Also ivtv and cx18 rely on the current behavior,
> so any changes need to be done carefully.
> 
> But one question first: isn't the current approach not better anyway than the
> old approach? In the past device creation would fail if you specified an
> explicit device kernel number that was already in use. Now it will attempt
> to fulfill the request and skip to the next free number otherwise. Seems a
> pretty good approach to me.

Well, the idea of not failing due to that is interesting. Yet, those force
parameters are provided to avoid that a different modprobe order would change
the device nodes. By doing something different than requested by the users without
even warning them about that doesn't sound nice. At least a KERN_ERR log
should be printed if the selected nr is different than the requested one.

In the specific case of ov511, however, it caused a regression, since the used
logic to get the next value of the array were based on the failure of
video_register_device(). As it doesn't fail anymore, the current logic is broken.

> There haven't been any complaints about this (probably also because nobody is
> using it).
> 
> Let me know what you want and I can implement it. It's not that hard.

IMO, what should be done:

1) better comment the function;
2) generate a KERN_ERR at v4l2-dev, if the requested nr is not available;
3) replace ov511 logic to restore the old behavior (or improve it a little bit);
4) double check if similar regressions are present on other drivers. Since
ov511 is an old driver, I don't doubt that its logic is duplicated on other
devices.

Since ov511 is used for an usb device, extra care should be taken, since it
should be considered the possibility of successive hot plug/unplug. I wrote an
interesting logic for this at em28xx driver, that can be used as an alternative
to the current logic



Cheers,
Mauro

Re: [PATCH] ov511.c: video_register_device() return zero on success

[Hans Verkuil]

> Em Thu, 11 Jun 2009 08:36:00 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>
>> Since I made that change I'm willing to look at this. Some comments
>> definitely
>> need improving at the least.
>
> Thanks! Since the behavior changed, it is important to better document it.
>
>> Also ivtv and cx18 rely on the current behavior,
>> so any changes need to be done carefully.
>>
>> But one question first: isn't the current approach not better anyway
>> than the
>> old approach? In the past device creation would fail if you specified an
>> explicit device kernel number that was already in use. Now it will
>> attempt
>> to fulfill the request and skip to the next free number otherwise. Seems
>> a
>> pretty good approach to me.
>
> Well, the idea of not failing due to that is interesting. Yet, those force
> parameters are provided to avoid that a different modprobe order would
> change
> the device nodes. By doing something different than requested by the users
> without
> even warning them about that doesn't sound nice. At least a KERN_ERR log
> should be printed if the selected nr is different than the requested one.

KERN_WARNING would be better, I think. It is not an error after all.

> In the specific case of ov511, however, it caused a regression, since the
> used
> logic to get the next value of the array were based on the failure of
> video_register_device(). As it doesn't fail anymore, the current logic is
> broken.
>
>> There haven't been any complaints about this (probably also because
>> nobody is
>> using it).
>>
>> Let me know what you want and I can implement it. It's not that hard.
>
> IMO, what should be done:
>
> 1) better comment the function;
> 2) generate a KERN_ERR at v4l2-dev, if the requested nr is not available;
> 3) replace ov511 logic to restore the old behavior (or improve it a little
> bit);
> 4) double check if similar regressions are present on other drivers. Since
> ov511 is an old driver, I don't doubt that its logic is duplicated on
> other
> devices.

I've just double checked all video_register_device calls and ov511.c is
the only one with this behavior.

Regards,

        Hans

>
> Since ov511 is used for an usb device, extra care should be taken, since
> it
> should be considered the possibility of successive hot plug/unplug. I
> wrote an
> interesting logic for this at em28xx driver, that can be used as an
> alternative
> to the current logic



-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH] ov511.c: video_register_device() return zero on success

[Mauro Carvalho Chehab]

Em Thu, 11 Jun 2009 14:39:56 +0800
"Figo.zhang" <figo1802@gmail.com> escreveu:

> in v2, if insmod without specify 'unit_video', it use autodetect video device.
> if specify the 'unit_video', it will try to detect start from nr.
> 
> Signed-off-by: Figo.zhang <figo1802@gmail.com>
> --- 
>  drivers/media/video/ov511.c |   32 +++++++++++++++++---------------
>  1 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/video/ov511.c b/drivers/media/video/ov511.c
> index 9af5532..293695f 100644
> --- a/drivers/media/video/ov511.c
> +++ b/drivers/media/video/ov511.c
> @@ -180,7 +180,7 @@ module_param(force_palette, int, 0);
>  MODULE_PARM_DESC(force_palette, "Force the palette to a specific value");
>  module_param(backlight, int, 0);
>  MODULE_PARM_DESC(backlight, "For objects that are lit from behind");
> -static unsigned int num_uv;
> +static unsigned int num_uv = 0;

Never initialize a static var with 0. This just increases the module size. On
Linux, all static vars already initialized with 0.

>  module_param_array(unit_video, int, &num_uv, 0);
>  MODULE_PARM_DESC(unit_video,
>    "Force use of specific minor number(s). 0 is not allowed.");
> @@ -5845,22 +5845,24 @@ ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	ov->vdev->parent = &intf->dev;
>  	video_set_drvdata(ov->vdev, ov);
>  
> -	for (i = 0; i < OV511_MAX_UNIT_VIDEO; i++) {
> -		/* Minor 0 cannot be specified; assume user wants autodetect */
> -		if (unit_video[i] == 0)
> -			break;
> -
> -		if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
> -			unit_video[i]) >= 0) {
> -			break;
> +	/*if num_uv is zero, it autodetect*/
> +	if(num_uv == 0){
> +		if ((ov->vdev->minor == -1) &&
> +	    		video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
> +			err("video_register_device failed");
> +			goto error;
>  		}
> -	}
> +	}else {
> +		for (i = 0; i < num_uv; i++) {
> +			/* Minor 0 cannot be specified; assume user wants autodetect */
> +			if (unit_video[i] == 0)
> +				break;
>  
> -	/* Use the next available one */
> -	if ((ov->vdev->minor == -1) &&
> -	    video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
> -		err("video_register_device failed");
> -		goto error;
> +			if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
> +				unit_video[i]) == 0) {
> +				break;
> +			}
> +		}
>  	}
>  
>  	dev_info(&intf->dev, "Device at %s registered to minor %d\n",
> 
> 

Thanks for the trial, but it is still broken (and also have lots of coding
style errors - Please always check your patches with checkpatch.pl).

Due to the hot plug/hot unplug nature of usb devices, the logic for it should
be more complex, since it should need to control the removal/reinsertion of a
device.

Since Douglas helped me with patchwork stuff (thanks, Douglas!), I found some
time for fixing it. Since I don't have any ov511 camera here, it would be nice
if people with this camera can do a review.

---

ov511: Fix unit_video parameter behavior

Fix a regression caused by changeset 9133:64aed7485a43 - v4l: disconnect kernel number from minor

Before the above changeset, ov511_probe used to allow forcing to use a certain
specific set of video devices, like:

modprobe ov511 unit_video=4,1,3 num_uv=3

So, assuming that you have 5 ov511 devices, and connect they one by one,
they'll gain the following device numbers (at the connection order):
/dev/video4
/dev/video1
/dev/video3
/dev/video0
/dev/video2

However, this was changed due to this change at video_register_device():

+ nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);

With the previous behavior, a trial to register on an already allocated mirror
would fail, and a loop would get the next requested minor. However, the current
behavior is to get the next available minor instead of failing. Due to that,
this means that the above modprobe parameter will give, instead:

/dev/video5
/dev/video6
/dev/video7
/dev/video8
/dev/video9

In order to restore the original behavior, a static var were added, storing the
amount of already registered devices.

While there, it also fixes the locking of the probe/disconnect functions.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/linux/drivers/media/video/ov511.c b/linux/drivers/media/video/ov511.c
--- a/linux/drivers/media/video/ov511.c
+++ b/linux/drivers/media/video/ov511.c
@@ -112,6 +112,8 @@ static int framedrop		= -1;
 static int fastset;
 static int force_palette;
 static int backlight;
+/* Bitmask marking allocated devices from 0 to OV511_MAX_UNIT_VIDEO */
+static unsigned long ov511_devused;
 static int unit_video[OV511_MAX_UNIT_VIDEO];
 static int remove_zeros;
 static int mirror;
@@ -5724,7 +5726,7 @@ ov51x_probe(struct usb_interface *intf, 
 	struct usb_device *dev = interface_to_usbdev(intf);
 	struct usb_interface_descriptor *idesc;
 	struct usb_ov511 *ov;
-	int i;
+	int i, rc, nr;
 
 	PDEBUG(1, "probing for device...");
 
@@ -5849,23 +5851,27 @@ ov51x_probe(struct usb_interface *intf, 
 	ov->vdev->parent = &intf->dev;
 	video_set_drvdata(ov->vdev, ov);
 
-	for (i = 0; i < OV511_MAX_UNIT_VIDEO; i++) {
-		/* Minor 0 cannot be specified; assume user wants autodetect */
-		if (unit_video[i] == 0)
-			break;
-
-		if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
-			unit_video[i]) >= 0) {
-			break;
-		}
-	}
-
-	/* Use the next available one */
-	if ((ov->vdev->minor == -1) &&
-	    video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
+	mutex_lock(&ov->lock);
+
+	/* Check to see next free device and mark as used */
+	nr = find_first_zero_bit(&ov511_devused, OV511_MAX_UNIT_VIDEO);
+
+	/* Registers device */
+	if (unit_video[nr] != 0)
+		rc = video_register_device(ov->vdev, VFL_TYPE_GRABBER,
+					   unit_video[nr]);
+	else
+		rc = video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1);
+
+	if (rc < 0) {
 		err("video_register_device failed");
-		goto error;
-	}
+		mutex_unlock(&ov->lock);
+		goto error;
+	}
+
+	/* Mark device as used */
+	ov511_devused |= 1 << nr;
+	ov->nr = nr;
 
 	dev_info(&intf->dev, "Device at %s registered to minor %d\n",
 		 ov->usb_path, ov->vdev->minor);
@@ -5873,8 +5879,12 @@ ov51x_probe(struct usb_interface *intf, 
 	usb_set_intfdata(intf, ov);
 	if (ov_create_sysfs(ov->vdev)) {
 		err("ov_create_sysfs failed");
-		goto error;
-	}
+		ov511_devused &= ~(1 << nr);
+		mutex_unlock(&ov->lock);
+		goto error;
+	}
+
+	mutex_lock(&ov->lock);
 
 	return 0;
 
@@ -5910,10 +5920,16 @@ ov51x_disconnect(struct usb_interface *i
 
 	PDEBUG(3, "");
 
+	mutex_lock(&ov->lock);
 	usb_set_intfdata (intf, NULL);
 
-	if (!ov)
-		return;
+	if (!ov) {
+		mutex_unlock(&ov->lock);
+		return;
+	}
+
+	/* Free device number */
+	ov511_devused &= ~(1 << ov->nr);
 
 	if (ov->vdev)
 		video_unregister_device(ov->vdev);
@@ -5931,6 +5947,7 @@ ov51x_disconnect(struct usb_interface *i
 
 	ov->streaming = 0;
 	ov51x_unlink_isoc(ov);
+	mutex_unlock(&ov->lock);
 
 	ov->dev = NULL;
 
diff --git a/linux/drivers/media/video/ov511.h b/linux/drivers/media/video/ov511.h
--- a/linux/drivers/media/video/ov511.h
+++ b/linux/drivers/media/video/ov511.h
@@ -495,6 +495,9 @@ struct usb_ov511 {
 	int has_decoder;	/* Device has a video decoder */
 	int pal;		/* Device is designed for PAL resolution */
 
+	/* ov511 device number ID */
+	int nr;			/* Stores a device number */
+
 	/* I2C interface */
 	struct mutex i2c_lock;	  /* Protect I2C controller regs */
 	unsigned char primary_i2c_slave;  /* I2C write id of sensor */




Cheers,
Mauro