| Message ID | alpine.LNX.2.00.1106232356530.17688@swampdragon.chaosbits.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 23 Jun 2011, Jesper Juhl wrote: > It was pointed out by 'make versioncheck' that some includes of > linux/version.h were not needed in include/. > This patch removes them. > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> Acked-by: Sage Weil <sage@newdream.net> for the ceph bit. sage > --- > include/linux/ceph/messenger.h | 1 - > include/media/pwc-ioctl.h | 1 - > 2 files changed, 0 insertions(+), 2 deletions(-) > > diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h > index 31d91a6..291aa6e 100644 > --- a/include/linux/ceph/messenger.h > +++ b/include/linux/ceph/messenger.h > @@ -6,7 +6,6 @@ > #include <linux/net.h> > #include <linux/radix-tree.h> > #include <linux/uio.h> > -#include <linux/version.h> > #include <linux/workqueue.h> > > #include "types.h" > diff --git a/include/media/pwc-ioctl.h b/include/media/pwc-ioctl.h > index 0f19779..1ed1e61 100644 > --- a/include/media/pwc-ioctl.h > +++ b/include/media/pwc-ioctl.h > @@ -53,7 +53,6 @@ > */ > > #include <linux/types.h> > -#include <linux/version.h> > > /* Enumeration of image sizes */ > #define PSZ_SQCIF 0x00 > -- > 1.7.5.2 > > > -- > Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ > Don't top-post http://www.catb.org/jargon/html/T/top-post.html > Plain text mails only, please. > > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, June 24, 2011 13:21:14 Mauro Carvalho Chehab wrote: > Em 23-06-2011 18:58, Jesper Juhl escreveu: > > It was pointed out by 'make versioncheck' that some includes of > > linux/version.h were not needed in include/. > > This patch removes them. > > > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> > > --- > > include/linux/ceph/messenger.h | 1 - > > include/media/pwc-ioctl.h | 1 - > > 2 files changed, 0 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h > > index 31d91a6..291aa6e 100644 > > --- a/include/linux/ceph/messenger.h > > +++ b/include/linux/ceph/messenger.h > > @@ -6,7 +6,6 @@ > > #include <linux/net.h> > > #include <linux/radix-tree.h> > > #include <linux/uio.h> > > -#include <linux/version.h> > > #include <linux/workqueue.h> > > > > #include "types.h" > > diff --git a/include/media/pwc-ioctl.h b/include/media/pwc-ioctl.h > > index 0f19779..1ed1e61 100644 > > --- a/include/media/pwc-ioctl.h > > +++ b/include/media/pwc-ioctl.h > > @@ -53,7 +53,6 @@ > > */ > > > > #include <linux/types.h> > > -#include <linux/version.h> > > > > /* Enumeration of image sizes */ > > #define PSZ_SQCIF 0x00 > > > The usage of version.h at the Linux media kernel is due to a V4L2 API requirement[1], > where an ioctl query of VIDIOC_QUERYCAP type would return the driver version formatted > with KERNEL_VERSION() macro. > > [1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-querycap.html > > While a few driver maintainers are careful enough to increment it on every new > kernel version where the driver was touched, others simply keep it outdated. > > IMHO, it doesn't make much sense on having a per-driver version field: the V4L2 layer > should be enough to abstract hardware differences, and to avoid userspace to have a per > driver list of hacks. I don't think that the userspace applications are really using it. Applications are certainly using it. I know this for a fact for the ivtv driver where feature improvements are marked that way. Without more research on how this is used I am not comfortable with this. Regards, Hans > Module versions should just use the MODULE_VERSION() macro. > > So, IMO, the better would be to convert this field into a V4L2 API version field > instead like the enclosed patch. Of course, this also means to change the V4L2 API > Docbook. After that, we can cleanup all those linux/version.h code on all V4L drivers. > > The idea is that, every time we add something new at the V4L2 API, we'll increment it > to match the current kernel version. > > On a quick look, all drivers, except by one uses versions <= KERNEL_VERSION(3, 0, 0). > The only exception is the pwc driver, with version is KERNEL_VERSION(10, 0, 12). Due to > a bug on it, it also reports its version as: "10.0.14" at module version. The version > 10.0.12 is reported there since 2006, even having suffered a major change, due to the > removal of the V4L1 API, on changeset 479567ce3af7b99d645a3c53b8ca2fc65e46efdc. > So, I think it would be safe to change it to 3.0.0, as using the version here, in the favor > of a greater good. We can keep the driver-specific version only at > > Comments? > > If others are ok with that, I'll prepare the changesets. > > Cheers, > Mauro > > > - > > [media] v4l2 core: Use a per-API version instead of a per driver version > > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> > > diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c > index 213ba7d..d8fa571 100644 > --- a/drivers/media/video/v4l2-ioctl.c > +++ b/drivers/media/video/v4l2-ioctl.c > @@ -16,6 +16,7 @@ > #include <linux/slab.h> > #include <linux/types.h> > #include <linux/kernel.h>diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c > index 213ba7d..b19ad56 100644 > --- a/drivers/media/video/v4l2-ioctl.c > +++ b/drivers/media/video/v4l2-ioctl.c > @@ -16,6 +16,7 @@ > #include <linux/slab.h> > #include <linux/types.h> > #include <linux/kernel.h> > +#include <linux/version.h> > > #include <linux/videodev2.h> > > @@ -27,6 +28,8 @@ > #include <media/v4l2-device.h> > #include <media/v4l2-chip-ident.h> > > +#define V4L2_API_VERSION KERNEL_VERSION(3, 0, 0) > + > #define dbgarg(cmd, fmt, arg...) \ > do { \ > if (vfd->debug & V4L2_DEBUG_IOCTL_ARG) { \ > @@ -606,13 +609,16 @@ static long __video_do_ioctl(struct file *file, > break; > > ret = ops->vidioc_querycap(file, fh, cap); > - if (!ret) > + if (!ret) { > + cap->version = V4L2_API_VERSION; > + > dbgarg(cmd, "driver=%s, card=%s, bus=%s, " > "version=0x%08x, " > "capabilities=0x%08x\n", > cap->driver, cap->card, cap->bus_info, > cap->version, > cap->capabilities); > + } > break; > } > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Applications are certainly using it. I know this for a fact for the ivtv driver where > feature improvements are marked that way. > > Without more research on how this is used I am not comfortable with this. > > Regards, > > Hans MythTV has a bunch of these too (mainly so the code can adapt to driver bugs that are fixed in later revisions). Putting Mauro's patch upstream will definitely cause breakage. Also, it screws up the ability for users to get fixes through the media_build tree (unless you are increasing the revision constantly with every merge you do). Devin
Em 24-06-2011 09:20, Devin Heitmueller escreveu: >> Applications are certainly using it. I know this for a fact for the ivtv driver where >> feature improvements are marked that way. >> >> Without more research on how this is used I am not comfortable with this. >> >> Regards, >> >> Hans > > MythTV has a bunch of these too (mainly so the code can adapt to > driver bugs that are fixed in later revisions). Putting Mauro's patch > upstream will definitely cause breakage. It shouldn't, as ivtv driver version is lower than 3.0.0. All the old bug fixes aren't needed if version is >= 3.0.0. Besides that, trusting on a driver revision number to detect that a bug is there is not the right thing to do, as version numbers are never increased at the stable kernels (nor distro modified kernels take care of increasing revision number as patches are backported there). In other words, relying on it doesn't work fine. > Also, it screws up the ability for users to get fixes through the > media_build tree (unless you are increasing the revision constantly > with every merge you do). Why? Developers don't increase version numbers on every applied patch (with is great, as it avoids merge conflicts). Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 24, 2011 at 9:29 AM, Mauro Carvalho Chehab <mchehab@infradead.org> wrote: >> MythTV has a bunch of these too (mainly so the code can adapt to >> driver bugs that are fixed in later revisions). Putting Mauro's patch >> upstream will definitely cause breakage. > > It shouldn't, as ivtv driver version is lower than 3.0.0. All the old bug fixes > aren't needed if version is >= 3.0.0. > > Besides that, trusting on a driver revision number to detect that a bug is > there is not the right thing to do, as version numbers are never increased at > the stable kernels (nor distro modified kernels take care of increasing revision > number as patches are backported there). The versions are increased at the discretion of the driver maintainer, usually when there is some userland visible change in driver behavior. I assure you the application developers don't *want* to rely on such a mechanism, but there have definitely been cases in the past where there was no easy way to detect the behavior of the driver from userland. It lets application developers work around things like violations of the V4L2 standard which get fixed in newer revisions of the driver. It provides them the ability to put a hack in their code that says "if (version < X) then this driver feature is broken and I shouldn't use it." > In other words, relying on it doesn't work fine. It's the best (and really only solution) we have today. >> Also, it screws up the ability for users to get fixes through the >> media_build tree (unless you are increasing the revision constantly >> with every merge you do). > > Why? Developers don't increase version numbers on every applied patch > (with is great, as it avoids merge conflicts). The driver maintainer doesn't *have* to increase the version - he does it when he thinks it's appropriate. The point is you are taking that discretion out of *their* hands, and you yourself are unaware of when it is actually needed. You need to stop looking at this from a purist standpoint and think of how application developers actually use the API. They need tools like this to allow them to work around driver bugs while having a source codebase which operates against different kernels (including kernels that may still have those bugs). Sure, in a perfect world where drivers don't have bugs and applications don't have to run against older kernels, what you are saying is not illogical. But then again, we don't live in a perfect world. Devin
On Friday, June 24, 2011 15:45:59 Devin Heitmueller wrote: > On Fri, Jun 24, 2011 at 9:29 AM, Mauro Carvalho Chehab > <mchehab@infradead.org> wrote: > >> MythTV has a bunch of these too (mainly so the code can adapt to > >> driver bugs that are fixed in later revisions). Putting Mauro's patch > >> upstream will definitely cause breakage. > > > > It shouldn't, as ivtv driver version is lower than 3.0.0. All the old bug fixes > > aren't needed if version is >= 3.0.0. > > > > Besides that, trusting on a driver revision number to detect that a bug is > > there is not the right thing to do, as version numbers are never increased at > > the stable kernels (nor distro modified kernels take care of increasing revision > > number as patches are backported there). > > The versions are increased at the discretion of the driver maintainer, > usually when there is some userland visible change in driver behavior. > I assure you the application developers don't *want* to rely on such > a mechanism, but there have definitely been cases in the past where > there was no easy way to detect the behavior of the driver from > userland. > > It lets application developers work around things like violations of > the V4L2 standard which get fixed in newer revisions of the driver. > It provides them the ability to put a hack in their code that says "if > (version < X) then this driver feature is broken and I shouldn't use > it." Indeed. Ideally we shouldn't need it. But reality is different. What we have right now works and I see no compelling reason to change the behavior. Regards, Hans > > In other words, relying on it doesn't work fine. > > It's the best (and really only solution) we have today. > > >> Also, it screws up the ability for users to get fixes through the > >> media_build tree (unless you are increasing the revision constantly > >> with every merge you do). > > > > Why? Developers don't increase version numbers on every applied patch > > (with is great, as it avoids merge conflicts). > > The driver maintainer doesn't *have* to increase the version - he does > it when he thinks it's appropriate. The point is you are taking that > discretion out of *their* hands, and you yourself are unaware of when > it is actually needed. > > You need to stop looking at this from a purist standpoint and think of > how application developers actually use the API. They need tools like > this to allow them to work around driver bugs while having a source > codebase which operates against different kernels (including kernels > that may still have those bugs). > > Sure, in a perfect world where drivers don't have bugs and > applications don't have to run against older kernels, what you are > saying is not illogical. But then again, we don't live in a perfect > world. > > Devin > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 24-06-2011 09:20, Devin Heitmueller escreveu: > Also, it screws up the ability for users to get fixes through the > media_build tree (unless you are increasing the revision constantly > with every merge you do). Patches merged, and media_build modified in order to use the V4L2 stack version, instead of the kernel one. So, while I'm using a 2.6.32 kernel: $ uname -a Linux pedra 2.6.32-131.0.15.el6.x86_64 #1 SMP Tue May 10 15:42:40 EDT 2011 x86_64 x86_64 x86_64 GNU/Linux Driver reports version 3.0.0: $ v4l2-ctl -D Driver Info (not using libv4l2): Driver name : vivi Card type : vivi Bus info : vivi-000 Driver version: 3.0.0 Capabilities : 0x05000001 Video Capture Read/Write Streaming It may be a good idea to increment the extraver number to be, for example, 3.0.99 (or to just decrement 1 number), in order to reflect that this driver is not the vanilla 3.0.0, but, instead, a backported one, otherwise, it will report the version from the last git backport we merge at media_tree.git. If we just subtract 1, we'll have (right now): $ v4l2-ctl -D Driver Info (not using libv4l2): Driver name : vivi Card type : vivi Bus info : vivi-000 Driver version: 2.255.255 Capabilities : 0x05000001 Video Capture Read/Write Streaming Which looks somewhat weird, but, after the 3.1 merge window, it will be 3.0.255, with would be nice. So, if we go this way, the better is to wait until 3.1-rc1 before applying it. Comments? PS.: The media_build is not optimized in the sense that a version increment will make it recompile everything. I'll likely fix it if it bothers me enough ;) Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h index 31d91a6..291aa6e 100644 --- a/include/linux/ceph/messenger.h +++ b/include/linux/ceph/messenger.h @@ -6,7 +6,6 @@ #include <linux/net.h> #include <linux/radix-tree.h> #include <linux/uio.h> -#include <linux/version.h> #include <linux/workqueue.h> #include "types.h" diff --git a/include/media/pwc-ioctl.h b/include/media/pwc-ioctl.h index 0f19779..1ed1e61 100644 --- a/include/media/pwc-ioctl.h +++ b/include/media/pwc-ioctl.h @@ -53,7 +53,6 @@ */ #include <linux/types.h> -#include <linux/version.h> /* Enumeration of image sizes */ #define PSZ_SQCIF 0x00
It was pointed out by 'make versioncheck' that some includes of linux/version.h were not needed in include/. This patch removes them. Signed-off-by: Jesper Juhl <jj@chaosbits.net> --- include/linux/ceph/messenger.h | 1 - include/media/pwc-ioctl.h | 1 - 2 files changed, 0 insertions(+), 2 deletions(-)