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 <hverkuil@xxxxxxxxx> 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 <hverkuil@xxxxxxxxx> > > # Date 1245063581 -7200 > > # Node ID 083bb5ad197e4c49430de2b26f3115743fe5cc27 > > # Parent 743225159afab6d79a0d6095bc302f9922305c33 > > v4l2-dev: fix some confusing variable names and comments > > > > From: Hans Verkuil <hverkuil@xxxxxxxxx> > > > > 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 <hverkuil@xxxxxxxxx> > > > > --- 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 majordomo@xxxxxxxxxxxxxxx > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
