[PATCHv3 0/6] media: drop driver_version from media_device

[PATCHv3 0/6] media: drop driver_version from media_device

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Just a little thing that always annoyed me: the driver_version shouldn't
be set in drivers.

The version number never, ever gets updated in drivers. We saw that in
the other media subsystems and now the core always sets it, not drivers.

This works much better, and also works well when backporting the media
code to an older kernel using the media_build system, where the driver
version is set to the kernel version you are backporting from.

So just set the driver_version in media_device_get_info() to
LINUX_VERSION_CODE and drop the driver_version field from struct
media_device.

In addition do the same with media_version, that too is never updated
when it should.

Regards,

	Hans

Changes since v2:

- remove obsolete comment about driver_version from media/media-device.h
- add patch to make the same change for media_version

Changes since v1:

- just set driver_version in media_device_get_info() and drop it
  in struct media_device.
- combine two lines in atomisp_v4l2.c

Hans Verkuil (6):
  media-device: set driver_version directly
  s3c-camif: don't set driver_version
  uvc: don't set driver_version
  atomisp2: don't set driver_version
  media-device: remove driver_version
  media: drop use of MEDIA_API_VERSION

 drivers/media/media-device.c                              | 6 +-----
 drivers/media/platform/s3c-camif/camif-core.c             | 1 -
 drivers/media/usb/uvc/uvc_driver.c                        | 1 -
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c | 6 +-----
 include/media/media-device.h                              | 7 -------
 include/uapi/linux/media.h                                | 5 +++--
 6 files changed, 5 insertions(+), 21 deletions(-)

-- 
2.13.2

[PATCHv3 1/6] media-device: set driver_version directly

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Don't use driver_version from struct media_device, just return
LINUX_VERSION_CODE as the other media subsystems do.

The driver_version field in struct media_device will be removed
in the following patches.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/media-device.c | 2 +-
 include/media/media-device.h | 5 -----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index fce91b543c14..7ff8e2d5bb07 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -71,7 +71,7 @@ static int media_device_get_info(struct media_device *dev,
 
 	info->media_version = MEDIA_API_VERSION;
 	info->hw_revision = dev->hw_revision;
-	info->driver_version = dev->driver_version;
+	info->driver_version = LINUX_VERSION_CODE;
 
 	return 0;
 }
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 6896266031b9..7ae200d89a9f 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -249,11 +249,6 @@ void media_device_cleanup(struct media_device *mdev);
  *    driver-specific format. When possible the revision should be formatted
  *    with the KERNEL_VERSION() macro.
  *
- *  - &media_entity.driver_version is formatted with the KERNEL_VERSION()
- *    macro. The version minor must be incremented when new features are added
- *    to the userspace API without breaking binary compatibility. The version
- *    major must be incremented when binary compatibility is broken.
- *
  * .. note::
  *
  *    #) Upon successful registration a character device named media[0-9]+ is created. The device major and minor numbers are dynamic. The model name is exported as a sysfs attribute.
-- 
2.13.2

[PATCHv3 2/6] s3c-camif: don't set driver_version

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This field will be removed as it is not needed anymore.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 drivers/media/platform/s3c-camif/camif-core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/s3c-camif/camif-core.c b/drivers/media/platform/s3c-camif/camif-core.c
index 8f0414041e81..c4ab63986c8f 100644
--- a/drivers/media/platform/s3c-camif/camif-core.c
+++ b/drivers/media/platform/s3c-camif/camif-core.c
@@ -317,7 +317,6 @@ static int camif_media_dev_init(struct camif_dev *camif)
 		 ip_rev == S3C6410_CAMIF_IP_REV ? "6410" : "244X");
 	strlcpy(md->bus_info, "platform", sizeof(md->bus_info));
 	md->hw_revision = ip_rev;
-	md->driver_version = LINUX_VERSION_CODE;
 
 	md->dev = camif->dev;
 
-- 
2.13.2

[PATCHv3 3/6] uvc: don't set driver_version

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This field will be removed as it is not needed anymore.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 70842c5af05b..4f463bf2b877 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2096,7 +2096,6 @@ static int uvc_probe(struct usb_interface *intf,
 			sizeof(dev->mdev.serial));
 	strcpy(dev->mdev.bus_info, udev->devpath);
 	dev->mdev.hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
