diff mbox

[RFC] Don't use linux/version.h anymore to indicate a per-driver version - Was: Re: [PATCH 03/37] Remove unneeded version.h includes from include/

Message ID 4E04A122.2080002@infradead.org (mailing list archive)
State Not Applicable
Headers show

Commit Message

Mauro Carvalho Chehab June 24, 2011, 2:37 p.m. UTC
Em 24-06-2011 10:54, Hans Verkuil escreveu:
> 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.

A per-driver version only works if the user is running a vanilla kernel without 
any stable patches applied. 

I doubt that this covers the large amount of the users: they'll either use an 
stable patched kernel or a distribution-specific one. On both cases, the driver
version is not associated with a bug fix, as the driver maintainers just take
care of increasing the driver version once per each new kernel version (when
they care enough).

Also, a git blame for the V4L2 drivers shows that only a few drivers have their
version increased as changes are applied there. So, relying on cap->version 
has a minimal chance of working only with a few drivers, with vanilla *.0 kernels.

Anyway, I think that we should at least apply the enclosed patch, and remove
KERNEL_VERSION and linux/version.h includes for the drivers that didn't change
its version in the past 2 kernel releases.

I'll work later on the linux/version.h cleanup patches.

Cheers,
Mauro

-

[media] v4l2-ioctl: Add a default value for kernel version

Most drivers don't increase kernel versions as newer features are added or
bug fixes are solved. So, vidioc_querycap returned value for cap->version is
meaningless. Instead of keeping this situation forever, let's add a default
value matching the current Linux version.

Drivers that want to keep their own version control can still do it, as they
can override the default value for cap->version.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

--
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

Comments

Stefan Richter June 24, 2011, 6:34 p.m. UTC | #1
On Jun 24 Mauro Carvalho Chehab wrote:
> Em 24-06-2011 10:54, Hans Verkuil escreveu:
> > On Friday, June 24, 2011 15:45:59 Devin Heitmueller wrote:
> >> 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.
> 
> A per-driver version only works if the user is running a vanilla kernel without 
> any stable patches applied. 
> 
> I doubt that this covers the large amount of the users: they'll either use an 
> stable patched kernel or a distribution-specific one. On both cases, the driver
> version is not associated with a bug fix, as the driver maintainers just take
> care of increasing the driver version once per each new kernel version (when
> they care enough).
> 
> Also, a git blame for the V4L2 drivers shows that only a few drivers have their
> version increased as changes are applied there. So, relying on cap->version 
> has a minimal chance of working only with a few drivers, with vanilla *.0 kernels.

If the "driver version" is in fact an ABI version, then the driver author
should really increase it only when ABI behavior is changed (and only if
the behavior change can only be communicated by version number --- e.g.
addition of an ioctl is not among such reasons).  And the author should
commit behavior changing implementation and version number change in a
single changeset.

And anybody who backmerges such an ABI behavior change into another kernel
branch (stable, longterm, distro...) must backmerge the associated version
number change too.

Of course sometimes people realize this only after the fact.  Or driver
authors don't have a clear understanding of ABI versioning to begin with.
I am saying so because I had to learn it too; I certainly wasn't born
with an instinct knowledge how to do it properly.

(Disclaimer:  I have no stake in drivers/media/ ABIs.  But I am involved
in maintaining a userspace ABI elsewhere in drivers/firewire/, and one of
the userspace libraries that use this ABI.)
Devin Heitmueller June 24, 2011, 6:48 p.m. UTC | #2
On Fri, Jun 24, 2011 at 2:34 PM, Stefan Richter
<stefanr@s5r6.in-berlin.de> wrote:
> If the "driver version" is in fact an ABI version, then the driver author
> should really increase it only when ABI behavior is changed (and only if
> the behavior change can only be communicated by version number --- e.g.
> addition of an ioctl is not among such reasons).  And the author should
> commit behavior changing implementation and version number change in a
> single changeset.
>
> And anybody who backmerges such an ABI behavior change into another kernel
> branch (stable, longterm, distro...) must backmerge the associated version
> number change too.
>
> Of course sometimes people realize this only after the fact.  Or driver
> authors don't have a clear understanding of ABI versioning to begin with.
> I am saying so because I had to learn it too; I certainly wasn't born
> with an instinct knowledge how to do it properly.
>
> (Disclaimer:  I have no stake in drivers/media/ ABIs.  But I am involved
> in maintaining a userspace ABI elsewhere in drivers/firewire/, and one of
> the userspace libraries that use this ABI.)

