On Monday 15 June 2009 21:48:32 Mauro Carvalho Chehab wrote:
> Hi Hans,
>
> Em Mon, 15 Jun 2009 13:27:42 +0200
> Hans Verkuil <hverk...@xs4all.nl> escreveu:
>
> > On Sunday 14 June 2009 12:15:34 Hans Verkuil wrote:
> > > On Sunday 14 June 2009 11:50:51 Hans Verkuil wrote:
> > > > Hi Mauro,
> > > >
> > > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for
> > > > the
> > > > following:
> > > >
> > > > - ivtv/cx18: fix regression: class controls are no longer seen
> > > > - v4l2-ctl: add modulator get/set options.
> > > > - v4l2-spec: update VIDIOC_G/S_MODULATOR section.
> > > > - compat: fix __fls check for the arm architecture.
> > > > - smscoreapi: fix compile warning
> > > >
> > > > The first one is a high prio bug as it is a 2.6.30 regression. Mike,
> > > > once
> > > > this is merged in the git tree this one can be queued for the 2.6.30
> > > > stable
> > > > series.
> > > >
> > > > The other changes are small stuff.
> > >
> > > Added one more minor change:
> > >
> > > - v4l2-i2c-drv.h: add comment describing when not to use this header.
>
> The above patches seem ok.
>
> >
> > And added this one as well:
> >
> > - v4l2-dev: fix some confusing variable names and comments
> >
> > # HG changeset patch
> > # User Hans Verkuil <hverk...@xs4all.nl>
> > # Date 1245063581 -7200
> > # Node ID 083bb5ad197e4c49430de2b26f3115743fe5cc27
> > # Parent 743225159afab6d79a0d6095bc302f9922305c33
> > v4l2-dev: fix some confusing variable names and comments
> >
> > From: Hans Verkuil <hverk...@xs4all.nl>
> >
> > Some variable names and comments were rather misleading when it came
> > to the distinction between kernel numbers and minor numbers. This should
> > clarify things.
> >
> > Priority: normal
> >
> > Signed-off-by: Hans Verkuil <hverk...@xs4all.nl>
> >
> > --- a/linux/drivers/media/video/v4l2-dev.c Sun Jun 14 12:12:11 2009 +0200
> > +++ b/linux/drivers/media/video/v4l2-dev.c Mon Jun 15 12:59:41 2009 +0200
> > @@ -419,10 +419,10 @@ int video_register_device_index(struct v
> > int video_register_device_index(struct video_device *vdev, int type, int
> > nr,
> > int index)
> > {
> > - int i = 0;
> > + int minor;
> > int ret;
> > int minor_offset = 0;
> > - int minor_cnt = VIDEO_NUM_DEVICES;
> > + int kernel_nrs = VIDEO_NUM_DEVICES;
> > const char *name_base;
> > void *priv = video_get_drvdata(vdev);
> >
> > @@ -470,52 +470,52 @@ int video_register_device_index(struct v
> > switch (type) {
> > case VFL_TYPE_GRABBER:
> > minor_offset = 0;
> > - minor_cnt = 64;
> > + kernel_nrs = 64;
> > break;
> > case VFL_TYPE_RADIO:
> > minor_offset = 64;
> > - minor_cnt = 64;
> > + kernel_nrs = 64;
> > break;
> > case VFL_TYPE_VTX:
> > minor_offset = 192;
> > - minor_cnt = 32;
> > + kernel_nrs = 32;
> > break;
> > case VFL_TYPE_VBI:
> > minor_offset = 224;
> > - minor_cnt = 32;
> > + kernel_nrs = 32;
> > break;
> > default:
> > minor_offset = 128;
> > - minor_cnt = 64;
> > - break;
> > - }
> > -#endif
> > -
> > - /* Pick a minor number */
> > + kernel_nrs = 64;
> > + break;
> > + }
> > +#endif
> > +
> > + /* Pick a kernel number */
> > mutex_lock(&videodev_lock);
> > - nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);
> > - if (nr == minor_cnt)
> > - nr = find_first_zero_bit(video_nums[type], minor_cnt);
> > - if (nr == minor_cnt) {
> > + nr = find_next_zero_bit(video_nums[type], kernel_nrs, nr == -1 ? 0 :
> > nr);
> > + if (nr == kernel_nrs)
> > + nr = find_first_zero_bit(video_nums[type], kernel_nrs);
> > + if (nr == kernel_nrs) {
> > printk(KERN_ERR "could not get a free kernel number\n");
> > mutex_unlock(&videodev_lock);
> > return -ENFILE;
> > }
> > #ifdef CONFIG_VIDEO_FIXED_MINOR_RANGES
> > /* 1-on-1 mapping of kernel number to minor number */
> > - i = nr;
> > + minor = nr;
> > #else
> > /* The kernel number and minor numbers are independent */
> > - for (i = 0; i < VIDEO_NUM_DEVICES; i++)
> > - if (video_device[i] == NULL)
> > + for (minor = 0; minor < VIDEO_NUM_DEVICES; minor++)
> > + if (video_device[minor] == NULL)
> > break;
> > - if (i == VIDEO_NUM_DEVICES) {
> > + if (minor == VIDEO_NUM_DEVICES) {
> > mutex_unlock(&videodev_lock);
> > printk(KERN_ERR "could not get a free minor\n");
> > return -ENFILE;
> > }
> > #endif
> > - vdev->minor = i + minor_offset;
> > + vdev->minor = minor + minor_offset;
> > vdev->num = nr;
> > set_bit(nr, video_nums[type]);
> > /* Should not happen since we thought this minor was free */
> >
>
> The progressive changes at video_register_device() created a mess on this
> function, that reflected on ov511 driver, but it is also present on the
> others.
>
> 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.
> 2) minor and major devices are kernel numbers per excellence. Since the start
> of Unix, they are defined by the Unix kernel developers as a kernel number to
> uniquely describe a char or block device;
>
> 4) the "int nr" non-negative parameter is passed by all drivers (maybe except
> by ivtv/cx18) to request the core to use an specific "minor" for the device.
> This still happens like this, if CONFIG_VIDEO_FIXED_MINOR_RANGES is defined.
In the past the minor number directly mapped to a kernel number (to keep that
terminology). In practice it was used to ensure that if you specified 1 you
would get a /dev/video1 node. So although nr officially stood for a minor
number, in practice you meant the kernel number. No one cares about the minor
number, it is the kernel number that the user wants to specify.
> 5) as "nr" is expected to be minor number, those changes seem wrong, as you're
> comparing a minor number with something else:
>
> - nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);
> + nr = find_next_zero_bit(video_nums[type], kernel_nrs, nr == -1 ? 0 :
> nr);
> - if (nr == minor_cnt)
> + if (nr == kernel_nrs)
See explanation above.
> 6) what you called as "kernel_nrs" is, in fact, the number of minors for each
> type, used only when CONFIG_VIDEO_FIXED_MINOR_RANGES is defined. So, this
> also seems wrong:
>
> case VFL_TYPE_VTX:
> minor_offset = 192;
> - minor_cnt = 32;
> + kernel_nrs = 32;
In the case of FIXED_MINOR_RANGES the two concepts (minor number or kernel
number) are identical (except for the offset). If this config isn't set, then
you always mean kernel number.
> 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.
> 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.
Regards,
Hans
>
>
>
> Cheers,
> Mauro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
