[PATCH v2 0/4] MC v2 cleanups

[PATCH v2 0/4] MC v2 cleanups

[Sakari Ailus]

Hi folks,

I've made small updates to the MC v2 cleanup set. v1 is here:

<URL:http://www.spinics.net/lists/linux-media/msg97694.html>

These changes are, since v1:

- Don't align the G_TOPOLOGY array argument structs to a power of 2, but 8
  bytes

- Move media_get_uptr() macro to media-device.c instead of removing it

-- 
Kind regards,
Sakari

[PATCH v2 1/4] media: Sanitise the reserved fields of the G_TOPOLOGY IOCTL arguments

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@iki.fi>

The argument structs are used in arrays for G_TOPOLOGY IOCTL. The
arguments themselves do not need to be aligned to a power of two, but
aligning them up to the largest basic type alignment (u64) on common ABIs
is a good thing to do.

The patch changes the size of the reserved fields to 8 or 9 u32's and
aligns the size of the struct to 8 bytes so we do no longer depend on the
compiler to perform the alignment.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/uapi/linux/media.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 6aac2f0..1468651 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -302,7 +302,7 @@ struct media_v2_entity {
 	__u32 id;
 	char name[64];		/* FIXME: move to a property? (RFC says so) */
 	__u32 function;		/* Main function of the entity */
-	__u16 reserved[12];
+	__u32 reserved[8];
 };
 
 /* Should match the specific fields at media_intf_devnode */