Hi Stefan,

To be clear, I don't think anyone is actually proposing that the
driver version number really be used as any form of formal "ABI
versioning" scheme.  In almost all cases, it's so the application can
know to *not* do something is the driver is older than X.

Given all the cases I've seen, it doesn't really hurt anything if the
driver contains a fix from newer than X, aside from the fact that the
application won't take advantage of whatever feature/functionality the
fix made work.  In other words, I think from a backport standpoint, it
usually doesn't *hurt* anything if a fix is backported without the
version being incremented, aside from applications not knowing that
the feature/fix is present.

Really, this is all about applications being able to jam a hack into
their code that translates to "don't call this ioctl() with some
particular argument if it's driver W less than version X, because the
driver had a bug that is likely to panic the guy's PC".  Sure, it's a
crummy solution, but at this point it's the best that we have got.

Devin
Andy Walls June 24, 2011, 9:04 p.m. UTC | #3
On Fri, 2011-06-24 at 14:48 -0400, Devin Heitmueller wrote:
> On Fri, Jun 24, 2011 at 2:34 PM, Stefan Richter
> <stefanr@s5r6.in-berlin.de> wrote:
> > If the "driver version" is in fact an ABI version, then the driver author
> > should really increase it only when ABI behavior is changed (and only if
> > the behavior change can only be communicated by version number --- e.g.
> > addition of an ioctl is not among such reasons).  And the author should
> > commit behavior changing implementation and version number change in a
> > single changeset.
> >
> > And anybody who backmerges such an ABI behavior change into another kernel
> > branch (stable, longterm, distro...) must backmerge the associated version
> > number change too.
> >
> > Of course sometimes people realize this only after the fact.  Or driver
> > authors don't have a clear understanding of ABI versioning to begin with.
> > I am saying so because I had to learn it too; I certainly wasn't born
> > with an instinct knowledge how to do it properly.
> >
> > (Disclaimer:  I have no stake in drivers/media/ ABIs.  But I am involved
> > in maintaining a userspace ABI elsewhere in drivers/firewire/, and one of
> > the userspace libraries that use this ABI.)
> 
> Hi Stefan,
> 
> To be clear, I don't think anyone is actually proposing that the
> driver version number really be used as any form of formal "ABI
> versioning" scheme.  In almost all cases, it's so the application can
> know to *not* do something is the driver is older than X.

MythTV, for example, used to use the driver version to work around old
VBI bugs and MPEG encoder quirks that the older version of the driver
may not have known how to handle:

https://github.com/MythTV/mythtv/blob/b98d3a98e3187000ae652df5ffebe2beb5221ba7/mythtv/libs/libmythtv/mpegrecorder.cpp#L335

But for newer versions, MythTV could avoid using its own odd hacks.
The bleeding edge MythTV now has most of these removed.


> Given all the cases I've seen, it doesn't really hurt anything if the
> driver contains a fix from newer than X, aside from the fact that the
> application won't take advantage of whatever feature/functionality the
> fix made work.  In other words, I think from a backport standpoint, it
> usually doesn't *hurt* anything if a fix is backported without the
> version being incremented, aside from applications not knowing that
> the feature/fix is present.

That seems to be the case to me.


> Really, this is all about applications being able to jam a hack into
> their code that translates to "don't call this ioctl() with some
> particular argument if it's driver W less than version X, because the
> driver had a bug that is likely to panic the guy's PC".

Well, not even panics per se, but some thing like the VBI is broken, or
the volume control doesn't work, IR blaster is works for this version,
or something else stupid that is very visible to the end user.

I also use the driver version for troubleshooting problem with users.  I
roughly know what wasn't working in what version of the cx18 and ivtv
drivers.  If the end user can tell me the driver version (using v4l2-ctl
--log-status) along with his symptoms, it makes my life easier.  Being
able to efficiently help the end user is a win for both me and the end
user.


>   Sure, it's a
> crummy solution, but at this point it's the best that we have got

Yup.  We do have crummier solutions:

Telling the end user to read their kernel source code to figure out what
bugs their driver release has, and to then adjust their application
command line arguments accordingly. ;)


Regards,
Andy

