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". 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. 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) 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; 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. So, the current code seems too complex and broken. 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. 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