@@ -327,7 +327,7 @@ struct media_v2_pad {
 	__u32 id;
 	__u32 entity_id;
 	__u32 flags;
-	__u16 reserved[9];
+	__u32 reserved[9];
 };
 
 struct media_v2_link {
@@ -335,7 +335,7 @@ struct media_v2_link {
 	__u32 source_id;
 	__u32 sink_id;
 	__u32 flags;
-	__u32 reserved[5];
+	__u32 reserved[8];
 };
 
 struct media_v2_topology {
-- 
2.1.4

Re: [PATCH v2 1/4] media: Sanitise the reserved fields of the G_TOPOLOGY IOCTL arguments

[Mauro Carvalho Chehab]

Em Mon, 22 Feb 2016 22:47:01 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> The argument structs are used in arrays for G_TOPOLOGY IOCTL. The
> arguments themselves do not need to be aligned to a power of two, but
> aligning them up to the largest basic type alignment (u64) on common ABIs
> is a good thing to do.
> 
> The patch changes the size of the reserved fields to 8 or 9 u32's and
> aligns the size of the struct to 8 bytes so we do no longer depend on the
> compiler to perform the alignment.

I ran some tests with both x86_64 and arch64 running both 32 and 64 bits
userspace versions of mc_nextgen_test.

Everything is working fine with the current structures. No need for any
extra alignment or compat32 bits.

So, this patch is not needed. Yet, I agree that it could be useful to
do 64 bits alignment, but I guess we're reserving too much space.

So, except for media_v2_interface, I would be reserving 5 or 6 u32 space,
as it is likely mor than enough for future usage.

See below.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/uapi/linux/media.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 6aac2f0..1468651 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -302,7 +302,7 @@ struct media_v2_entity {
>  	__u32 id;
>  	char name[64];		/* FIXME: move to a property? (RFC says so) */
>  	__u32 function;		/* Main function of the entity */
> -	__u16 reserved[12];
> +	__u32 reserved[8];

You extended the size here. Any reason? If not
I would use, instead:
	__u32 reserved[6];

>  };
>  
>  /* Should match the specific fields at media_intf_devnode */
> @@ -327,7 +327,7 @@ struct media_v2_pad {
>  	__u32 id;
>  	__u32 entity_id;
>  	__u32 flags;
> -	__u16 reserved[9];
> +	__u32 reserved[9];

Again, you're doubling the reserved space here. Any reason? If not,
I would use, instead:
	__u32 reserved[5];

>  };
>  
>  struct media_v2_link {
> @@ -335,7 +335,7 @@ struct media_v2_link {
>  	__u32 source_id;
>  	__u32 sink_id;
>  	__u32 flags;
> -	__u32 reserved[5];
> +	__u32 reserved[8];

Again, you're doubling the reserved space here. Any reason? If not,
I would use, instead:

	__u32 reserved[6];

>  };
>  
>  struct media_v2_topology {


-- 
Thanks,
Mauro

[PATCH v2.1 1/4] media: Sanitise the reserved fields of the G_TOPOLOGY IOCTL arguments

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@iki.fi>

The argument structs are used in arrays for G_TOPOLOGY IOCTL. The
arguments themselves do not need to be aligned to a power of two, but
aligning them up to the largest basic type alignment (u64) on common ABIs
is a good thing to do.

The patch changes the size of the reserved fields to 5 or 6 u32's and
aligns the size of the struct to 8 bytes so we do no longer depend on the
compiler to perform the alignment.

While at it, add __attribute__ ((packed)) to these structs as well.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v2:

- Use 5 or 6 __u32's for alignment instead of 8 or 9, except for struct
  media_v2_interface.

- Add __attribute__ ((packed)).

 include/uapi/linux/media.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 95e126e..860932f 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -319,14 +319,14 @@ struct media_v2_entity {
 	__u32 id;
 	char name[64];		/* FIXME: move to a property? (RFC says so) */
 	__u32 function;		/* Main function of the entity */
-	__u16 reserved[12];
-};
+	__u32 reserved[6];
+} __attribute__ ((packed));
 
 /* Should match the specific fields at media_intf_devnode */
 struct media_v2_intf_devnode {
 	__u32 major;
 	__u32 minor;
-};
+} __attribute__ ((packed));
 
 struct media_v2_interface {
 	__u32 id;
@@ -338,22 +338,22 @@ struct media_v2_interface {
 		struct media_v2_intf_devnode devnode;
 		__u32 raw[16];
 	};
-};
+} __attribute__ ((packed));
 
 struct media_v2_pad {
 	__u32 id;
 	__u32 entity_id;
 	__u32 flags;
-	__u16 reserved[9];
-};
+	__u32 reserved[5];
+} __attribute__ ((packed));
 
 struct media_v2_link {
 	__u32 id;
 	__u32 source_id;
 	__u32 sink_id;
 	__u32 flags;
-	__u32 reserved[5];
-};
+	__u32 reserved[6];
+} __attribute__ ((packed));
 
 struct media_v2_topology {
 	__u64 topology_version;
@@ -373,7 +373,7 @@ struct media_v2_topology {
 	__u32 num_links;
 	__u32 reserved4;
 	__u64 ptr_links;
-};
+} __attribute__ ((packed));
 
 static inline void __user *media_get_uptr(__u64 arg)
 {
-- 
2.1.4

[PATCH v2 2/4] media: Rearrange the fields in the G_TOPOLOGY IOCTL argument

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@iki.fi>

This avoids having multiple reserved fields in the struct. Reserved fields
are added in order to align the struct size to a power of two as well.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/uapi/linux/media.h | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 1468651..65991df 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -341,21 +341,16 @@ struct media_v2_link {
 struct media_v2_topology {
 	__u64 topology_version;
 
-	__u32 num_entities;
-	__u32 reserved1;
 	__u64 ptr_entities;
-
-	__u32 num_interfaces;
-	__u32 reserved2;
 	__u64 ptr_interfaces;
-
-	__u32 num_pads;
-	__u32 reserved3;
 	__u64 ptr_pads;
+	__u64 ptr_links;
 
+	__u32 num_entities;
+	__u32 num_interfaces;
+	__u32 num_pads;
 	__u32 num_links;
-	__u32 reserved4;
-	__u64 ptr_links;
+	__u32 reserved[18];
 };
 
 static inline void __user *media_get_uptr(__u64 arg)
-- 
2.1.4

Re: [PATCH v2 2/4] media: Rearrange the fields in the G_TOPOLOGY IOCTL argument

[Mauro Carvalho Chehab]

Em Mon, 22 Feb 2016 22:47:02 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> This avoids having multiple reserved fields in the struct. Reserved fields
> are added in order to align the struct size to a power of two as well.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/uapi/linux/media.h | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 1468651..65991df 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -341,21 +341,16 @@ struct media_v2_link {
>  struct media_v2_topology {
>  	__u64 topology_version;
>  
> -	__u32 num_entities;
> -	__u32 reserved1;
>  	__u64 ptr_entities;
> -
> -	__u32 num_interfaces;
> -	__u32 reserved2;
>  	__u64 ptr_interfaces;
> -
> -	__u32 num_pads;
> -	__u32 reserved3;
>  	__u64 ptr_pads;
> +	__u64 ptr_links;
>  
> +	__u32 num_entities;
> +	__u32 num_interfaces;
> +	__u32 num_pads;
>  	__u32 num_links;
> -	__u32 reserved4;
> -	__u64 ptr_links;
> +	__u32 reserved[18];
>  };

First of all, there's no need to add reserved fields here. As we've
discussed last year. If we need to grow the number of fields, we
can use the same model as the drivers/input/evdev.c does.
Something like:

#define MASK_SIZE(nr)	((nr) & ~(_IOC_SIZEMASK << _IOC_SIZESHIFT))

	size = _IOC_SIZE(cmd);
	switch (MASK_SIZE(cmd)) {
	case MASK_SIZE(MEDIA_IOC_G_TOPOLOGY):
		media_device_get_topology(dev,
					  (struct media_v2_topology __user *)arg,
					  size);

This warrants binary compatibility if newer versions of the Kernel comes
with a bigger structure.

The problem on using:

struct media_v2_topology {
  	__u64 topology_version;
  	__u64 ptr_entities;
  	__u64 ptr_interfaces;
  	__u64 ptr_pads;
 	__u64 ptr_links;
  
 	__u32 num_entities;
 	__u32 num_interfaces;
 	__u32 num_pads;
  	__u32 num_links;
};

Is that, if we add a new type (for example "foo"), we would have:

struct media_v2_topology {
  	__u64 topology_version;
  	__u64 ptr_entities;
  	__u64 ptr_interfaces;
  	__u64 ptr_pads;
 	__u64 ptr_links;
  
 	__u32 num_entities;
 	__u32 num_interfaces;
 	__u32 num_pads;
  	__u32 num_links;

	__u64 ptr_foo;
	__u32 num_foo;
};

Then, on a next addition for "bar", we would have:

struct media_v2_topology {
  	__u64 topology_version;
  	__u64 ptr_entities;
  	__u64 ptr_interfaces;
  	__u64 ptr_pads;
 	__u64 ptr_links;
  
 	__u32 num_entities;
 	__u32 num_interfaces;
 	__u32 num_pads;
  	__u32 num_links;

	__u64 ptr_foo;
	__u32 num_foo;
	__u32 num_bar;
	__u64 ptr_bar;
};

So, it becomes messy at the addition of new fields.

On the other hand, the way it currently is:

struct media_v2_topology {
	__u64 topology_version;

	__u32 num_entities;
	__u32 reserved1;
	__u64 ptr_entities;

	__u32 num_interfaces;
	__u32 reserved2;
	__u64 ptr_interfaces;

	__u32 num_pads;
	__u32 reserved3;
	__u64 ptr_pads;

	__u32 num_links;
	__u32 reserved4;
	__u64 ptr_links;
};

Adding "foo":

struct media_v2_topology {
	__u64 topology_version;

	__u32 num_entities;
	__u32 reserved1;
	__u64 ptr_entities;

	__u32 num_interfaces;
	__u32 reserved2;
	__u64 ptr_interfaces;

	__u32 num_pads;
	__u32 reserved3;
	__u64 ptr_pads;

	__u32 num_links;
	__u32 reserved4;
	__u64 ptr_links;

	__u32 num_foo;
	__u32 reserved5;
	__u64 ptr_foo;
};

Adding bar:

struct media_v2_topology {
	__u64 topology_version;

	__u32 num_entities;
	__u32 reserved1;
	__u64 ptr_entities;

	__u32 num_interfaces;
	__u32 reserved2;
	__u64 ptr_interfaces;

	__u32 num_pads;
	__u32 reserved3;
	__u64 ptr_pads;

	__u32 num_links;
	__u32 reserved4;
	__u64 ptr_links;

	__u32 num_foo;
	__u32 reserved5;
	__u64 ptr_foo;

	__u32 num_bar;
	__u32 reserved6;
	__u64 ptr_bar;
};

If all your concern is with regards to "reserved[1..n]", then maybe
we could, instead, call them as "align_foo" or "reserved_foo", like:

struct media_v2_topology {
	__u64 topology_version;

	__u32 num_entities;
	__u32 align_entities;	/* not used, should be filled with 0 by userspace */
	__u64 ptr_entities;

	__u32 num_interfaces;
	__u32 align_interfaces;	/* not used, should be filled with 0 by userspace */
	__u64 ptr_interfaces;

	__u32 num_pads;
	__u32 align_pads;	/* not used, should be filled with 0 by userspace */
	__u64 ptr_pads;

	__u32 num_links;
	__u32 align_links;	/* not used, should be filled with 0 by userspace */
	__u64 ptr_links;
};

-- 
Thanks,
Mauro

[PATCH v2 3/4] media: Properly handle user pointers

[Sakari Ailus]

Mark pointers containing user pointers as such.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/media-device.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 5ebb3cd..f001c27 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -245,10 +245,10 @@ static long __media_device_get_topology(struct media_device *mdev,
 	struct media_interface *intf;
 	struct media_pad *pad;
 	struct media_link *link;
-	struct media_v2_entity kentity, *uentity;
-	struct media_v2_interface kintf, *uintf;
-	struct media_v2_pad kpad, *upad;
-	struct media_v2_link klink, *ulink;
+	struct media_v2_entity kentity, __user *uentity;
+	struct media_v2_interface kintf, __user *uintf;
+	struct media_v2_pad kpad, __user *upad;
+	struct media_v2_link klink, __user *ulink;
 	unsigned int i;
 	int ret = 0;
 
-- 
2.1.4

Re: [PATCH v2 3/4] media: Properly handle user pointers

[Mauro Carvalho Chehab]

Em Mon, 22 Feb 2016 22:47:03 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Mark pointers containing user pointers as such.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Looks OK to me.

> ---
>  drivers/media/media-device.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 5ebb3cd..f001c27 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -245,10 +245,10 @@ static long __media_device_get_topology(struct media_device *mdev,
>  	struct media_interface *intf;
>  	struct media_pad *pad;
>  	struct media_link *link;
> -	struct media_v2_entity kentity, *uentity;
> -	struct media_v2_interface kintf, *uintf;
> -	struct media_v2_pad kpad, *upad;
> -	struct media_v2_link klink, *ulink;
> +	struct media_v2_entity kentity, __user *uentity;
> +	struct media_v2_interface kintf, __user *uintf;
> +	struct media_v2_pad kpad, __user *upad;
> +	struct media_v2_link klink, __user *ulink;
>  	unsigned int i;
>  	int ret = 0;
>  


-- 
Thanks,
Mauro

[PATCH v2 4/4] media: Move media_get_uptr() macro out of the media.h user space header

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@iki.fi>

The media_get_uptr() macro is mostly useful only for the IOCTL handling
code in media-device.c so move it there.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 5 +++++
 include/uapi/linux/media.h   | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index f001c27..39afba0 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -38,6 +38,11 @@
  * Userspace API
  */
 
+static inline void __user *media_get_uptr(__u64 arg)
+{
+	return (void __user *)(uintptr_t)arg;
+}
+
 static int media_device_open(struct file *filp)
 {
 	return 0;
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 65991df..b989494 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -353,11 +353,6 @@ struct media_v2_topology {
 	__u32 reserved[18];
 };
 
-static inline void __user *media_get_uptr(__u64 arg)
-{
-	return (void __user *)(uintptr_t)arg;
-}
-
 /* ioctls */
 
 #define MEDIA_IOC_DEVICE_INFO		_IOWR('|', 0x00, struct media_device_info)
-- 
2.1.4

Re: [PATCH v2 4/4] media: Move media_get_uptr() macro out of the media.h user space header

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Monday 22 February 2016 22:47:04 Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> The media_get_uptr() macro is mostly useful only for the IOCTL handling
> code in media-device.c so move it there.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

> ---
>  drivers/media/media-device.c | 5 +++++
>  include/uapi/linux/media.h   | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index f001c27..39afba0 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -38,6 +38,11 @@
>   * Userspace API
>   */
> 
> +static inline void __user *media_get_uptr(__u64 arg)
> +{
> +	return (void __user *)(uintptr_t)arg;
> +}
> +
>  static int media_device_open(struct file *filp)
>  {
>  	return 0;
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 65991df..b989494 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -353,11 +353,6 @@ struct media_v2_topology {
>  	__u32 reserved[18];
>  };
> 
> -static inline void __user *media_get_uptr(__u64 arg)
> -{
> -	return (void __user *)(uintptr_t)arg;
> -}
> -
>  /* ioctls */
> 
>  #define MEDIA_IOC_DEVICE_INFO		_IOWR('|', 0x00, struct 
media_device_info)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] media: Move media_get_uptr() macro out of the media.h user space header

[Mauro Carvalho Chehab]

Em Mon, 22 Feb 2016 22:47:04 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> The media_get_uptr() macro is mostly useful only for the IOCTL handling
> code in media-device.c so move it there.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Looks OK to me.

> ---
>  drivers/media/media-device.c | 5 +++++
>  include/uapi/linux/media.h   | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index f001c27..39afba0 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -38,6 +38,11 @@
>   * Userspace API
>   */
>  
> +static inline void __user *media_get_uptr(__u64 arg)
> +{
> +	return (void __user *)(uintptr_t)arg;
> +}
> +
>  static int media_device_open(struct file *filp)
>  {
>  	return 0;
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 65991df..b989494 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -353,11 +353,6 @@ struct media_v2_topology {
>  	__u32 reserved[18];
>  };
>  
> -static inline void __user *media_get_uptr(__u64 arg)
> -{
> -	return (void __user *)(uintptr_t)arg;
> -}
> -
>  /* ioctls */
>  
>  #define MEDIA_IOC_DEVICE_INFO		_IOWR('|', 0x00, struct media_device_info)


-- 
Thanks,
Mauro