On Tuesday 16 June 2009 02:03:54 Mauro Carvalho Chehab wrote:
> 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.
That's better, yes. I remember that at the time I wasn't sure what to do
here and I agree I made the wrong choice.
>
> 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.
I'll attempt that this weekend.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
--
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