--
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
Stefan Richter June 24, 2011, 9:10 p.m. UTC | #4
On Jun 24 Devin Heitmueller wrote:
> Really, this is all about applications being able to jam a hack into
> their code that translates to "don't call this ioctl() with some
> particular argument if it's driver W less than version X, because the
> driver had a bug that is likely to panic the guy's PC".  Sure, it's a
> crummy solution, but at this point it's the best that we have got.

The second best.  The best that we have got is that the user runs a fixed
kernel.

Anyway; if this is the only purpose that this interface version¹ serves,
then Mauro's subsystem-centralized solution has the benefit that it
eliminates mistakes due to oversight by individual driver authors.
Especially because the kind of implementation behavior changes that are
tracked by this type of version datum are sometimes just discovered or
documented in hindsight.  On the other hand, Mauro's solution is redundant
to the uname(2) syscall.

¹) Yes, it is still an ABI version, nothing less.  With all its backwards
and forwards compatibility ramifications.
Stefan Richter June 24, 2011, 9:20 p.m. UTC | #5
On Jun 24 Andy Walls wrote:
> I also use the driver version for troubleshooting problem with users.  I
> roughly know what wasn't working in what version of the cx18 and ivtv
> drivers.  If the end user can tell me the driver version (using v4l2-ctl
> --log-status) along with his symptoms, it makes my life easier.

Easier:
  "I run Ubuntu 10.4".
  "I run kernel 2.6.32."
One of these is usually already included in the first post or IRC message
from the user.

Separate driver versions are only needed on platforms where drivers are
not distributed by the operating system distributor, or driver source code
is not released within kernel source code.
Devin Heitmueller June 24, 2011, 9:22 p.m. UTC | #6
On Fri, Jun 24, 2011 at 5:20 PM, Stefan Richter
<stefanr@s5r6.in-berlin.de> wrote:
> Easier:
>  "I run Ubuntu 10.4".
>  "I run kernel 2.6.32."
> One of these is usually already included in the first post or IRC message
> from the user.
>
> Separate driver versions are only needed on platforms where drivers are
> not distributed by the operating system distributor, or driver source code
> is not released within kernel source code.

Unfortunately, this doesn't work as all too often the user has "Ubuntu
10.1 but I installed the latest media_build tree a few months ago".
Hence they are not necessarily on a particular binary release from a
distro but rather have a mix of a distro's binary release and a
v4l-dvb tree compiled from source.

Devin
Mauro Carvalho Chehab June 24, 2011, 9:44 p.m. UTC | #7
Em 24-06-2011 15:34, Stefan Richter escreveu:
> On Jun 24 Mauro Carvalho Chehab wrote:
>> Em 24-06-2011 10:54, Hans Verkuil escreveu:
>>> On Friday, June 24, 2011 15:45:59 Devin Heitmueller wrote:
>>>> 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.
>>
>> A per-driver version only works if the user is running a vanilla kernel without 
>> any stable patches applied. 
>>
>> I doubt that this covers the large amount of the users: they'll either use an 
>> stable patched kernel or a distribution-specific one. On both cases, the driver
>> version is not associated with a bug fix, as the driver maintainers just take
>> care of increasing the driver version once per each new kernel version (when
>> they care enough).
>>
>> Also, a git blame for the V4L2 drivers shows that only a few drivers have their
>> version increased as changes are applied there. So, relying on cap->version 
>> has a minimal chance of working only with a few drivers, with vanilla *.0 kernels.
> 
> If the "driver version" is in fact an ABI version, then the driver author
> should really increase it only when ABI behavior is changed (and only if
> the behavior change can only be communicated by version number --- e.g.
> addition of an ioctl is not among such reasons).  And the author should
> commit behavior changing implementation and version number change in a
> single changeset.

Yes, but "driver version" were never used as such. Several drivers got lots of
updates, ABI change behavior (like the removal of V4L1 API), etc without having
a single "driver version" increment. Other drivers increase it even on minor
changes.

IMO, it makes no sense on keeping it, but removing this field would break 
userspace, as a few programs seem to use it.

> And anybody who backmerges such an ABI behavior change into another kernel
> branch (stable, longterm, distro...) must backmerge the associated version
> number change too.

Yes, but, again, this doesn't happen. In general, the drivers that use it either
increment the version number on a separate patch, or integrate it with one of
the patches.

It is easy to take a look at ivtv, as it has a separate file with the version
number:

