| Message ID | 4E04732A.3060305@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ceph-devel" 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 ceph-devel" 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 ceph-devel" 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/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>