Hi, Am Montag, den 15.06.2009, 23:35 +0200 schrieb Hans Verkuil: > 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. without looking in any details yet, this always caused discussions and misunderstandings about mapping and even distributions clashed on it. I think the best is to follow how it was historically in use. If needed, even udev has to consider the use cases. In fact, the users can determine the minors at their will currently for some good reasons. Starts with alsa stuff over several drivers and frontend enumeration. Cheers, Hermann -- 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