$ git log --oneline drivers/media/video/ivtv/ivtv-version.h
4359e5b V4L/DVB: ivtv: Increment driver version due to firmware loading changes
c019f90 V4L/DVB (10965): ivtv: bump version
c58dc0b V4L/DVB (8633): ivtv: update ivtv version number
be303e1 V4L/DVB (7930): ivtv: bump version to 1.3.0.
fcbbf6f V4L/DVB (7759): ivtv: increase version number to 1.2.1
0170a48 V4L/DVB (6762): ivtv: update version number to 1.2
612570f V4L/DVB (6091): ivtv: header cleanup
f38a798 V4L/DVB (5909): ivtv: update version to 1.1 to mark ivtv-fb support
1a0adaf V4L/DVB (5345): ivtv driver for Conexant cx23416/cx23415 MPEG encoder/decoder

Looking at the details of the above commits, on several cases there's no explanation
why the version was incremented, or why an userspace application should bother to have
any special treatment for that version or for the previous one.

The date of the commits also don't help much:

Date:   Sat Jun 12 13:55:33 2010 -0300
Date:   Wed Mar 11 18:50:04 2009 -0300
Date:   Wed Sep 3 16:46:58 2008 -0300
Date:   Sat May 24 12:43:43 2008 -0300
Date:   Sat Apr 26 09:43:22 2008 -0300
Date:   Fri Dec 7 20:31:17 2007 -0300
Date:   Thu Aug 23 05:42:59 2007 -0300
Date:   Sun Jul 22 08:39:43 2007 -0300
Date:   Fri Apr 27 12:31:25 2007 -0300

Even when there seems to have a good reason for version bump, like on this example
(from cx18 driver):

commit 9982be8
Author: Andy Walls <awalls@radix.net>
Date:   Wed Apr 15 20:49:19 2009 -0300

    V4L/DVB (11620): cx18: Increment version due to significant buffer handling changes
    
    Version bump from 1.1.0 to 1.2.0
    
    Signed-off-by: Andy Walls <awalls@radix.net>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

The commit message doesn't help to tell the application developer how version 1.1.0 is
different than version 1.2.0.

Also, as this is on a separate commit, if the buffer changes were backported into a
stable or distro kernel, the application will have no way to detect the differences.

On several cases, the version upgrade is simply due to the addition of a new type of
support, like this one:

commit 437b775
Author: Andy Walls <awalls@md.metrocast.net>
Date:   Sun Mar 27 00:43:30 2011 -0300

    [media] cx18: Bump driver version, since a new class of HVR-1600 is properly supported
    
    Make a user visible driver version change, for the inevitable user support
    questions about why newer model HVR-1600's do not work with (older
    versions of) the cx18 driver.

For things like that, it would be enough to simply increment MODULE_VERSION().

> Of course sometimes people realize this only after the fact.  Or driver
> authors don't have a clear understanding of ABI versioning to begin with.
> I am saying so because I had to learn it too; I certainly wasn't born
> with an instinct knowledge how to do it properly.
> 
> (Disclaimer:  I have no stake in drivers/media/ ABIs.  But I am involved
> in maintaining a userspace ABI elsewhere in drivers/firewire/, and one of
> the userspace libraries that use this ABI.)


Em 24-06-2011 15:48, Devin Heitmueller escreveu:
> To be clear, I don't think anyone is actually proposing that the
> driver version number really be used as any form of formal "ABI
> versioning" scheme.  In almost all cases, it's so the application can
> know to *not* do something is the driver is older than X.

As I've explained before, even that is wrong, as the version increment
patch may not have been backported.

> Given all the cases I've seen, it doesn't really hurt anything if the
> driver contains a fix from newer than X, aside from the fact that the
> application won't take advantage of whatever feature/functionality the
> fix made work.  In other words, I think from a backport standpoint, it
> usually doesn't *hurt* anything if a fix is backported without the
> version being incremented, aside from applications not knowing that
> the feature/fix is present.

New features could also be backported without version increment. This happens
from time to time at the enterprise distros, and on long duration stable
releases.

> Really, this is all about applications being able to jam a hack into
> their code that translates to "don't call this ioctl() with some
> particular argument if it's driver W less than version X, because the
> driver had a bug that is likely to panic the guy's PC".  Sure, it's a
> crummy solution, but at this point it's the best that we have got.

Version number is not enough for that. When there are such panic/OOPS bugs,
they are backported to -stable and to the distro kernels.

