[RFC 0/4] MC v2 cleanups

[RFC 0/4] MC v2 cleanups

[Sakari Ailus]

Hi all,

I have a few RFC patches to clean up the recent MC v2 API additions.

Let me know what you think.

At least the compat IOCTL handling requires some work still in general (MC
v2 or not).

-- 
Kind regards,
Sakari

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

[Sakari Ailus]

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

Align them up to a power of two.

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

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 6aac2f0..008d077 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[14];
 };
 
 /* Should match the specific fields at media_intf_devnode */
@@ -315,7 +315,7 @@ struct media_v2_interface {
 	__u32 id;
 	__u32 intf_type;
 	__u32 flags;
-	__u32 reserved[9];
+	__u32 reserved[13];
 
 	union {
 		struct media_v2_intf_devnode devnode;
@@ -327,7 +327,7 @@ struct media_v2_pad {
 	__u32 id;
 	__u32 entity_id;
 	__u32 flags;
-	__u16 reserved[9];
+	__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[4];
 };
 
 struct media_v2_topology {
-- 
2.1.4

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

[Mauro Carvalho Chehab]

Em Sun, 21 Feb 2016 23:36:12 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Align them up to a power of two.

Looks OK to me.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/uapi/linux/media.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 6aac2f0..008d077 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[14];
>  };
>  
>  /* Should match the specific fields at media_intf_devnode */
> @@ -315,7 +315,7 @@ struct media_v2_interface {
>  	__u32 id;
>  	__u32 intf_type;
>  	__u32 flags;
> -	__u32 reserved[9];
> +	__u32 reserved[13];
>  
>  	union {
>  		struct media_v2_intf_devnode devnode;
> @@ -327,7 +327,7 @@ struct media_v2_pad {
>  	__u32 id;
>  	__u32 entity_id;
>  	__u32 flags;
> -	__u16 reserved[9];
> +	__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[4];
>  };
>  
>  struct media_v2_topology {


-- 
Thanks,
Mauro

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

[Mauro Carvalho Chehab]

Em Sun, 21 Feb 2016 23:36:12 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Align them up to a power of two.

Looks OK to me, but I would comment that the structs are aligned to
2^n for those structs.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/uapi/linux/media.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 6aac2f0..008d077 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[14];
>  };
>  
>  /* Should match the specific fields at media_intf_devnode */
> @@ -315,7 +315,7 @@ struct media_v2_interface {
>  	__u32 id;
>  	__u32 intf_type;
>  	__u32 flags;
> -	__u32 reserved[9];
> +	__u32 reserved[13];
>  
>  	union {
>  		struct media_v2_intf_devnode devnode;
> @@ -327,7 +327,7 @@ struct media_v2_pad {
>  	__u32 id;
>  	__u32 entity_id;
>  	__u32 flags;
> -	__u16 reserved[9];
> +	__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[4];
>  };
>  
>  struct media_v2_topology {


-- 
Thanks,
Mauro

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

[Mauro Carvalho Chehab]

Em Mon, 22 Feb 2016 07:00:47 -0300
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> Em Sun, 21 Feb 2016 23:36:12 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> > Align them up to a power of two.  
> 
> Looks OK to me, but I would comment that the structs are aligned to
> 2^n for those structs.

Hmm... on a second tought, I don't think this patch makes any sense.
As those structs will be part of an array at media_v2_topology,
this won't be aligned to a power of two, as we don't require that
the number of links, entities, etc.. to be a aligned.

Regards,
Mauro

> 
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/uapi/linux/media.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index 6aac2f0..008d077 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[14];
> >  };
> >  
> >  /* Should match the specific fields at media_intf_devnode */
> > @@ -315,7 +315,7 @@ struct media_v2_interface {
> >  	__u32 id;
> >  	__u32 intf_type;
> >  	__u32 flags;
> > -	__u32 reserved[9];
> > +	__u32 reserved[13];
> >  
> >  	union {
> >  		struct media_v2_intf_devnode devnode;
> > @@ -327,7 +327,7 @@ struct media_v2_pad {
> >  	__u32 id;
> >  	__u32 entity_id;
> >  	__u32 flags;
> > -	__u16 reserved[9];
> > +	__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[4];
> >  };
> >  
> >  struct media_v2_topology {  
> 
> 


-- 
Thanks,
Mauro

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

[Sakari Ailus]

Hi Mauro,

On Mon, Feb 22, 2016 at 07:23:21AM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 22 Feb 2016 07:00:47 -0300
> Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:
> 
> > Em Sun, 21 Feb 2016 23:36:12 +0200
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > 
> > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > 
> > > Align them up to a power of two.  
> > 
> > Looks OK to me, but I would comment that the structs are aligned to
> > 2^n for those structs.
> 
> Hmm... on a second tought, I don't think this patch makes any sense.
> As those structs will be part of an array at media_v2_topology,
> this won't be aligned to a power of two, as we don't require that
> the number of links, entities, etc.. to be a aligned.

Well... that's a valid point indeed. Still I think the reserved fields do
need some changes, at least aligning the size to the common ABI alignment
(e.g. 8 bytes) rather than relying on the compiler to do it.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[RFC 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 008d077..77a95db 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: [RFC 2/4] media: Rearrange the fields in the G_TOPOLOGY IOCTL argument

[Mauro Carvalho Chehab]

Em Sun, 21 Feb 2016 23:36:13 +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 008d077..77a95db 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];
>  };

This patch deserves more discussion. I suggest we discuss it on our
next IRC meeting about the MC (likely today).

>  
>  static inline void __user *media_get_uptr(__u64 arg)


-- 
Thanks,
Mauro

[RFC 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>
---
 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: [RFC 3/4] media: Properly handle user pointers

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Sunday 21 February 2016 23:36:14 Sakari Ailus wrote:
> 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;

-- 
Regards,

Laurent Pinchart

[RFC 4/4] media: Drop media_get_uptr() macro

[Sakari Ailus]

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

There's no real need for such a macro, especially not in the user space
header.

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

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index f001c27..8a20383 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -256,7 +256,7 @@ static long __media_device_get_topology(struct media_device *mdev,
 
 	/* Get entities and number of entities */
 	i = 0;
-	uentity = media_get_uptr(topo->ptr_entities);
+	uentity = (void __user *)(uintptr_t)topo->ptr_entities;
 	media_device_for_each_entity(entity, mdev) {
 		i++;
 		if (ret || !uentity)
@@ -282,7 +282,7 @@ static long __media_device_get_topology(struct media_device *mdev,
 
 	/* Get interfaces and number of interfaces */
 	i = 0;
-	uintf = media_get_uptr(topo->ptr_interfaces);
+	uintf = (void __user *)(uintptr_t)topo->ptr_interfaces;
 	media_device_for_each_intf(intf, mdev) {
 		i++;
 		if (ret || !uintf)
@@ -317,7 +317,7 @@ static long __media_device_get_topology(struct media_device *mdev,
 
 	/* Get pads and number of pads */
 	i = 0;
-	upad = media_get_uptr(topo->ptr_pads);
+	upad = (void __user *)(uintptr_t)topo->ptr_pads;
 	media_device_for_each_pad(pad, mdev) {
 		i++;
 		if (ret || !upad)
@@ -343,7 +343,7 @@ static long __media_device_get_topology(struct media_device *mdev,
 
 	/* Get links and number of links */
 	i = 0;
-	ulink = media_get_uptr(topo->ptr_links);
+	ulink = (void __user *)(uintptr_t)topo->ptr_links;
 	media_device_for_each_link(link, mdev) {
 		if (link->is_backlink)
 			continue;
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 77a95db..f4f7897 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: [RFC 4/4] media: Drop media_get_uptr() macro

[Mauro Carvalho Chehab]

Em Sun, 21 Feb 2016 23:36:15 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> There's no real need for such a macro, especially not in the user space
> header.

Ok, good point, but I would, instead, move the macro to
drivers/media/media-device.c. That double-casting is something unusual,
and we don't want to start receiving patch from newbie janitors wanting
to strip the casts.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-device.c | 8 ++++----
>  include/uapi/linux/media.h   | 5 -----
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index f001c27..8a20383 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -256,7 +256,7 @@ static long __media_device_get_topology(struct media_device *mdev,
>  
>  	/* Get entities and number of entities */
>  	i = 0;
> -	uentity = media_get_uptr(topo->ptr_entities);
> +	uentity = (void __user *)(uintptr_t)topo->ptr_entities;
>  	media_device_for_each_entity(entity, mdev) {
>  		i++;
>  		if (ret || !uentity)
> @@ -282,7 +282,7 @@ static long __media_device_get_topology(struct media_device *mdev,
>  
>  	/* Get interfaces and number of interfaces */
>  	i = 0;
> -	uintf = media_get_uptr(topo->ptr_interfaces);
> +	uintf = (void __user *)(uintptr_t)topo->ptr_interfaces;
>  	media_device_for_each_intf(intf, mdev) {
>  		i++;
>  		if (ret || !uintf)
> @@ -317,7 +317,7 @@ static long __media_device_get_topology(struct media_device *mdev,
>  
>  	/* Get pads and number of pads */
>  	i = 0;
> -	upad = media_get_uptr(topo->ptr_pads);
> +	upad = (void __user *)(uintptr_t)topo->ptr_pads;
>  	media_device_for_each_pad(pad, mdev) {
>  		i++;
>  		if (ret || !upad)
> @@ -343,7 +343,7 @@ static long __media_device_get_topology(struct media_device *mdev,
>  
>  	/* Get links and number of links */
>  	i = 0;
> -	ulink = media_get_uptr(topo->ptr_links);
> +	ulink = (void __user *)(uintptr_t)topo->ptr_links;
>  	media_device_for_each_link(link, mdev) {
>  		if (link->is_backlink)
>  			continue;
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 77a95db..f4f7897 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

Re: [RFC 4/4] media: Drop media_get_uptr() macro

[Sakari Ailus]

Hi Mauro,

On Mon, Feb 22, 2016 at 06:52:51AM -0300, Mauro Carvalho Chehab wrote:
> Em Sun, 21 Feb 2016 23:36:15 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> > There's no real need for such a macro, especially not in the user space
> > header.
> 
> Ok, good point, but I would, instead, move the macro to
> drivers/media/media-device.c. That double-casting is something unusual,
> and we don't want to start receiving patch from newbie janitors wanting
> to strip the casts.

Ok, I'll move it to media-device.c then.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk