Em Mon, 15 Jun 2009 23:35:59 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> > By looking on this patch, it is obfuscating the function even more. Creating more
> > confusion on it, due to some reasons:
> >
> > 1) The name "kernel number" doesn't seem much appropriate. Any number used in
> > "kernel" can be called as a "kernel number".
>
> Actually, that is apparently what the number X is called in a node like
> /dev/videoX. I didn't make up that term, it's what I found when reading
> 'man udev'. Since udev deals extensively with these concepts I borrowed the
> udev terminology for this. If someone knows a better word, then I'm happy
> to use that.
Ah, now I see where you got this. Yet, this is what man udev says:
$number, %n
The kernel number for this device. For example, ’sda3’ has
kernel number of ’3’
$major, %M
The kernel major number for the device.
$minor %m
The kernel minor number for the device.
See, on all all tree is calls "kernel [<something>] number". Seems confusing
enough to avoid those names at the V4L2 core. Also, even the man needed to give
an example, showing that this nomenclature may not be widely understood.
Maybe we can just call it as "dev_number" and properly explain its meaning.
> > That's said, the logic of when minor range is not fixed seems broken, as it
> > will change only an internal representation of the device. So the module
> > parameters that reflect at "nr" var won't work as expected.
>
> No, they work exactly as expected: if you set nr to 1 then you get a /dev/video1
> node no matter what the FIXED_MINOR_RANGES setting is (provided there isn't
> already a /dev/video1 node, in which case it will find the next available
> kernel number).
>
> > So, the current code seems too complex and broken.
>
> No, it's neither too complex nor broken, although it clearly needs still more
> comments.
The code is complex. For example, take a look at this:
#ifdef CONFIG_VIDEO_FIXED_MINOR_RANGES
...
#else
/* The kernel number and minor numbers are independent */
for (i = 0; i < VIDEO_NUM_DEVICES; i++)
if (video_device[i] == NULL)
break;
if (i == VIDEO_NUM_DEVICES) {
..
return -ENFILE;
}
#endif
vdev->minor = i + minor_offset;
...
/* Should not happen since we thought this minor was free */
WARN_ON(video_device[vdev->minor] != NULL);
when !FIXED_MINOR_RANGES, you're return if all video_device[i] are used. Then,
you do a WARN_ON(video_device[i + minor_offset]) instead of video_device[i].
People need to look back at the code to identify that minor_offset is equal to
0 when !FIXED_MINOR_RANGES.
Also, by letting the function to allocate a device even when video_device !=
NULL seems broken. It should instead call WARN and return with -EINVAL.
The presence of the #if's, plus the high number of "phases" at the same
function make it harder to read and understand. The code will be more readable
if we break it into a few static functions (that will be inline anyway, due to
gcc optimizer).
>
> > Just as reference, the behavior before changeset 9133 is:
> >
> > switch (type) {
> > case VFL_TYPE_GRABBER:
> > base = MINOR_VFL_TYPE_GRABBER_MIN;
> > ...
> > }
> >
> > i = base + nr;
> > vfd->minor = i;
> >
> >
> > where nr is auto-selected for negative values, or just used above otherwise.
> >
> > That's said, IMO, we need to rework at the function, making it simpler, and
> > fixing the behavior where the user wants to select a minor.
>
> The user doesn't select a minor number, the user selects a kernel number.
> With udev minor numbers have become completely uninteresting and unless
> FIXED_MINOR_RANGES is set each node will be assigned the first free minor
> number.
OK, now I see what you're meaning.
With respect to the code, I still think it does too much into just one
function. It may make sense to break the code into more functions to allow an
easier understanding.
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