An application doing that will be removing a feature that is probably already
fixed.

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
Mauro Carvalho Chehab June 24, 2011, 9:49 p.m. UTC | #8
Em 24-06-2011 18:22, Devin Heitmueller escreveu:
> On Fri, Jun 24, 2011 at 5:20 PM, Stefan Richter
> <stefanr@s5r6.in-berlin.de> wrote:
>> Easier:
>>  "I run Ubuntu 10.4".
>>  "I run kernel 2.6.32."
>> One of these is usually already included in the first post or IRC message
>> from the user.
>>
>> Separate driver versions are only needed on platforms where drivers are
>> not distributed by the operating system distributor, or driver source code
>> is not released within kernel source code.
> 
> Unfortunately, this doesn't work as all too often the user has "Ubuntu
> 10.1 but I installed the latest media_build tree a few months ago".

> Hence they are not necessarily on a particular binary release from a
> distro but rather have a mix of a distro's binary release and a
> v4l-dvb tree compiled from source.


# modprobe vivi
# dmesg
WARNING: You are using an experimental version of the media stack.
	As the driver is backported to an older kernel, it doesn't offer
	enough quality for its usage in production.
	Use it with care.
Latest git patches (needed if you report a bug to linux-media@vger.kernel.org):
	d3302689d697a99d565ea37c8fb9a19a1a5d0f06 [media] rc-core: fix winbond-cir issues
	6337ae50f81c99efbf9fa924c9d1b8b51efbcef2 [media] rc/redrat3: dereferencing null pointer
	ad0fc4c9ac8bf871b7bf874b2cd34b6b65f099c7 [media] rc: double unlock in rc_register_device()
vivi-000: V4L2 device registered as video0
Video Technology Magazine Virtual Video Capture Board ver 0.8.0 successfully loaded.

The git changeset is a way better than a version number.

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
Mauro Carvalho Chehab June 24, 2011, 9:52 p.m. UTC | #9
Em 24-06-2011 18:10, Stefan Richter escreveu:
> On Jun 24 Devin Heitmueller wrote:
>> Really, this is all about applications being able to jam a hack into
>> their code that translates to "don't call this ioctl() with some
>> particular argument if it's driver W less than version X, because the
>> driver had a bug that is likely to panic the guy's PC".  Sure, it's a
>> crummy solution, but at this point it's the best that we have got.
> 
> The second best.  The best that we have got is that the user runs a fixed
> kernel.
> 
> Anyway; if this is the only purpose that this interface version¹ serves,
> then Mauro's subsystem-centralized solution has the benefit that it
> eliminates mistakes due to oversight by individual driver authors.
> Especially because the kind of implementation behavior changes that are
> tracked by this type of version datum are sometimes just discovered or
> documented in hindsight.  On the other hand, Mauro's solution is redundant
> to the uname(2) syscall.

Yes. That's why my initial proposal were to add some value to it by not associating
it with the kernel version, but with a number that will be incremented only if
the V4L2 API changes.

> 
> ¹) Yes, it is still an ABI version, nothing less.  With all its backwards
> and forwards compatibility ramifications.

--
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
Mauro Carvalho Chehab June 24, 2011, 10:16 p.m. UTC | #10
Em 24-06-2011 18:04, Andy Walls escreveu:
> On Fri, 2011-06-24 at 14:48 -0400, Devin Heitmueller wrote:
>> On Fri, Jun 24, 2011 at 2:34 PM, Stefan Richter
>> <stefanr@s5r6.in-berlin.de> wrote:
>>> If the "driver version" is in fact an ABI version, then the driver author
>>> should really increase it only when ABI behavior is changed (and only if
>>> the behavior change can only be communicated by version number --- e.g.
>>> addition of an ioctl is not among such reasons).  And the author should
>>> commit behavior changing implementation and version number change in a
>>> single changeset.
>>>
>>> And anybody who backmerges such an ABI behavior change into another kernel
>>> branch (stable, longterm, distro...) must backmerge the associated version
>>> number change too.
>>>
>>> Of course sometimes people realize this only after the fact.  Or driver
>>> authors don't have a clear understanding of ABI versioning to begin with.
>>> I am saying so because I had to learn it too; I certainly wasn't born
>>> with an instinct knowledge how to do it properly.
>>>
>>> (Disclaimer:  I have no stake in drivers/media/ ABIs.  But I am involved
>>> in maintaining a userspace ABI elsewhere in drivers/firewire/, and one of
>>> the userspace libraries that use this ABI.)
>>
>> Hi Stefan,
>>
>> To be clear, I don't think anyone is actually proposing that the
>> driver version number really be used as any form of formal "ABI
>> versioning" scheme.  In almost all cases, it's so the application can
>> know to *not* do something is the driver is older than X.
> 
> MythTV, for example, used to use the driver version to work around old
> VBI bugs and MPEG encoder quirks that the older version of the driver
> may not have known how to handle:
> 
> https://github.com/MythTV/mythtv/blob/b98d3a98e3187000ae652df5ffebe2beb5221ba7/mythtv/libs/libmythtv/mpegrecorder.cpp#L335
> 
> But for newer versions, MythTV could avoid using its own odd hacks.
> The bleeding edge MythTV now has most of these removed.