-	dev->mdev.driver_version = LINUX_VERSION_CODE;
 	media_device_init(&dev->mdev);
 
 	dev->vdev.mdev = &dev->mdev;
-- 
2.13.2

[PATCHv3 4/6] atomisp2: don't set driver_version

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This field will be removed as it is not needed anymore.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
index 2f49562377e6..663aa916e3ca 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
@@ -1099,9 +1099,7 @@ atomisp_load_firmware(struct atomisp_device *isp)
 		fw_path = "shisp_2400b0_v21.bin";
 
 	if (!fw_path) {
-		dev_err(isp->dev,
-			"Unsupported driver_version 0x%x, hw_revision 0x%x\n",
-			isp->media_dev.driver_version,
+		dev_err(isp->dev, "Unsupported hw_revision 0x%x\n",
 			isp->media_dev.hw_revision);
 		return NULL;
 	}
@@ -1249,8 +1247,6 @@ static int atomisp_pci_probe(struct pci_dev *dev,
 	/* This is not a true PCI device on SoC, so the delay is not needed. */
 	isp->pdev->d3_delay = 0;
 
-	isp->media_dev.driver_version = LINUX_VERSION_CODE;
-
 	switch (id->device & ATOMISP_PCI_DEVICE_SOC_MASK) {
 	case ATOMISP_PCI_DEVICE_SOC_MRFLD:
 		isp->media_dev.hw_revision =
-- 
2.13.2

[PATCHv3 5/6] media-device: remove driver_version

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Since the driver_version field in struct media_device is no longer
used, just remove it.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/media-device.c | 3 ---
 include/media/media-device.h | 2 --
 2 files changed, 5 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 7ff8e2d5bb07..979e4307d248 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -833,8 +833,6 @@ void media_device_pci_init(struct media_device *mdev,
 	mdev->hw_revision = (pci_dev->subsystem_vendor << 16)
 			    | pci_dev->subsystem_device;
 
-	mdev->driver_version = LINUX_VERSION_CODE;
-
 	media_device_init(mdev);
 }
 EXPORT_SYMBOL_GPL(media_device_pci_init);
@@ -862,7 +860,6 @@ void __media_device_usb_init(struct media_device *mdev,
 		strlcpy(mdev->serial, udev->serial, sizeof(mdev->serial));
 	usb_make_path(udev, mdev->bus_info, sizeof(mdev->bus_info));
 	mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
-	mdev->driver_version = LINUX_VERSION_CODE;
 
 	media_device_init(mdev);
 }
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 7ae200d89a9f..bcc6ec434f1f 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -68,7 +68,6 @@ struct media_device_ops {
  * @serial:	Device serial number (optional)
  * @bus_info:	Unique and stable device location identifier
  * @hw_revision: Hardware device revision
- * @driver_version: Device driver version
  * @topology_version: Monotonic counter for storing the version of the graph
  *		topology. Should be incremented each time the topology changes.
  * @id:		Unique ID used on the last registered graph object
@@ -134,7 +133,6 @@ struct media_device {
 	char serial[40];
 	char bus_info[32];
 	u32 hw_revision;
-	u32 driver_version;
 
 	u64 topology_version;
 
-- 
2.13.2

[PATCHv3 6/6] media: drop use of MEDIA_API_VERSION

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Set media_version to LINUX_VERSION_CODE, just as we did for
driver_version.

Nobody ever rememebers to update the version number, but
LINUX_VERSION_CODE will always be updated.

Move the MEDIA_API_VERSION define to the ifndef __KERNEL__ section of the
media.h header. That way kernelspace can't accidentally start to use
it again.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 3 +--
 include/uapi/linux/media.h   | 5 +++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 979e4307d248..3c99294e3ebf 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -69,9 +69,8 @@ static int media_device_get_info(struct media_device *dev,
 	strlcpy(info->serial, dev->serial, sizeof(info->serial));
 	strlcpy(info->bus_info, dev->bus_info, sizeof(info->bus_info));
 
-	info->media_version = MEDIA_API_VERSION;
+	info->media_version = info->driver_version = LINUX_VERSION_CODE;
 	info->hw_revision = dev->hw_revision;
-	info->driver_version = LINUX_VERSION_CODE;
 
 	return 0;
 }
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index fac96c64fe51..4865f1e71339 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -30,8 +30,6 @@
 #include <linux/types.h>
 #include <linux/version.h>
 
-#define MEDIA_API_VERSION	KERNEL_VERSION(0, 1, 0)
-
 struct media_device_info {
 	char driver[16];
 	char model[32];
@@ -187,6 +185,9 @@ struct media_device_info {
 #define MEDIA_ENT_T_V4L2_SUBDEV_LENS	MEDIA_ENT_F_LENS
 #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	MEDIA_ENT_F_ATV_DECODER
 #define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	MEDIA_ENT_F_TUNER
+
+/* Obsolete symbol for media_version, no longer used in the kernel */
+#define MEDIA_API_VERSION		KERNEL_VERSION(0, 1, 0)
 #endif
 
 /* Entity flags */
-- 
2.13.2

Re: [PATCHv3 6/6] media: drop use of MEDIA_API_VERSION

[Mauro Carvalho Chehab]

Em Sat, 22 Jul 2017 13:30:57 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Set media_version to LINUX_VERSION_CODE, just as we did for
> driver_version.
> 
> Nobody ever rememebers to update the version number, but
> LINUX_VERSION_CODE will always be updated.
> 
> Move the MEDIA_API_VERSION define to the ifndef __KERNEL__ section of the
> media.h header. That way kernelspace can't accidentally start to use
> it again.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/media-device.c | 3 +--
>  include/uapi/linux/media.h   | 5 +++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 979e4307d248..3c99294e3ebf 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -69,9 +69,8 @@ static int media_device_get_info(struct media_device *dev,
>  	strlcpy(info->serial, dev->serial, sizeof(info->serial));
>  	strlcpy(info->bus_info, dev->bus_info, sizeof(info->bus_info));
>  
> -	info->media_version = MEDIA_API_VERSION;
> +	info->media_version = info->driver_version = LINUX_VERSION_CODE;
>  	info->hw_revision = dev->hw_revision;
> -	info->driver_version = LINUX_VERSION_CODE;
>  
>  	return 0;
>  }
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index fac96c64fe51..4865f1e71339 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -30,8 +30,6 @@
>  #include <linux/types.h>
>  #include <linux/version.h>
>  
> -#define MEDIA_API_VERSION	KERNEL_VERSION(0, 1, 0)
> -
>  struct media_device_info {
>  	char driver[16];
>  	char model[32];
> @@ -187,6 +185,9 @@ struct media_device_info {
>  #define MEDIA_ENT_T_V4L2_SUBDEV_LENS	MEDIA_ENT_F_LENS
>  #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	MEDIA_ENT_F_ATV_DECODER
>  #define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	MEDIA_ENT_F_TUNER
> +
> +/* Obsolete symbol for media_version, no longer used in the kernel */
> +#define MEDIA_API_VERSION		KERNEL_VERSION(0, 1, 0)

IMHO, it should, instead be identical to LINUX_VERSION_CODE, as
applications might be relying on it in order to check what
media API version they receive from the MC queries.

The problem is that this macro is defined only internally inside
the Kernel tree.

>  #endif
>  
>  /* Entity flags */



Thanks,
Mauro

Re: [PATCHv3 6/6] media: drop use of MEDIA_API_VERSION

[Hans Verkuil]

On 22/07/17 15:24, Mauro Carvalho Chehab wrote:
> Em Sat, 22 Jul 2017 13:30:57 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Set media_version to LINUX_VERSION_CODE, just as we did for
>> driver_version.
>>
>> Nobody ever rememebers to update the version number, but
>> LINUX_VERSION_CODE will always be updated.
>>
>> Move the MEDIA_API_VERSION define to the ifndef __KERNEL__ section of the
>> media.h header. That way kernelspace can't accidentally start to use
>> it again.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/media-device.c | 3 +--
>>  include/uapi/linux/media.h   | 5 +++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 979e4307d248..3c99294e3ebf 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -69,9 +69,8 @@ static int media_device_get_info(struct media_device *dev,
>>  	strlcpy(info->serial, dev->serial, sizeof(info->serial));
>>  	strlcpy(info->bus_info, dev->bus_info, sizeof(info->bus_info));
>>  
>> -	info->media_version = MEDIA_API_VERSION;
>> +	info->media_version = info->driver_version = LINUX_VERSION_CODE;
>>  	info->hw_revision = dev->hw_revision;
>> -	info->driver_version = LINUX_VERSION_CODE;
>>  
>>  	return 0;
>>  }
>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>> index fac96c64fe51..4865f1e71339 100644
>> --- a/include/uapi/linux/media.h
>> +++ b/include/uapi/linux/media.h
>> @@ -30,8 +30,6 @@
>>  #include <linux/types.h>
>>  #include <linux/version.h>
>>  
>> -#define MEDIA_API_VERSION	KERNEL_VERSION(0, 1, 0)
>> -
>>  struct media_device_info {
>>  	char driver[16];
>>  	char model[32];
>> @@ -187,6 +185,9 @@ struct media_device_info {
>>  #define MEDIA_ENT_T_V4L2_SUBDEV_LENS	MEDIA_ENT_F_LENS
>>  #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	MEDIA_ENT_F_ATV_DECODER
>>  #define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	MEDIA_ENT_F_TUNER
>> +
>> +/* Obsolete symbol for media_version, no longer used in the kernel */
>> +#define MEDIA_API_VERSION		KERNEL_VERSION(0, 1, 0)
> 
> IMHO, it should, instead be identical to LINUX_VERSION_CODE, as
> applications might be relying on it in order to check what
> media API version they receive from the MC queries.

That's useless. The only reason you want to use media_version in an application
is to check whether a feature is present provided you know for which kernel
version it appeared. So you will explicitly compare it against e.g. 4.x.0.
Never against LINUX_VERSION_CODE.

In fact, any application using this today will do something like:

media_version >= MEDIA_API_VERSION

and that should keep working in the future. Since any kernel release is >= 0.1.0
this will always work.

media_version >= LINUX_VERSION_CODE is dangerous since this define comes from
a userspace header (/usr/include/linux/version.h) which may be newer/older than the
actual running kernel. So the result of "media_version >= LINUX_VERSION_CODE" is
effectively undefined.

Regards,

	Hans

> 
> The problem is that this macro is defined only internally inside
> the Kernel tree.
> 
>>  #endif
>>  
>>  /* Entity flags */
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCHv3 6/6] media: drop use of MEDIA_API_VERSION

[Laurent Pinchart]

Hi Hans,

On Saturday 22 Jul 2017 13:30:57 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Set media_version to LINUX_VERSION_CODE, just as we did for
> driver_version.
> 
> Nobody ever rememebers to update the version number, but
> LINUX_VERSION_CODE will always be updated.
> 
> Move the MEDIA_API_VERSION define to the ifndef __KERNEL__ section of the
> media.h header. That way kernelspace can't accidentally start to use
> it again.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/media-device.c | 3 +--
>  include/uapi/linux/media.h   | 5 +++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 979e4307d248..3c99294e3ebf 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -69,9 +69,8 @@ static int media_device_get_info(struct media_device *dev,
> strlcpy(info->serial, dev->serial, sizeof(info->serial));
>  	strlcpy(info->bus_info, dev->bus_info, sizeof(info->bus_info));
> 
> -	info->media_version = MEDIA_API_VERSION;
> +	info->media_version = info->driver_version = LINUX_VERSION_CODE;

I'd split this on two lines, I believe it would be clearer. Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	info->hw_revision = dev->hw_revision;
> -	info->driver_version = LINUX_VERSION_CODE;
> 
>  	return 0;
>  }
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index fac96c64fe51..4865f1e71339 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -30,8 +30,6 @@
>  #include <linux/types.h>
>  #include <linux/version.h>
> 
> -#define MEDIA_API_VERSION	KERNEL_VERSION(0, 1, 0)
> -
>  struct media_device_info {
>  	char driver[16];
>  	char model[32];
> @@ -187,6 +185,9 @@ struct media_device_info {
>  #define MEDIA_ENT_T_V4L2_SUBDEV_LENS	MEDIA_ENT_F_LENS
>  #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	MEDIA_ENT_F_ATV_DECODER
>  #define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	MEDIA_ENT_F_TUNER
> +
> +/* Obsolete symbol for media_version, no longer used in the kernel */
> +#define MEDIA_API_VERSION		KERNEL_VERSION(0, 1, 0)
>  #endif
> 
>  /* Entity flags */

-- 
Regards,

Laurent Pinchart