Removing it is a good thing.

>> Really, this is all about applications being able to jam a hack into
>> their code that translates to "don't call this ioctl() with some
>> particular argument if it's driver W less than version X, because the
>> driver had a bug that is likely to panic the guy's PC".
> 
> Well, not even panics per se, but some thing like the VBI is broken, or
> the volume control doesn't work, IR blaster is works for this version,
> or something else stupid that is very visible to the end user.
> 
> I also use the driver version for troubleshooting problem with users.  I
> roughly know what wasn't working in what version of the cx18 and ivtv
> drivers.  If the end user can tell me the driver version (using v4l2-ctl
> --log-status) along with his symptoms, it makes my life easier.  Being
> able to efficiently help the end user is a win for both me and the end
> user.

If you add it to MODULE_VERSION, you can get the version with:

$ modinfo -F version vivi
0.8.1

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
Stefan Richter June 24, 2011, 10:39 p.m. UTC | #11
On Jun 24 Devin Heitmueller wrote:
> On Fri, Jun 24, 2011 at 5:20 PM, Stefan Richter
> <stefanr@s5r6.in-berlin.de> wrote:
> > Easier:
> >  "I run Ubuntu 10.4".
> >  "I run kernel 2.6.32."
> > One of these is usually already included in the first post or IRC message
> > from the user.
> >
> > Separate driver versions are only needed on platforms where drivers are
> > not distributed by the operating system distributor, or driver source code
> > is not released within kernel source code.
> 
> Unfortunately, this doesn't work as all too often the user has "Ubuntu
> 10.1 but I installed the latest media_build tree a few months ago".
> Hence they are not necessarily on a particular binary release from a
> distro but rather have a mix of a distro's binary release and a
> v4l-dvb tree compiled from source.

If you release out-of-kernel-source driver sources for compilation against
binary kernels, and you have got users who go through this procedure, then
the user can for sure tell you the SCM version of the driver.

Besides, isn't this outdated practice in times where Joe Enduser can get
the very latest -rc kernel prepackaged on many distributions, including
ones like Ubuntu?

[Sorry, I'm getting perhaps a bit off-topic.]
Andy Walls June 24, 2011, 10:57 p.m. UTC | #12
On Fri, 2011-06-24 at 19:16 -0300, Mauro Carvalho Chehab wrote:
> Em 24-06-2011 18:04, Andy Walls escreveu:
> > On Fri, 2011-06-24 at 14:48 -0400, Devin Heitmueller wrote:
> >> On Fri, Jun 24, 2011 at 2:34 PM, Stefan Richter
> >> <stefanr@s5r6.in-berlin.de> wrote:
> >>> If the "driver version" is in fact an ABI version, then the driver author
> >>> should really increase it only when ABI behavior is changed (and only if
> >>> the behavior change can only be communicated by version number --- e.g.
> >>> addition of an ioctl is not among such reasons).  And the author should
> >>> commit behavior changing implementation and version number change in a
> >>> single changeset.
> >>>
> >>> And anybody who backmerges such an ABI behavior change into another kernel
> >>> branch (stable, longterm, distro...) must backmerge the associated version
> >>> number change too.
> >>>
> >>> Of course sometimes people realize this only after the fact.  Or driver
> >>> authors don't have a clear understanding of ABI versioning to begin with.
> >>> I am saying so because I had to learn it too; I certainly wasn't born
> >>> with an instinct knowledge how to do it properly.
> >>>
> >>> (Disclaimer:  I have no stake in drivers/media/ ABIs.  But I am involved
> >>> in maintaining a userspace ABI elsewhere in drivers/firewire/, and one of
> >>> the userspace libraries that use this ABI.)
> >>
> >> Hi Stefan,
> >>
> >> To be clear, I don't think anyone is actually proposing that the
> >> driver version number really be used as any form of formal "ABI
> >> versioning" scheme.  In almost all cases, it's so the application can
> >> know to *not* do something is the driver is older than X.
> > 
> > MythTV, for example, used to use the driver version to work around old
> > VBI bugs and MPEG encoder quirks that the older version of the driver
> > may not have known how to handle:
> > 
> > https://github.com/MythTV/mythtv/blob/b98d3a98e3187000ae652df5ffebe2beb5221ba7/mythtv/libs/libmythtv/mpegrecorder.cpp#L335
> > 
> > But for newer versions, MythTV could avoid using its own odd hacks.
> > The bleeding edge MythTV now has most of these removed.
> 
> Removing it is a good thing.
> 
> >> Really, this is all about applications being able to jam a hack into
> >> their code that translates to "don't call this ioctl() with some
> >> particular argument if it's driver W less than version X, because the
> >> driver had a bug that is likely to panic the guy's PC".
> > 
> > Well, not even panics per se, but some thing like the VBI is broken, or
> > the volume control doesn't work, IR blaster is works for this version,
> > or something else stupid that is very visible to the end user.
> > 
> > I also use the driver version for troubleshooting problem with users.  I
> > roughly know what wasn't working in what version of the cx18 and ivtv
> > drivers.  If the end user can tell me the driver version (using v4l2-ctl
> > --log-status) along with his symptoms, it makes my life easier.  Being
> > able to efficiently help the end user is a win for both me and the end
> > user.
> 
> If you add it to MODULE_VERSION, you can get the version with:
> 
> $ modinfo -F version vivi
> 0.8.1

Well, since you mention it....

http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/cx18/cx18-driver.c;h=9e2f870f4258665ae6093c762f752d45147a8c98;hb=staging/for_v3.1#l252
http://git.linuxtv.org/media_tree.git?a=blob;f=drivers/media/video/ivtv/ivtv-driver.c;h=0fb75524484d909af4925c3c33c9f12cf6d6519e;hb=staging/for_v3.1#l280

However, since I often must ask for the output of v4l2-ctl --log-status,
which already has the version number, I never need to ask the user to
run /sbin/modinfo for the version.

Regards,
Andy



--
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
Mauro Carvalho Chehab June 24, 2011, 11:02 p.m. UTC | #13
Em 24-06-2011 19:39, Stefan Richter escreveu:
> On Jun 24 Devin Heitmueller wrote:
>> On Fri, Jun 24, 2011 at 5:20 PM, Stefan Richter
>> <stefanr@s5r6.in-berlin.de> wrote:
>>> Easier:
>>>  "I run Ubuntu 10.4".
>>>  "I run kernel 2.6.32."
>>> One of these is usually already included in the first post or IRC message
>>> from the user.
>>>
>>> Separate driver versions are only needed on platforms where drivers are
>>> not distributed by the operating system distributor, or driver source code
>>> is not released within kernel source code.
>>
>> Unfortunately, this doesn't work as all too often the user has "Ubuntu
>> 10.1 but I installed the latest media_build tree a few months ago".
>> Hence they are not necessarily on a particular binary release from a
>> distro but rather have a mix of a distro's binary release and a
>> v4l-dvb tree compiled from source.
> 
> If you release out-of-kernel-source driver sources for compilation against
> binary kernels, and you have got users who go through this procedure, then
> the user can for sure tell you the SCM version of the driver.

Yes, and this is currently provided. The dmesg will show the last 3 git commits.
A developer can just use git diff or git log to discover what changed since those
commits.

> Besides, isn't this outdated practice in times where Joe Enduser can get
> the very latest -rc kernel prepackaged on many distributions, including
> ones like Ubuntu?

Perhaps, but the cost to maintain the out-of-tree driver git tree is cheap. We provide
just a small building system, with a script that downloads a daily tarball
with just drivers/media and the corresponding includes (and a few drivers/staging).
The building system has a couple patches to allow backport compilation since 2.6.32.

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 mbox

Patch

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 213ba7d..61ac6bf 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>
 
@@ -605,6 +606,7 @@  static long __video_do_ioctl(struct file *file,
 		if (!ops->vidioc_querycap)
 			break;
 
+		cap->version = LINUX_VERSION_CODE;
 		ret = ops->vidioc_querycap(file, fh, cap);
 		if (!ret)
 			dbgarg(cmd, "driver=%s, card=%s, bus=%s, "