[RFC v2 0/2] V4L2 IOCTL enum compat wrapper

[RFC v2 0/2] V4L2 IOCTL enum compat wrapper

[Sakari Ailus]

Hi all,

This is my first intended-to-be-complete RFC patch to get rid of the enums
in V4L2 IOCTL structs. The approach is the one outlined first by Mauro
(AFAIR):

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

A set of compat structs (and compat IOCTLs) are created and there are a few
functions to convert between the in-kernel representation and the old
representation with enums the user space may well be using. On many archs
the two IOCTLs are actually the same.

This patchset depends on my earlier patch to remove v4l2_buffer.input:

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

All three patches are also available here:

<URL:http://git.linuxtv.org/sailus/media_tree.git/shortlog/refs/heads/enum-fix>

Open questions:

- Orring the return values of {get,put}_user etc. is time-consuming on
  modern CPUs with deep pipelines. Would if be better to use | (logical or)
  instead? The regular case where the access is successful would be
  optimised on the expense of the error case. The end result in error cases
  may be different, too, but does that matter?

- Testing this patch completely is difficult. I've only got access to
  capture hardware on 32-bit systems which generally do not easily exhibit
  problems with enums in IOCTLs (since they're 32-bit ints) in first place.
  I've tested this by changing fields from __u32 to __u64 where the old code
  had enums; that works, so at least something is working correctly. Help in
  testing would be very much appreciated.

Questions and comments are welcome.

Kind regards,

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

[RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Sakari Ailus]

Replace enums in IOCTL structs by __u32. The size of enums is variable and
thus problematic. Compatibility structs having exactly the same as original
definition are provided for compatibility with old binaries with the
required conversion code.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/linux/videodev2.h  |   42 +++++-----
 include/media/v4l2-ioctl.h |  209 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 230 insertions(+), 21 deletions(-)

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index fed1d40..ec0928d 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -292,10 +292,10 @@ struct v4l2_pix_format {
 	__u32         		width;
 	__u32			height;
 	__u32			pixelformat;
-	enum v4l2_field  	field;
+	__u32			field;		/* enum v4l2_field */
 	__u32            	bytesperline;	/* for padding, zero if unused */
 	__u32          		sizeimage;
-	enum v4l2_colorspace	colorspace;
+	__u32			colorspace;	/* enum v4l2_colorspace */
 	__u32			priv;		/* private data, depends on pixelformat */
 };
 
@@ -432,7 +432,7 @@ struct v4l2_pix_format {
  */
 struct v4l2_fmtdesc {
 	__u32		    index;             /* Format number      */
-	enum v4l2_buf_type  type;              /* buffer type        */
+	__u32		    type;	       /* buffer type (enum v4l2_buf_type) */
 	__u32               flags;
 	__u8		    description[32];   /* Description string */
 	__u32		    pixelformat;       /* Format fourcc      */
@@ -573,8 +573,8 @@ struct v4l2_jpegcompression {
  */
 struct v4l2_requestbuffers {
 	__u32			count;
-	enum v4l2_buf_type      type;
-	enum v4l2_memory        memory;
+	__u32		      type;		/* enum v4l2_buf_type */
+	__u32		        memory;		/* enum v4l2_memory */
 	__u32			reserved[2];
 };
 
@@ -636,16 +636,16 @@ struct v4l2_plane {
  */
 struct v4l2_buffer {
 	__u32			index;
-	enum v4l2_buf_type      type;
+	__u32			type;		/* enum v4l2_buf_type */
 	__u32			bytesused;
 	__u32			flags;
-	enum v4l2_field		field;
+	__u32			field;		/* enum v4l2_field */
 	struct timeval		timestamp;
 	struct v4l2_timecode	timecode;
 	__u32			sequence;
 
 	/* memory location */
-	enum v4l2_memory        memory;
+	__u32		        memory;		/* enum v4l2_memory */
 	union {
 		__u32           offset;
 		unsigned long   userptr;
@@ -707,7 +707,7 @@ struct v4l2_clip {
 
 struct v4l2_window {
 	struct v4l2_rect        w;
-	enum v4l2_field  	field;
+	__u32			field;		/* enum v4l2_field */
 	__u32			chromakey;
 	struct v4l2_clip	__user *clips;
 	__u32			clipcount;
@@ -744,14 +744,14 @@ struct v4l2_outputparm {
  *	I N P U T   I M A G E   C R O P P I N G
  */
 struct v4l2_cropcap {
-	enum v4l2_buf_type      type;
+	__u32			type;		/* enum v4l2_buf_type */
 	struct v4l2_rect        bounds;
 	struct v4l2_rect        defrect;
 	struct v4l2_fract       pixelaspect;
 };
 
 struct v4l2_crop {
-	enum v4l2_buf_type      type;
+	__u32			type;		/* enum v4l2_buf_type */
 	struct v4l2_rect        c;
 };
 
@@ -1156,7 +1156,7 @@ enum v4l2_ctrl_type {
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
 struct v4l2_queryctrl {
 	__u32		     id;
-	enum v4l2_ctrl_type  type;
+	__u32		     type;	/* enum v4l2_ctrl_type */
 	__u8		     name[32];	/* Whatever */
 	__s32		     minimum;	/* Note signedness */
 	__s32		     maximum;
@@ -1791,7 +1791,7 @@ enum v4l2_jpeg_chroma_subsampling {
 struct v4l2_tuner {
 	__u32                   index;
 	__u8			name[32];
-	enum v4l2_tuner_type    type;
+	__u32			type;		/* enum v4l2_tuner_type */
 	__u32			capability;
 	__u32			rangelow;
 	__u32			rangehigh;
@@ -1841,14 +1841,14 @@ struct v4l2_modulator {
 
 struct v4l2_frequency {
 	__u32		      tuner;
-	enum v4l2_tuner_type  type;
+	__u32		      type;		/* enum v4l2_tuner_type */
 	__u32		      frequency;
 	__u32		      reserved[8];
 };
 
 struct v4l2_hw_freq_seek {
 	__u32		      tuner;
-	enum v4l2_tuner_type  type;
+	__u32		      type;		/* enum v4l2_tuner_type */
 	__u32		      seek_upward;
 	__u32		      wrap_around;
 	__u32		      spacing;
@@ -2059,7 +2059,7 @@ struct v4l2_sliced_vbi_cap {
 				 (equals frame lines 313-336 for 625 line video
 				  standards, 263-286 for 525 line standards) */
 	__u16   service_lines[2][24];
-	enum v4l2_buf_type type;
+	__u32	 type;		/* enum v4l2_buf_type */
 	__u32   reserved[3];    /* must be 0 */
 };
 
@@ -2149,8 +2149,8 @@ struct v4l2_pix_format_mplane {
 	__u32				width;
 	__u32				height;
 	__u32				pixelformat;
-	enum v4l2_field			field;
-	enum v4l2_colorspace		colorspace;
+	__u32				field;		/* enum v4l2_field */
+	__u32				colorspace;	/* enum v4l2_colorspace */
 
 	struct v4l2_plane_pix_format	plane_fmt[VIDEO_MAX_PLANES];
 	__u8				num_planes;
@@ -2168,7 +2168,7 @@ struct v4l2_pix_format_mplane {
  * @raw_data:	placeholder for future extensions and custom formats
  */
 struct v4l2_format {
-	enum v4l2_buf_type type;
+	__u32	type;		/* enum v4l2_buf_type */
 	union {
 		struct v4l2_pix_format		pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
 		struct v4l2_pix_format_mplane	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
@@ -2182,7 +2182,7 @@ struct v4l2_format {
 /*	Stream type-dependent parameters
  */
 struct v4l2_streamparm {
-	enum v4l2_buf_type type;
+	__u32	type;		/* enum v4l2_buf_type */
 	union {
 		struct v4l2_captureparm	capture;
 		struct v4l2_outputparm	output;
@@ -2302,7 +2302,7 @@ struct v4l2_dbg_chip_ident {
 struct v4l2_create_buffers {
 	__u32			index;
 	__u32			count;
-	enum v4l2_memory        memory;
+	__u32		        memory;		/* enum v4l2_memory */
 	struct v4l2_format	format;
 	__u32			reserved[8];
 };
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index 3cb939c..77018d8 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -333,4 +333,213 @@ extern long video_usercopy(struct file *file, unsigned int cmd,
 extern long video_ioctl2(struct file *file,
 			unsigned int cmd, unsigned long arg);
 
+/*
+ * Backward-compatible IOCTL's to be used by V4L2 core to work with the
+ * old ioctl's defined with "enum's" inside the structures
+ */
+
+#ifdef CONFIG_V4L2_COMPAT
+
+struct v4l2_pix_format_enum {
+	__u32			width;
+	__u32			height;
+	__u32			pixelformat;
+	enum v4l2_field		field;
+	__u32			bytesperline;	/* for padding, zero if unused */
+	__u32			sizeimage;
+	enum v4l2_colorspace	colorspace;
+	__u32			priv;		/* private data, depends on pixelformat */
+};
+
+struct v4l2_fmtdesc_enum {
+	__u32			index;             /* Format number      */
+	enum v4l2_buf_type	type;              /* buffer type        */
+	__u32			flags;
+	__u8			description[32];   /* Description string */
+	__u32			pixelformat;       /* Format fourcc      */
+	__u32			reserved[4];
+};
+
+struct v4l2_requestbuffers_enum {
+	__u32			count;
+	enum v4l2_buf_type	type;
+	enum v4l2_memory	memory;
+	__u32			reserved[2];
+};
+
+struct v4l2_buffer_enum {
+	__u32			index;
+	enum v4l2_buf_type	type;
+	__u32			bytesused;
+	__u32			flags;
+	enum v4l2_field		field;
+	struct timeval		timestamp;
+	struct v4l2_timecode	timecode;
+	__u32			sequence;
+
+	/* memory location */
+	enum v4l2_memory	memory;
+	union {
+		__u32		offset;
+		unsigned long	userptr;
+		struct v4l2_plane *planes;
+	} m;
+	__u32			length;
+	__u32			reserved2;
+	__u32			reserved;
+};
+
+struct v4l2_framebuffer_enum {
+	__u32			capability;
+	__u32			flags;
+/* FIXME: in theory we should pass something like PCI device + memory
+ * region + offset instead of some physical address */
+	void			*base;
+	struct v4l2_pix_format_enum fmt;
+};
+
+struct v4l2_window_enum {
+	struct v4l2_rect	w;
+	enum v4l2_field		field;
+	__u32			chromakey;
+	struct v4l2_clip	__user *clips;
+	__u32			clipcount;
+	void			__user *bitmap;
+	__u8			global_alpha;
+};
+
+struct v4l2_cropcap_enum {
+	enum v4l2_buf_type	type;
+	struct v4l2_rect	bounds;
+	struct v4l2_rect	defrect;
+	struct v4l2_fract	pixelaspect;
+};
+
+struct v4l2_crop_enum {
+	enum v4l2_buf_type	type;
+	struct v4l2_rect	c;
+};
+
+struct v4l2_queryctrl_enum {
+	__u32			id;
+	enum v4l2_ctrl_type	type;
+	__u8			name[32];	/* Whatever */
+	__s32			minimum;	/* Note signedness */
+	__s32			maximum;
+	__s32			step;
+	__s32			default_value;
+	__u32			flags;
+	__u32			reserved[2];
+};
+
+struct v4l2_tuner_enum {
+	__u32			index;
+	__u8			name[32];
+	enum v4l2_tuner_type	type;
+	__u32			capability;
+	__u32			rangelow;
+	__u32			rangehigh;
+	__u32			rxsubchans;
+	__u32			audmode;
+	__s32			signal;
+	__s32			afc;
+	__u32			reserved[4];
+};
+
+struct v4l2_frequency_enum {
+	__u32			tuner;
+	enum v4l2_tuner_type	type;
+	__u32			frequency;
+	__u32			reserved[8];
+};
+
+struct v4l2_hw_freq_seek_enum {
+	__u32			tuner;
+	enum v4l2_tuner_type	type;
+	__u32			seek_upward;
+	__u32			wrap_around;
+	__u32			spacing;
+	__u32			reserved[7];
+};
+
+struct v4l2_sliced_vbi_cap_enum {
+	__u16	service_set;
+	/* service_lines[0][...] specifies lines 0-23 (1-23 used) of the first field
+	   service_lines[1][...] specifies lines 0-23 (1-23 used) of the second field
+				 (equals frame lines 313-336 for 625 line video
+				  standards, 263-286 for 525 line standards) */
+	__u16	service_lines[2][24];
+	enum v4l2_buf_type type;
+	__u32	reserved[3];    /* must be 0 */
+};
+
+struct v4l2_pix_format_mplane_enum {
+	__u32				width;
+	__u32				height;
+	__u32				pixelformat;
+	enum v4l2_field			field;
+	enum v4l2_colorspace		colorspace;
+
+	struct v4l2_plane_pix_format	plane_fmt[VIDEO_MAX_PLANES];
+	__u8				num_planes;
+	__u8				reserved[11];
+} __packed;
+
+struct v4l2_format_enum {
+	enum v4l2_buf_type type;
+	union {
+		struct v4l2_pix_format_enum	pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
+		struct v4l2_pix_format_mplane_enum	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
+		struct v4l2_window_enum		win;     /* V4L2_BUF_TYPE_VIDEO_OVERLAY */
+		struct v4l2_vbi_format		vbi;     /* V4L2_BUF_TYPE_VBI_CAPTURE */
+		struct v4l2_sliced_vbi_format	sliced;  /* V4L2_BUF_TYPE_SLICED_VBI_CAPTURE */
+		__u8	raw_data[200];                   /* user-defined */
+	} fmt;
+};
+
+/*	Stream type-dependent parameters
+ */
+struct v4l2_streamparm_enum {
+	enum v4l2_buf_type type;
+	union {
+		struct v4l2_captureparm	capture;
+		struct v4l2_outputparm	output;
+		__u8	raw_data[200];  /* user-defined */
+	} parm;
+};
+
+struct v4l2_create_buffers_enum {
+	__u32			index;
+	__u32			count;
+	enum v4l2_memory	memory;
+	struct v4l2_format_enum	format;
+	__u32			reserved[8];
+};
+
+#define VIDIOC_ENUM_FMT_ENUM		_IOWR('V',  2, struct v4l2_fmtdesc_enum)
+#define VIDIOC_G_FMT_ENUM		_IOWR('V',  4, struct v4l2_format_enum)
+#define VIDIOC_S_FMT_ENUM		_IOWR('V',  5, struct v4l2_format_enum)
+#define VIDIOC_REQBUFS_ENUM		_IOWR('V',  8, struct v4l2_requestbuffers_enum)
+#define VIDIOC_QUERYBUF_ENUM		_IOWR('V',  9, struct v4l2_buffer_enum)
+#define VIDIOC_G_FBUF_ENUM		_IOR('V', 10, struct v4l2_framebuffer_enum)
+#define VIDIOC_S_FBUF_ENUM		_IOW('V', 11, struct v4l2_framebuffer_enum)
+#define VIDIOC_QBUF_ENUM		_IOWR('V', 15, struct v4l2_buffer_enum)
+#define VIDIOC_DQBUF_ENUM		_IOWR('V', 17, struct v4l2_buffer_enum)
+#define VIDIOC_G_PARM_ENUM		_IOWR('V', 21, struct v4l2_streamparm_enum)
+#define VIDIOC_S_PARM_ENUM		_IOWR('V', 22, struct v4l2_streamparm_enum)
+#define VIDIOC_G_TUNER_ENUM		_IOWR('V', 29, struct v4l2_tuner_enum)
+#define VIDIOC_S_TUNER_ENUM		_IOW('V', 30, struct v4l2_tuner_enum)
+#define VIDIOC_QUERYCTRL_ENUM		_IOWR('V', 36, struct v4l2_queryctrl_enum)
+#define VIDIOC_G_FREQUENCY_ENUM		_IOWR('V', 56, struct v4l2_frequency_enum)
+#define VIDIOC_S_FREQUENCY_ENUM		_IOW('V', 57, struct v4l2_frequency_enum)
+#define VIDIOC_CROPCAP_ENUM		_IOWR('V', 58, struct v4l2_cropcap_enum)
+#define VIDIOC_G_CROP_ENUM		_IOWR('V', 59, struct v4l2_crop_enum)
+#define VIDIOC_S_CROP_ENUM		_IOW('V', 60, struct v4l2_crop_enum)
+#define VIDIOC_TRY_FMT_ENUM		_IOWR('V', 64, struct v4l2_format_enum)
+#define VIDIOC_G_SLICED_VBI_CAP_ENUM	_IOWR('V', 69, struct v4l2_sliced_vbi_cap_enum)
+#define VIDIOC_S_HW_FREQ_SEEK_ENUM	_IOW('V', 82, struct v4l2_hw_freq_seek_enum)
+#define VIDIOC_CREATE_BUFS_ENUM		_IOWR('V', 92, struct v4l2_create_buffers_enum)
+#define VIDIOC_PREPARE_BUF_ENUM		_IOWR('V', 93, struct v4l2_buffer_enum)
+#endif /* CONFIG_V4L2_COMPAT */
+
 #endif /* _V4L2_IOCTL_H */
-- 
1.7.2.5

[RFC v3 2/2] v4l: Implement compat functions for enum to __u32 change

[Sakari Ailus]

Implement compat functions to provide conversion between structs containing
enums and those not. The functions are intended to be removed when the
support for such old binaries is no longer necessary.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/Kconfig      |   12 +
 drivers/media/video/v4l2-ioctl.c |  684 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 687 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index f2479c5..949f804 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -7,6 +7,18 @@ config VIDEO_V4L2
 	depends on VIDEO_DEV && VIDEO_V4L2_COMMON
 	default VIDEO_DEV && VIDEO_V4L2_COMMON
 
+config V4L2_COMPAT
+	bool "Compatibility for old binaries"
+	depends on VIDEO_V4L2
+	default y
+	---help---
+	  Compatibility code to support binaries compiled with old V4L2
+	  IOCTL definitions containing enums that have been replaced by
+	  __u32. If you do not need to use such binaries you can disable
+	  this option.
+
+	  When in doubt, say Y.
+
 config VIDEOBUF_GEN
 	tristate
 
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 5b2ec1f..9b88360 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -2303,6 +2303,653 @@ static long __video_do_ioctl(struct file *file,
 	return ret;
 }
 
+#ifdef CONFIG_V4L2_COMPAT
+
+static inline unsigned int get_non_compat_cmd(unsigned int cmd)
+{
+	switch (cmd) {
+	case VIDIOC_ENUM_FMT_ENUM:
+		return VIDIOC_ENUM_FMT;
+	case VIDIOC_G_FMT_ENUM:
+		return VIDIOC_G_FMT;
+	case VIDIOC_S_FMT_ENUM:
+		return VIDIOC_S_FMT;
+	case VIDIOC_REQBUFS_ENUM:
+		return VIDIOC_REQBUFS;
+	case VIDIOC_QUERYBUF_ENUM:
+		return VIDIOC_QUERYBUF;
+	case VIDIOC_G_FBUF_ENUM:
+		return VIDIOC_G_FBUF;
+	case VIDIOC_S_FBUF_ENUM:
+		return VIDIOC_S_FBUF;
+	case VIDIOC_QBUF_ENUM:
+		return VIDIOC_QBUF;
+	case VIDIOC_DQBUF_ENUM:
+		return VIDIOC_DQBUF;
+	case VIDIOC_G_PARM_ENUM:
+		return VIDIOC_G_PARM;
+	case VIDIOC_S_PARM_ENUM:
+		return VIDIOC_S_PARM;
+	case VIDIOC_G_TUNER_ENUM:
+		return VIDIOC_G_TUNER;
+	case VIDIOC_S_TUNER_ENUM:
+		return VIDIOC_S_TUNER;
+	case VIDIOC_QUERYCTRL_ENUM:
+		return VIDIOC_QUERYCTRL;
+	case VIDIOC_G_FREQUENCY_ENUM:
+		return VIDIOC_G_FREQUENCY;
+	case VIDIOC_S_FREQUENCY_ENUM:
+		return VIDIOC_S_FREQUENCY;
+	case VIDIOC_CROPCAP_ENUM:
+		return VIDIOC_CROPCAP;
+	case VIDIOC_G_CROP_ENUM:
+		return VIDIOC_G_CROP;
+	case VIDIOC_S_CROP_ENUM:
+		return VIDIOC_S_CROP;
+	case VIDIOC_TRY_FMT_ENUM:
+		return VIDIOC_TRY_FMT;
+	case VIDIOC_G_SLICED_VBI_CAP_ENUM:
+		return VIDIOC_G_SLICED_VBI_CAP;
+	case VIDIOC_S_HW_FREQ_SEEK_ENUM:
+		return VIDIOC_S_HW_FREQ_SEEK;
+	case VIDIOC_CREATE_BUFS_ENUM:
+		return VIDIOC_CREATE_BUFS;
+	case VIDIOC_PREPARE_BUF_ENUM:
+		return VIDIOC_PREPARE_BUF;
+	default:
+		return cmd;
+	}
+}
+
+#define get_user_conv(x, ptr)			\
+	({					\
+		typeof (*ptr) tmp;		\
+		int rval;			\
+						\
+		rval = get_user(tmp, ptr);	\
+		if (!rval)			\
+			x = (typeof (x))tmp;	\
+						\
+		rval;				\
+	})
+
+static int get_user_pix_format(struct v4l2_pix_format *k,
+			       struct v4l2_pix_format_enum *u)
+{
+	return get_user(k->width, &u->width)
+		|| get_user(k->height, &u->height)
+		|| get_user(k->pixelformat, &u->pixelformat)
+		|| get_user_conv(k->field, &u->field)
+		|| get_user(k->bytesperline, &u->bytesperline)
+		|| get_user(k->sizeimage, &u->sizeimage)
+		|| get_user_conv(k->colorspace, &u->colorspace)
+		|| get_user(k->priv, &u->priv);
+}
+
+static int get_user_pix_format_mplane(struct v4l2_pix_format_mplane *k,
+				      struct v4l2_pix_format_mplane_enum *u)
+{
+	return get_user(k->width, &u->width)
+		|| get_user(k->height, &u->height)
+		|| get_user(k->pixelformat, &u->pixelformat)
+		|| get_user_conv(k->field, &u->field)
+		|| get_user_conv(k->colorspace, &u->colorspace)
+		|| copy_from_user(k->plane_fmt, u->plane_fmt,
+				  sizeof(k->plane_fmt))
+		|| get_user(k->num_planes, &u->num_planes)
+		|| copy_from_user(k->reserved, u->reserved,
+				  sizeof(k->reserved));
+}
+
+static int get_user_window(struct v4l2_window *k,
+			   struct v4l2_window_enum *u)
+{
+	return copy_from_user(&k->w, &u->w, sizeof(k->w))
+		|| get_user_conv(k->field, &u->field)
+		|| get_user(k->chromakey, &u->chromakey)
+		|| get_user(k->clips, &u->clips)
+		|| get_user(k->clipcount, &u->clipcount)
+		|| get_user(k->bitmap, &u->bitmap)
+		|| get_user(k->global_alpha, &u->global_alpha);
+}
+
+static int get_user_format(struct v4l2_format *k,
+			   struct v4l2_format_enum *u)
+{
+	if (get_user(k->type, &u->type))
+		return -EFAULT;
+
+	switch (k->type) {
+	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
+	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
+		if (get_user_pix_format(&k->fmt.pix, &u->fmt.pix))
+			return -EFAULT;
+		return 0;
+	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
+	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
+		if (get_user_pix_format_mplane(&k->fmt.pix_mp, &u->fmt.pix_mp))
+			return -EFAULT;
+		return 0;
+	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
+	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
+		if (get_user_window(&k->fmt.win, &u->fmt.win))
+			return -EFAULT;
+		return 0;
+	default:
+		if (copy_from_user(k->fmt.raw_data, u->fmt.raw_data,
+				   sizeof(k->fmt.raw_data)))
+			return -EFAULT;
+		return 0;
+	}
+}
+
+static long copy_compat_from_user(unsigned int cmd, void *parg,
+				  void __user *arg)
+{
+	union {
+		struct v4l2_fmtdesc_enum fmtdesc;
+		struct v4l2_format_enum fmt;
+		struct v4l2_requestbuffers_enum reqbufs;
+		struct v4l2_framebuffer_enum fb;
+		struct v4l2_buffer_enum buf;
+		struct v4l2_streamparm_enum sparm;
+		struct v4l2_tuner_enum tuner;
+		struct v4l2_queryctrl_enum qc;
+		struct v4l2_frequency_enum freq;
+		struct v4l2_cropcap_enum cropcap;
+		struct v4l2_crop_enum crop;
+		struct v4l2_sliced_vbi_cap_enum vbi_cap;
+		struct v4l2_hw_freq_seek_enum freq_seek;
+		struct v4l2_create_buffers_enum create_bufs;
+	} __user *u = arg;
+	union {
+		struct v4l2_fmtdesc fmtdesc;
+		struct v4l2_format fmt;
+		struct v4l2_requestbuffers reqbufs;
+		struct v4l2_framebuffer fb;
+		struct v4l2_buffer buf;
+		struct v4l2_streamparm sparm;
+		struct v4l2_tuner tuner;
+		struct v4l2_queryctrl qc;
+		struct v4l2_frequency freq;
+		struct v4l2_cropcap cropcap;
+		struct v4l2_crop crop;
+		struct v4l2_sliced_vbi_cap vbi_cap;
+		struct v4l2_hw_freq_seek freq_seek;
+		struct v4l2_create_buffers create_bufs;
+	} *k = parg;
+
+	if (!access_ok(VERIFY_READ, u, _IOC_SIZE(cmd)))
+		return -EFAULT;
+
+	switch (cmd) {
+	case VIDIOC_ENUM_FMT_ENUM:
+		if (get_user(k->fmtdesc.index, &u->fmtdesc.index)
+		    || get_user_conv(k->fmtdesc.type, &u->fmtdesc.type)
+		    || get_user(k->fmtdesc.flags, &u->fmtdesc.flags)
+		    || copy_from_user(k->fmtdesc.description,
+				      u->fmtdesc.description,
+				      sizeof(k->fmtdesc.description))
+		    || get_user(k->fmtdesc.pixelformat,
+				&u->fmtdesc.pixelformat)
+		    || copy_from_user(k->fmtdesc.reserved,
+				      u->fmtdesc.reserved,
+				      sizeof(k->fmtdesc.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_FMT_ENUM:
+	case VIDIOC_S_FMT_ENUM:
+	case VIDIOC_TRY_FMT_ENUM:
+		if (get_user_format(&k->fmt, &u->fmt))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_REQBUFS_ENUM:
+		if (get_user(k->reqbufs.count, &u->reqbufs.count)
+		    || get_user_conv(k->reqbufs.type, &u->reqbufs.type)
+		    || get_user_conv(k->reqbufs.memory, &u->reqbufs.memory)
+		    || copy_from_user(k->reqbufs.reserved,
+				      u->reqbufs.reserved,
+				      sizeof(k->reqbufs.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_QUERYBUF_ENUM:
+	case VIDIOC_QBUF_ENUM:
+	case VIDIOC_DQBUF_ENUM:
+	case VIDIOC_PREPARE_BUF_ENUM:
+		if (get_user(k->buf.index, &u->buf.index)
+		    || get_user_conv(k->buf.type, &u->buf.type)
+		    || get_user(k->buf.bytesused, &u->buf.bytesused)
+		    || get_user(k->buf.flags, &u->buf.flags)
+		    || get_user_conv(k->buf.field, &u->buf.field)
+		    || copy_from_user(&k->buf.timestamp, &u->buf.timestamp,
+				      sizeof(k->buf.timestamp))
+		    || copy_from_user(&k->buf.timecode, &u->buf.timecode,
+				      sizeof(k->buf.timecode))
+		    || get_user(k->buf.sequence, &u->buf.sequence)
+		    || get_user_conv(k->buf.memory, &u->buf.memory)
+		    || copy_from_user(&k->buf.m, &u->buf.m, sizeof(k->buf.m))
+		    || get_user(k->buf.length, &u->buf.length)
+		    || get_user(k->buf.reserved2, &u->buf.reserved2)
+		    || get_user(k->buf.reserved, &u->buf.reserved))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_FBUF_ENUM:
+	case VIDIOC_S_FBUF_ENUM:
+		if (get_user(k->fb.capability, &u->fb.capability)
+		    || get_user(k->fb.flags, &u->fb.flags)
+		    || get_user(k->fb.base, &u->fb.base)
+		    || get_user_pix_format(&k->fb.fmt, &u->fb.fmt))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_PARM_ENUM:
+	case VIDIOC_S_PARM_ENUM:
+		if (get_user_conv(k->sparm.type, &u->sparm.type)
+		    || copy_from_user(&k->sparm.parm, &u->sparm.parm,
+				      sizeof(k->sparm.parm)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_TUNER_ENUM:
+	case VIDIOC_S_TUNER_ENUM:
+		if (get_user(k->tuner.index, &u->tuner.index)
+		    || copy_from_user(k->tuner.name, u->tuner.name,
+				      sizeof(k->tuner.name))
+		    || get_user_conv(k->tuner.type, &u->tuner.type)
+		    || get_user(k->tuner.capability, &u->tuner.capability)
+		    || get_user(k->tuner.rangelow, &u->tuner.rangelow)
+		    || get_user(k->tuner.rangehigh, &u->tuner.rangehigh)
+		    || get_user(k->tuner.rxsubchans, &u->tuner.rxsubchans)
+		    || get_user(k->tuner.audmode, &u->tuner.audmode)
+		    || get_user(k->tuner.signal, &u->tuner.signal)
+		    || get_user(k->tuner.afc, &u->tuner.afc)
+		    || copy_from_user(k->tuner.reserved, u->tuner.reserved,
+				      sizeof(k->tuner.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_QUERYCTRL_ENUM:
+		if (get_user(k->qc.id, &u->qc.id)
+		    || get_user_conv(k->qc.type, &u->qc.type)
+		    || copy_from_user(k->qc.name, u->qc.name,
+				      sizeof(k->qc.name))
+		    || get_user(k->qc.minimum, &u->qc.minimum)
+		    || get_user(k->qc.maximum, &u->qc.maximum)
+		    || get_user(k->qc.step, &u->qc.step)
+		    || get_user(k->qc.default_value, &u->qc.default_value)
+		    || get_user(k->qc.flags, &u->qc.flags)
+		    || copy_from_user(k->qc.reserved, u->qc.reserved,
+				      sizeof(k->qc.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_FREQUENCY_ENUM:
+	case VIDIOC_S_FREQUENCY_ENUM:
+		if (get_user(k->freq.tuner, &u->freq.tuner)
+		    || get_user_conv(k->freq.type, &u->freq.type)
+		    || get_user(k->freq.frequency, &u->freq.frequency)
+		    || copy_from_user(k->freq.reserved, u->freq.reserved,
+				      sizeof(k->freq.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_CROPCAP_ENUM:
+		if (get_user_conv(k->cropcap.type, &u->cropcap.type)
+		    || copy_from_user(&k->cropcap.bounds, &u->cropcap.bounds,
+				      sizeof(k->cropcap.bounds))
+		    || copy_from_user(&k->cropcap.defrect, &u->cropcap.defrect,
+				      sizeof(k->cropcap.bounds))
+		    || copy_from_user(&k->cropcap.pixelaspect,
+				      &u->cropcap.pixelaspect,
+				      sizeof(k->cropcap.pixelaspect)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_CROP_ENUM:
+	case VIDIOC_S_CROP_ENUM:
+		if (get_user_conv(k->crop.type, &u->crop.type)
+		    || copy_from_user(&k->crop.c, &u->crop.c,
+				      sizeof(k->crop.c)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_SLICED_VBI_CAP_ENUM:
+		if (get_user(k->vbi_cap.service_set, &u->vbi_cap.service_set)
+		    || copy_from_user(k->vbi_cap.service_lines,
+				      u->vbi_cap.service_lines,
+				      sizeof(k->vbi_cap.service_lines))
+		    || get_user_conv(k->vbi_cap.type, &u->vbi_cap.type)
+		    || copy_from_user(k->vbi_cap.reserved, u->vbi_cap.reserved,
+				      sizeof(k->vbi_cap.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_S_HW_FREQ_SEEK_ENUM:
+		if (get_user(k->freq_seek.tuner, &u->freq_seek.tuner)
+		    || get_user_conv(k->freq_seek.type, &u->freq_seek.type)
+		    || get_user(k->freq_seek.seek_upward,
+				&u->freq_seek.seek_upward)
+		    || get_user(k->freq_seek.wrap_around,
+				&u->freq_seek.wrap_around)
+		    || get_user(k->freq_seek.spacing, &u->freq_seek.spacing)
+		    || copy_from_user(k->freq_seek.reserved,
+				      u->freq_seek.reserved,
+				      sizeof(k->freq_seek.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_CREATE_BUFS_ENUM:
+		if (get_user(k->create_bufs.index, &u->create_bufs.index)
+		    || get_user(k->create_bufs.count, &u->create_bufs.count)
+		    || get_user_conv(k->create_bufs.memory,
+				     &u->create_bufs.memory)
+		    || get_user_format(&k->create_bufs.format,
+				       &u->create_bufs.format)
+		    || copy_from_user(k->create_bufs.reserved,
+				      u->create_bufs.reserved,
+				      sizeof(k->create_bufs.reserved)))
+			return -EFAULT;
+		return 0;
+	default:
+		WARN(1, "%s: bad compat cmd %8.8x\n", __func__, cmd);
+		return -EINVAL;
+	}
+}
+
+#define put_user_conv(x, ptr)			\
+	({					\
+		typeof (*ptr) tmp;		\
+						\
+		tmp = (typeof (*ptr))x;		\
+		put_user(tmp, ptr);		\
+	})
+
+static int put_user_pix_format(struct v4l2_pix_format *k,
+			       struct v4l2_pix_format_enum *u)
+{
+	return put_user(k->width, &u->width)
+		|| put_user(k->height, &u->height)
+		|| put_user(k->pixelformat, &u->pixelformat)
+		|| put_user_conv(k->field, &u->field)
+		|| put_user(k->bytesperline, &u->bytesperline)
+		|| put_user(k->sizeimage, &u->sizeimage)
+		|| put_user_conv(k->colorspace, &u->colorspace)
+		|| put_user(k->priv, &u->priv);
+}
+
+static int put_user_pix_format_mplane(struct v4l2_pix_format_mplane *k,
+				      struct v4l2_pix_format_mplane_enum *u)
+{
+	return put_user(k->width, &u->width)
+		|| put_user(k->height, &u->height)
+		|| put_user(k->pixelformat, &u->pixelformat)
+		|| put_user_conv(k->field, &u->field)
+		|| put_user_conv(k->colorspace, &u->colorspace)
+		|| copy_to_user(u->plane_fmt, k->plane_fmt,
+				sizeof(k->plane_fmt))
+		|| put_user(k->num_planes, &u->num_planes)
+		|| copy_to_user(u->reserved, k->reserved,
+				sizeof(k->reserved));
+}
+
+static int put_user_window(struct v4l2_window *k,
+			   struct v4l2_window_enum *u)
+{
+	return copy_to_user(&u->w, &k->w, sizeof(k->w))
+		|| put_user_conv(k->field, &u->field)
+		|| put_user(k->chromakey, &u->chromakey)
+		|| put_user(k->clips, &u->clips)
+		|| put_user(k->clipcount, &u->clipcount)
+		|| put_user(k->bitmap, &u->bitmap)
+		|| put_user(k->global_alpha, &u->global_alpha);
+}
+
+static int put_user_format(struct v4l2_format *k,
+			   struct v4l2_format_enum *u)
+{
+	if (put_user(k->type, &u->type))
+		return -EFAULT;
+
+	switch (k->type) {
+	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
+	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
+		if (put_user_pix_format(&k->fmt.pix, &u->fmt.pix))
+			return -EFAULT;
+		return 0;
+	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
+	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
+		if (put_user_pix_format_mplane(&k->fmt.pix_mp, &u->fmt.pix_mp))
+			return -EFAULT;
+		return 0;
+	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
+	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
+		if (put_user_window(&k->fmt.win, &u->fmt.win))
+			return -EFAULT;
+		return 0;
+	default:
+		if (copy_to_user(u->fmt.raw_data, k->fmt.raw_data,
+				 sizeof(k->fmt.raw_data)))
+			return -EFAULT;
+		return 0;
+	}
+}
+
+static long copy_compat_to_user(unsigned int cmd, void __user *arg,
+				void *parg)
+{
+	union {
+		struct v4l2_fmtdesc_enum fmtdesc;
+		struct v4l2_format_enum fmt;
+		struct v4l2_requestbuffers_enum reqbufs;
+		struct v4l2_framebuffer_enum fb;
+		struct v4l2_buffer_enum buf;
+		struct v4l2_streamparm_enum sparm;
+		struct v4l2_tuner_enum tuner;
+		struct v4l2_queryctrl_enum qc;
+		struct v4l2_frequency_enum freq;
+		struct v4l2_cropcap_enum cropcap;
+		struct v4l2_crop_enum crop;
+		struct v4l2_sliced_vbi_cap_enum vbi_cap;
+		struct v4l2_hw_freq_seek_enum freq_seek;
+		struct v4l2_create_buffers_enum create_bufs;
+	} __user *u = arg;
+	union {
+		struct v4l2_fmtdesc fmtdesc;
+		struct v4l2_format fmt;
+		struct v4l2_requestbuffers reqbufs;
+		struct v4l2_framebuffer fb;
+		struct v4l2_buffer buf;
+		struct v4l2_streamparm sparm;
+		struct v4l2_tuner tuner;
+		struct v4l2_queryctrl qc;
+		struct v4l2_frequency freq;
+		struct v4l2_cropcap cropcap;
+		struct v4l2_crop crop;
+		struct v4l2_sliced_vbi_cap vbi_cap;
+		struct v4l2_hw_freq_seek freq_seek;
+		struct v4l2_create_buffers create_bufs;
+	} *k = parg;
+
+	if (!access_ok(VERIFY_WRITE, u, _IOC_SIZE(cmd)))
+		return -EFAULT;
+
+	switch (cmd) {
+	case VIDIOC_ENUM_FMT_ENUM:
+		if (put_user(k->fmtdesc.index, &u->fmtdesc.index)
+		    || put_user_conv(k->fmtdesc.type, &u->fmtdesc.type)
+		    || put_user(k->fmtdesc.flags, &u->fmtdesc.flags)
+		    || copy_to_user(u->fmtdesc.description,
+				    k->fmtdesc.description,
+				    sizeof(k->fmtdesc.description))
+		    || put_user(k->fmtdesc.pixelformat,
+				&u->fmtdesc.pixelformat)
+		    || copy_to_user(u->fmtdesc.reserved,
+				    k->fmtdesc.reserved,
+				    sizeof(k->fmtdesc.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_FMT_ENUM:
+	case VIDIOC_S_FMT_ENUM:
+	case VIDIOC_TRY_FMT_ENUM:
+		if (put_user_format(&k->fmt, &u->fmt))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_REQBUFS_ENUM:
+		if (put_user(k->reqbufs.count, &u->reqbufs.count)
+		    || put_user_conv(k->reqbufs.type, &u->reqbufs.type)
+		    || put_user_conv(k->reqbufs.memory, &u->reqbufs.memory)
+		    || copy_to_user(u->reqbufs.reserved,
+				    k->reqbufs.reserved,
+				    sizeof(k->reqbufs.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_QUERYBUF_ENUM:
+	case VIDIOC_QBUF_ENUM:
+	case VIDIOC_DQBUF_ENUM:
+	case VIDIOC_PREPARE_BUF_ENUM:
+		if (put_user(k->buf.index, &u->buf.index)
+		    || put_user_conv(k->buf.type, &u->buf.type)
+		    || put_user(k->buf.bytesused, &u->buf.bytesused)
+		    || put_user(k->buf.flags, &u->buf.flags)
+		    || put_user_conv(k->buf.field, &u->buf.field)
+		    || copy_to_user(&u->buf.timestamp, &k->buf.timestamp,
+				    sizeof(k->buf.timestamp))
+		    || copy_to_user(&u->buf.timecode, &k->buf.timecode,
+				    sizeof(k->buf.timecode))
+		    || put_user(k->buf.sequence, &u->buf.sequence)
+		    || put_user_conv(k->buf.memory, &u->buf.memory)
+		    || copy_to_user(&u->buf.m, &k->buf.m, sizeof(k->buf.m))
+		    || put_user(k->buf.length, &u->buf.length)
+		    || put_user(k->buf.reserved2, &u->buf.reserved2)
+		    || put_user(k->buf.reserved, &u->buf.reserved))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_FBUF_ENUM:
+	case VIDIOC_S_FBUF_ENUM:
+		if (put_user(k->fb.capability, &u->fb.capability)
+		    || put_user(k->fb.flags, &u->fb.flags)
+		    || put_user(k->fb.base, &u->fb.base)
+		    || put_user_pix_format(&k->fb.fmt, &u->fb.fmt))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_PARM_ENUM:
+	case VIDIOC_S_PARM_ENUM:
+		if (put_user_conv(k->sparm.type, &u->sparm.type)
+		    || copy_to_user(&u->sparm.parm, &k->sparm.parm,
+				    sizeof(k->sparm.parm)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_TUNER_ENUM:
+	case VIDIOC_S_TUNER_ENUM:
+		if (put_user(k->tuner.index, &u->tuner.index)
+		    || copy_to_user(u->tuner.name, k->tuner.name,
+				    sizeof(k->tuner.name))
+		    || put_user_conv(k->tuner.type, &u->tuner.type)
+		    || put_user(k->tuner.capability, &u->tuner.capability)
+		    || put_user(k->tuner.rangelow, &u->tuner.rangelow)
+		    || put_user(k->tuner.rangehigh, &u->tuner.rangehigh)
+		    || put_user(k->tuner.rxsubchans, &u->tuner.rxsubchans)
+		    || put_user(k->tuner.audmode, &u->tuner.audmode)
+		    || put_user(k->tuner.signal, &u->tuner.signal)
+		    || put_user(k->tuner.afc, &u->tuner.afc)
+		    || copy_to_user(u->tuner.reserved, k->tuner.reserved,
+				    sizeof(k->tuner.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_QUERYCTRL_ENUM:
+		if (put_user(k->qc.id, &u->qc.id)
+		    || put_user_conv(k->qc.type, &u->qc.type)
+		    || copy_to_user(u->qc.name, k->qc.name,
+				    sizeof(k->qc.name))
+		    || put_user(k->qc.minimum, &u->qc.minimum)
+		    || put_user(k->qc.maximum, &u->qc.maximum)
+		    || put_user(k->qc.step, &u->qc.step)
+		    || put_user(k->qc.default_value, &u->qc.default_value)
+		    || put_user(k->qc.flags, &u->qc.flags)
+		    || copy_to_user(u->qc.reserved, k->qc.reserved,
+				    sizeof(k->qc.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_FREQUENCY_ENUM:
+	case VIDIOC_S_FREQUENCY_ENUM:
+		if (put_user(k->freq.tuner, &u->freq.tuner)
+		    || put_user_conv(k->freq.type, &u->freq.type)
+		    || put_user(k->freq.frequency, &u->freq.frequency)
+		    || copy_to_user(u->freq.reserved, k->freq.reserved,
+				    sizeof(k->freq.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_CROPCAP_ENUM:
+		if (put_user_conv(k->cropcap.type, &u->cropcap.type)
+		    || copy_to_user(&u->cropcap.bounds, &k->cropcap.bounds,
+				    sizeof(k->cropcap.bounds))
+		    || copy_to_user(&u->cropcap.defrect, &k->cropcap.defrect,
+				    sizeof(k->cropcap.bounds))
+		    || copy_to_user(&u->cropcap.pixelaspect,
+				    &k->cropcap.pixelaspect,
+				    sizeof(k->cropcap.pixelaspect)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_CROP_ENUM:
+	case VIDIOC_S_CROP_ENUM:
+		if (put_user_conv(k->crop.type, &u->crop.type)
+		    || copy_to_user(&u->crop.c, &k->crop.c,
+				    sizeof(k->crop.c)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_G_SLICED_VBI_CAP_ENUM:
+		if (put_user(k->vbi_cap.service_set, &u->vbi_cap.service_set)
+		    || copy_to_user(u->vbi_cap.service_lines,
+				    k->vbi_cap.service_lines,
+				    sizeof(k->vbi_cap.service_lines))
+		    || put_user_conv(k->vbi_cap.type, &u->vbi_cap.type)
+		    || copy_to_user(u->vbi_cap.reserved, k->vbi_cap.reserved,
+				    sizeof(k->vbi_cap.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_S_HW_FREQ_SEEK_ENUM:
+		if (put_user(k->freq_seek.tuner, &u->freq_seek.tuner)
+		    || put_user_conv(k->freq_seek.type, &u->freq_seek.type)
+		    || put_user(k->freq_seek.seek_upward,
+				&u->freq_seek.seek_upward)
+		    || put_user(k->freq_seek.wrap_around,
+				&u->freq_seek.wrap_around)
+		    || put_user(k->freq_seek.spacing, &u->freq_seek.spacing)
+		    || copy_to_user(u->freq_seek.reserved,
+				    k->freq_seek.reserved,
+				    sizeof(k->freq_seek.reserved)))
+			return -EFAULT;
+		return 0;
+	case VIDIOC_CREATE_BUFS_ENUM:
+		if (put_user(k->create_bufs.index, &u->create_bufs.index)
+		    || put_user(k->create_bufs.count, &u->create_bufs.count)
+		    || put_user_conv(k->create_bufs.memory,
+				     &u->create_bufs.memory)
+		    || put_user_format(&k->create_bufs.format,
+				       &u->create_bufs.format)
+		    || copy_to_user(u->create_bufs.reserved,
+				    k->create_bufs.reserved,
+				    sizeof(k->create_bufs.reserved)))
+			return -EFAULT;
+		return 0;
+	default:
+		WARN(1, "%s: bad compat cmd %8.8x\n", __func__, cmd);
+		return -EINVAL;
+	}
+}
+
+#else /* CONFIG_V4L2_COMPAT */
+
+static inline unsigned int get_non_compat_cmd(unsigned int cmd)
+{
+	return cmd;
+}
+
+static inline long copy_compat_from_user(unsigned int cmd, void __user *arg,
+					 void *parg)
+{
+	return 0;
+}
+
+static inline long copy_compat_to_user(unsigned int cmd, void __user *arg,
+				       void *parg)
+{
+	return 0;
+}
+
+#endif /* CONFIG_V4L2_COMPAT */
+
 /* In some cases, only a few fields are used as input, i.e. when the app sets
  * "index" and then the driver fills in the rest of the structure for the thing
  * with that index.  We only need to copy up the first non-input field.  */
@@ -2390,7 +3037,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
 }
 
 long
-video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
+video_usercopy(struct file *file, unsigned int compat_cmd, unsigned long arg,
 	       v4l2_kioctl func)
 {
 	char	sbuf[128];
@@ -2401,6 +3048,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 	size_t  array_size = 0;
 	void __user *user_ptr = NULL;
 	void	**kernel_ptr = NULL;
+	unsigned int cmd = get_non_compat_cmd(compat_cmd);
 
 	/*  Copy arguments into temp kernel buffer  */
 	if (_IOC_DIR(cmd) != _IOC_NONE) {
@@ -2418,12 +3066,23 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 		if (_IOC_DIR(cmd) & _IOC_WRITE) {
 			unsigned long n = cmd_input_size(cmd);
 
-			if (copy_from_user(parg, (void __user *)arg, n))
-				goto out;
-
-			/* zero out anything we don't copy from userspace */
-			if (n < _IOC_SIZE(cmd))
-				memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n);
+			if (cmd == compat_cmd) {
+				if (copy_from_user(
+					    parg, (void __user *)arg, n))
+					goto out;
+
+				/*
+				 * zero out anything we don't copy
+				 * from userspace
+				 */
+				if (n < _IOC_SIZE(cmd))
+					memset((u8 *)parg + n, 0,
+					       _IOC_SIZE(cmd) - n);
+			} else {
+				if (copy_compat_from_user(compat_cmd, parg,
+							  (void __user *)arg))
+					goto out;
+			}
 		} else {
 			/* read-only ioctl */
 			memset(parg, 0, _IOC_SIZE(cmd));
@@ -2471,8 +3130,15 @@ out_array_args:
 	switch (_IOC_DIR(cmd)) {
 	case _IOC_READ:
 	case (_IOC_WRITE | _IOC_READ):
-		if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
-			err = -EFAULT;
+		if (cmd == compat_cmd) {
+			if (copy_to_user((void __user *)arg, parg,
+					 _IOC_SIZE(cmd)))
+				err = -EFAULT;
+		} else {
+			if (copy_compat_to_user(compat_cmd, (void __user *)arg,
+						parg))
+				err = -EFAULT;
+		}
 		break;
 	}
 
-- 
1.7.2.5

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Hans Verkuil]

On Wed May 2 2012 21:13:47 Sakari Ailus wrote:
> Replace enums in IOCTL structs by __u32. The size of enums is variable and
> thus problematic. Compatibility structs having exactly the same as original
> definition are provided for compatibility with old binaries with the
> required conversion code.

Does someone actually have hard proof that there really is a problem? You know,
demonstrate it with actual example code?

It's pretty horrible that you have to do all those conversions and that code
will be with us for years to come.

For most (if not all!) architectures sizeof(enum) == sizeof(u32), so there is
no need for any compat code for those.

Note that I don't question that using u32 is better than using enums, but I
really wonder if there is any need for all the conversions.

Regards,

	Hans

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/linux/videodev2.h  |   42 +++++-----
>  include/media/v4l2-ioctl.h |  209 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 230 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index fed1d40..ec0928d 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -292,10 +292,10 @@ struct v4l2_pix_format {
>  	__u32         		width;
>  	__u32			height;
>  	__u32			pixelformat;
> -	enum v4l2_field  	field;
> +	__u32			field;		/* enum v4l2_field */
>  	__u32            	bytesperline;	/* for padding, zero if unused */
>  	__u32          		sizeimage;
> -	enum v4l2_colorspace	colorspace;
> +	__u32			colorspace;	/* enum v4l2_colorspace */
>  	__u32			priv;		/* private data, depends on pixelformat */
>  };
>  
> @@ -432,7 +432,7 @@ struct v4l2_pix_format {
>   */
>  struct v4l2_fmtdesc {
>  	__u32		    index;             /* Format number      */
> -	enum v4l2_buf_type  type;              /* buffer type        */
> +	__u32		    type;	       /* buffer type (enum v4l2_buf_type) */
>  	__u32               flags;
>  	__u8		    description[32];   /* Description string */
>  	__u32		    pixelformat;       /* Format fourcc      */
> @@ -573,8 +573,8 @@ struct v4l2_jpegcompression {
>   */
>  struct v4l2_requestbuffers {
>  	__u32			count;
> -	enum v4l2_buf_type      type;
> -	enum v4l2_memory        memory;
> +	__u32		      type;		/* enum v4l2_buf_type */
> +	__u32		        memory;		/* enum v4l2_memory */
>  	__u32			reserved[2];
>  };
>  
> @@ -636,16 +636,16 @@ struct v4l2_plane {
>   */
>  struct v4l2_buffer {
>  	__u32			index;
> -	enum v4l2_buf_type      type;
> +	__u32			type;		/* enum v4l2_buf_type */
>  	__u32			bytesused;
>  	__u32			flags;
> -	enum v4l2_field		field;
> +	__u32			field;		/* enum v4l2_field */
>  	struct timeval		timestamp;
>  	struct v4l2_timecode	timecode;
>  	__u32			sequence;
>  
>  	/* memory location */
> -	enum v4l2_memory        memory;
> +	__u32		        memory;		/* enum v4l2_memory */
>  	union {
>  		__u32           offset;
>  		unsigned long   userptr;
> @@ -707,7 +707,7 @@ struct v4l2_clip {
>  
>  struct v4l2_window {
>  	struct v4l2_rect        w;
> -	enum v4l2_field  	field;
> +	__u32			field;		/* enum v4l2_field */
>  	__u32			chromakey;
>  	struct v4l2_clip	__user *clips;
>  	__u32			clipcount;
> @@ -744,14 +744,14 @@ struct v4l2_outputparm {
>   *	I N P U T   I M A G E   C R O P P I N G
>   */
>  struct v4l2_cropcap {
> -	enum v4l2_buf_type      type;
> +	__u32			type;		/* enum v4l2_buf_type */
>  	struct v4l2_rect        bounds;
>  	struct v4l2_rect        defrect;
>  	struct v4l2_fract       pixelaspect;
>  };
>  
>  struct v4l2_crop {
> -	enum v4l2_buf_type      type;
> +	__u32			type;		/* enum v4l2_buf_type */
>  	struct v4l2_rect        c;
>  };
>  
> @@ -1156,7 +1156,7 @@ enum v4l2_ctrl_type {
>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>  struct v4l2_queryctrl {
>  	__u32		     id;
> -	enum v4l2_ctrl_type  type;
> +	__u32		     type;	/* enum v4l2_ctrl_type */
>  	__u8		     name[32];	/* Whatever */
>  	__s32		     minimum;	/* Note signedness */
>  	__s32		     maximum;
> @@ -1791,7 +1791,7 @@ enum v4l2_jpeg_chroma_subsampling {
>  struct v4l2_tuner {
>  	__u32                   index;
>  	__u8			name[32];
> -	enum v4l2_tuner_type    type;
> +	__u32			type;		/* enum v4l2_tuner_type */
>  	__u32			capability;
>  	__u32			rangelow;
>  	__u32			rangehigh;
> @@ -1841,14 +1841,14 @@ struct v4l2_modulator {
>  
>  struct v4l2_frequency {
>  	__u32		      tuner;
> -	enum v4l2_tuner_type  type;
> +	__u32		      type;		/* enum v4l2_tuner_type */
>  	__u32		      frequency;
>  	__u32		      reserved[8];
>  };
>  
>  struct v4l2_hw_freq_seek {
>  	__u32		      tuner;
> -	enum v4l2_tuner_type  type;
> +	__u32		      type;		/* enum v4l2_tuner_type */
>  	__u32		      seek_upward;
>  	__u32		      wrap_around;
>  	__u32		      spacing;
> @@ -2059,7 +2059,7 @@ struct v4l2_sliced_vbi_cap {
>  				 (equals frame lines 313-336 for 625 line video
>  				  standards, 263-286 for 525 line standards) */
>  	__u16   service_lines[2][24];
> -	enum v4l2_buf_type type;
> +	__u32	 type;		/* enum v4l2_buf_type */
>  	__u32   reserved[3];    /* must be 0 */
>  };
>  
> @@ -2149,8 +2149,8 @@ struct v4l2_pix_format_mplane {
>  	__u32				width;
>  	__u32				height;
>  	__u32				pixelformat;
> -	enum v4l2_field			field;
> -	enum v4l2_colorspace		colorspace;
> +	__u32				field;		/* enum v4l2_field */
> +	__u32				colorspace;	/* enum v4l2_colorspace */
>  
>  	struct v4l2_plane_pix_format	plane_fmt[VIDEO_MAX_PLANES];
>  	__u8				num_planes;
> @@ -2168,7 +2168,7 @@ struct v4l2_pix_format_mplane {
>   * @raw_data:	placeholder for future extensions and custom formats
>   */
>  struct v4l2_format {
> -	enum v4l2_buf_type type;
> +	__u32	type;		/* enum v4l2_buf_type */
>  	union {
>  		struct v4l2_pix_format		pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
>  		struct v4l2_pix_format_mplane	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
> @@ -2182,7 +2182,7 @@ struct v4l2_format {
>  /*	Stream type-dependent parameters
>   */
>  struct v4l2_streamparm {
> -	enum v4l2_buf_type type;
> +	__u32	type;		/* enum v4l2_buf_type */
>  	union {
>  		struct v4l2_captureparm	capture;
>  		struct v4l2_outputparm	output;
> @@ -2302,7 +2302,7 @@ struct v4l2_dbg_chip_ident {
>  struct v4l2_create_buffers {
>  	__u32			index;
>  	__u32			count;
> -	enum v4l2_memory        memory;
> +	__u32		        memory;		/* enum v4l2_memory */
>  	struct v4l2_format	format;
>  	__u32			reserved[8];
>  };
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index 3cb939c..77018d8 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -333,4 +333,213 @@ extern long video_usercopy(struct file *file, unsigned int cmd,
>  extern long video_ioctl2(struct file *file,
>  			unsigned int cmd, unsigned long arg);
>  
> +/*
> + * Backward-compatible IOCTL's to be used by V4L2 core to work with the
> + * old ioctl's defined with "enum's" inside the structures
> + */
> +
> +#ifdef CONFIG_V4L2_COMPAT
> +
> +struct v4l2_pix_format_enum {
> +	__u32			width;
> +	__u32			height;
> +	__u32			pixelformat;
> +	enum v4l2_field		field;
> +	__u32			bytesperline;	/* for padding, zero if unused */
> +	__u32			sizeimage;
> +	enum v4l2_colorspace	colorspace;
> +	__u32			priv;		/* private data, depends on pixelformat */
> +};
> +
> +struct v4l2_fmtdesc_enum {
> +	__u32			index;             /* Format number      */
> +	enum v4l2_buf_type	type;              /* buffer type        */
> +	__u32			flags;
> +	__u8			description[32];   /* Description string */
> +	__u32			pixelformat;       /* Format fourcc      */
> +	__u32			reserved[4];
> +};
> +
> +struct v4l2_requestbuffers_enum {
> +	__u32			count;
> +	enum v4l2_buf_type	type;
> +	enum v4l2_memory	memory;
> +	__u32			reserved[2];
> +};
> +
> +struct v4l2_buffer_enum {
> +	__u32			index;
> +	enum v4l2_buf_type	type;
> +	__u32			bytesused;
> +	__u32			flags;
> +	enum v4l2_field		field;
> +	struct timeval		timestamp;
> +	struct v4l2_timecode	timecode;
> +	__u32			sequence;
> +
> +	/* memory location */
> +	enum v4l2_memory	memory;
> +	union {
> +		__u32		offset;
> +		unsigned long	userptr;
> +		struct v4l2_plane *planes;
> +	} m;
> +	__u32			length;
> +	__u32			reserved2;
> +	__u32			reserved;
> +};
> +
> +struct v4l2_framebuffer_enum {
> +	__u32			capability;
> +	__u32			flags;
> +/* FIXME: in theory we should pass something like PCI device + memory
> + * region + offset instead of some physical address */
> +	void			*base;
> +	struct v4l2_pix_format_enum fmt;
> +};
> +
> +struct v4l2_window_enum {
> +	struct v4l2_rect	w;
> +	enum v4l2_field		field;
> +	__u32			chromakey;
> +	struct v4l2_clip	__user *clips;
> +	__u32			clipcount;
> +	void			__user *bitmap;
> +	__u8			global_alpha;
> +};
> +
> +struct v4l2_cropcap_enum {
> +	enum v4l2_buf_type	type;
> +	struct v4l2_rect	bounds;
> +	struct v4l2_rect	defrect;
> +	struct v4l2_fract	pixelaspect;
> +};
> +
> +struct v4l2_crop_enum {
> +	enum v4l2_buf_type	type;
> +	struct v4l2_rect	c;
> +};
> +
> +struct v4l2_queryctrl_enum {
> +	__u32			id;
> +	enum v4l2_ctrl_type	type;
> +	__u8			name[32];	/* Whatever */
> +	__s32			minimum;	/* Note signedness */
> +	__s32			maximum;
> +	__s32			step;
> +	__s32			default_value;
> +	__u32			flags;
> +	__u32			reserved[2];
> +};
> +
> +struct v4l2_tuner_enum {
> +	__u32			index;
> +	__u8			name[32];
> +	enum v4l2_tuner_type	type;
> +	__u32			capability;
> +	__u32			rangelow;
> +	__u32			rangehigh;
> +	__u32			rxsubchans;
> +	__u32			audmode;
> +	__s32			signal;
> +	__s32			afc;
> +	__u32			reserved[4];
> +};
> +
> +struct v4l2_frequency_enum {
> +	__u32			tuner;
> +	enum v4l2_tuner_type	type;
> +	__u32			frequency;
> +	__u32			reserved[8];
> +};
> +
> +struct v4l2_hw_freq_seek_enum {
> +	__u32			tuner;
> +	enum v4l2_tuner_type	type;
> +	__u32			seek_upward;
> +	__u32			wrap_around;
> +	__u32			spacing;
> +	__u32			reserved[7];
> +};
> +
> +struct v4l2_sliced_vbi_cap_enum {
> +	__u16	service_set;
> +	/* service_lines[0][...] specifies lines 0-23 (1-23 used) of the first field
> +	   service_lines[1][...] specifies lines 0-23 (1-23 used) of the second field
> +				 (equals frame lines 313-336 for 625 line video
> +				  standards, 263-286 for 525 line standards) */
> +	__u16	service_lines[2][24];
> +	enum v4l2_buf_type type;
> +	__u32	reserved[3];    /* must be 0 */
> +};
> +
> +struct v4l2_pix_format_mplane_enum {
> +	__u32				width;
> +	__u32				height;
> +	__u32				pixelformat;
> +	enum v4l2_field			field;
> +	enum v4l2_colorspace		colorspace;
> +
> +	struct v4l2_plane_pix_format	plane_fmt[VIDEO_MAX_PLANES];
> +	__u8				num_planes;
> +	__u8				reserved[11];
> +} __packed;
> +
> +struct v4l2_format_enum {
> +	enum v4l2_buf_type type;
> +	union {
> +		struct v4l2_pix_format_enum	pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
> +		struct v4l2_pix_format_mplane_enum	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
> +		struct v4l2_window_enum		win;     /* V4L2_BUF_TYPE_VIDEO_OVERLAY */
> +		struct v4l2_vbi_format		vbi;     /* V4L2_BUF_TYPE_VBI_CAPTURE */
> +		struct v4l2_sliced_vbi_format	sliced;  /* V4L2_BUF_TYPE_SLICED_VBI_CAPTURE */
> +		__u8	raw_data[200];                   /* user-defined */
> +	} fmt;
> +};
> +
> +/*	Stream type-dependent parameters
> + */
> +struct v4l2_streamparm_enum {
> +	enum v4l2_buf_type type;
> +	union {
> +		struct v4l2_captureparm	capture;
> +		struct v4l2_outputparm	output;
> +		__u8	raw_data[200];  /* user-defined */
> +	} parm;
> +};
> +
> +struct v4l2_create_buffers_enum {
> +	__u32			index;
> +	__u32			count;
> +	enum v4l2_memory	memory;
> +	struct v4l2_format_enum	format;
> +	__u32			reserved[8];
> +};
> +
> +#define VIDIOC_ENUM_FMT_ENUM		_IOWR('V',  2, struct v4l2_fmtdesc_enum)
> +#define VIDIOC_G_FMT_ENUM		_IOWR('V',  4, struct v4l2_format_enum)
> +#define VIDIOC_S_FMT_ENUM		_IOWR('V',  5, struct v4l2_format_enum)
> +#define VIDIOC_REQBUFS_ENUM		_IOWR('V',  8, struct v4l2_requestbuffers_enum)
> +#define VIDIOC_QUERYBUF_ENUM		_IOWR('V',  9, struct v4l2_buffer_enum)
> +#define VIDIOC_G_FBUF_ENUM		_IOR('V', 10, struct v4l2_framebuffer_enum)
> +#define VIDIOC_S_FBUF_ENUM		_IOW('V', 11, struct v4l2_framebuffer_enum)
> +#define VIDIOC_QBUF_ENUM		_IOWR('V', 15, struct v4l2_buffer_enum)
> +#define VIDIOC_DQBUF_ENUM		_IOWR('V', 17, struct v4l2_buffer_enum)
> +#define VIDIOC_G_PARM_ENUM		_IOWR('V', 21, struct v4l2_streamparm_enum)
> +#define VIDIOC_S_PARM_ENUM		_IOWR('V', 22, struct v4l2_streamparm_enum)
> +#define VIDIOC_G_TUNER_ENUM		_IOWR('V', 29, struct v4l2_tuner_enum)
> +#define VIDIOC_S_TUNER_ENUM		_IOW('V', 30, struct v4l2_tuner_enum)
> +#define VIDIOC_QUERYCTRL_ENUM		_IOWR('V', 36, struct v4l2_queryctrl_enum)
> +#define VIDIOC_G_FREQUENCY_ENUM		_IOWR('V', 56, struct v4l2_frequency_enum)
> +#define VIDIOC_S_FREQUENCY_ENUM		_IOW('V', 57, struct v4l2_frequency_enum)
> +#define VIDIOC_CROPCAP_ENUM		_IOWR('V', 58, struct v4l2_cropcap_enum)
> +#define VIDIOC_G_CROP_ENUM		_IOWR('V', 59, struct v4l2_crop_enum)
> +#define VIDIOC_S_CROP_ENUM		_IOW('V', 60, struct v4l2_crop_enum)
> +#define VIDIOC_TRY_FMT_ENUM		_IOWR('V', 64, struct v4l2_format_enum)
> +#define VIDIOC_G_SLICED_VBI_CAP_ENUM	_IOWR('V', 69, struct v4l2_sliced_vbi_cap_enum)
> +#define VIDIOC_S_HW_FREQ_SEEK_ENUM	_IOW('V', 82, struct v4l2_hw_freq_seek_enum)
> +#define VIDIOC_CREATE_BUFS_ENUM		_IOWR('V', 92, struct v4l2_create_buffers_enum)
> +#define VIDIOC_PREPARE_BUF_ENUM		_IOWR('V', 93, struct v4l2_buffer_enum)
> +#endif /* CONFIG_V4L2_COMPAT */
> +
>  #endif /* _V4L2_IOCTL_H */
> 

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Sakari Ailus]

Hi Hans,

On Wed, May 02, 2012 at 10:45:22PM +0200, Hans Verkuil wrote:
> On Wed May 2 2012 21:13:47 Sakari Ailus wrote:
> > Replace enums in IOCTL structs by __u32. The size of enums is variable and
> > thus problematic. Compatibility structs having exactly the same as original
> > definition are provided for compatibility with old binaries with the
> > required conversion code.
> 
> Does someone actually have hard proof that there really is a problem? You know,
> demonstrate it with actual example code?
> 
> It's pretty horrible that you have to do all those conversions and that code
> will be with us for years to come.
> 
> For most (if not all!) architectures sizeof(enum) == sizeof(u32), so there is
> no need for any compat code for those.

Cases I know where this can go wrong are, but there may well be others:

- ppc64: int is 64 bits there, and thus also enums,

- Enums are quite a different concept in C++ than in C --- the compiler may
  make assumpton based on the value range of the enums --- videodev2.h should
  be included with extern "C" in that case, though,

- C does not specify which integer type enums actually use; this is what GCC
  manual says about it:

  <URL:http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Enumerations>

  So a compiler other than GCC should use 16-bit enums and conform to C
  while breaking V4L2. This might be a theoretical issue, though.

More discussion took place in this thread:

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

Regards,

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

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Mauro Carvalho Chehab]

Hi Hans,

Em 02-05-2012 17:45, Hans Verkuil escreveu:
> On Wed May 2 2012 21:13:47 Sakari Ailus wrote:
>> Replace enums in IOCTL structs by __u32. The size of enums is variable and
>> thus problematic. Compatibility structs having exactly the same as original
>> definition are provided for compatibility with old binaries with the
>> required conversion code.
> 
> Does someone actually have hard proof that there really is a problem? You know,
> demonstrate it with actual example code?
> 
> It's pretty horrible that you have to do all those conversions and that code
> will be with us for years to come.
> 
> For most (if not all!) architectures sizeof(enum) == sizeof(u32), so there is
> no need for any compat code for those.

"Most" is not enough. We need a solution for all.

Also, the usage of -fshort-enum compilation directive would cause a V4L2 application 
to not work.

  I've checked with a gcc developer: he said that,
C standard says that the type should be represented as an integer, but it does allow
that it could be represented by a sorter type, like char. Gcc also allows using larger
enumber types, as a compatible extension, but doesn't use (by default) sorter types,
except if packed attribute is used.

As we use __packed on several structs, this can cause troubles.

It seems that the compiler may also choose to use either signed or unsigned integers
for enums.

He also doesn't recommend to use enums for any bitfield, as this is not defined on
C standard, and the way GCC implements it can cause troubles.

I would prefer a simpler solution, but, after thinking for some time, we should really
do something like that. Yes, compat code sucks, but, on the long term, we'll get rid
of all those mess. If we don't do that, I'm sure that this problem will return back in
the future (as this is the second time it returned. If we had fixed it on that time,
all our problems would already be solved nowadays).

> 
> Note that I don't question that using u32 is better than using enums, but I
> really wonder if there is any need for all the conversions.

We can speed-up the conversions, with something like:

enum foo {
	BAR
};

if (sizeof(foo) != sizeof(u32))
	call_compat_logic().

I suspect that sizeof() won't work inside a macro. 

Regards,
Mauro

Re: [RFC v3 2/2] v4l: Implement compat functions for enum to __u32 change

[Mauro Carvalho Chehab]

Em 02-05-2012 16:13, Sakari Ailus escreveu:
> Implement compat functions to provide conversion between structs containing
> enums and those not. The functions are intended to be removed when the
> support for such old binaries is no longer necessary.

This is not a full review of this patch, as checking the full logic
will consume some time.

This is just a few points to consider.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/video/Kconfig      |   12 +
>  drivers/media/video/v4l2-ioctl.c |  684 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 687 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index f2479c5..949f804 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -7,6 +7,18 @@ config VIDEO_V4L2
>  	depends on VIDEO_DEV && VIDEO_V4L2_COMMON
>  	default VIDEO_DEV && VIDEO_V4L2_COMMON
>  
> +config V4L2_COMPAT
> +	bool "Compatibility for old binaries"
> +	depends on VIDEO_V4L2
> +	default y
> +	---help---
> +	  Compatibility code to support binaries compiled with old V4L2
> +	  IOCTL definitions containing enums that have been replaced by
> +	  __u32. If you do not need to use such binaries you can disable
> +	  this option.
> +
> +	  When in doubt, say Y.
> +
>  config VIDEOBUF_GEN
>  	tristate
>  
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index 5b2ec1f..9b88360 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -2303,6 +2303,653 @@ static long __video_do_ioctl(struct file *file,
>  	return ret;
>  }
>  
> +#ifdef CONFIG_V4L2_COMPAT

Hmm... maybe the better would be to put it on a separate file.

Also, as the size on userspace can be different than on Kernelspace, a similar
code is needed at v4l2-compat-ioctl32.c.

As I said before (not sure if it was via e-mail or on irc), I suspect that the
code there can be simplified a lot, as only the structs with enums and pointers
require compat code. For all others, the code could simply use the default
behavior.

> +
> +static inline unsigned int get_non_compat_cmd(unsigned int cmd)
> +{
> +	switch (cmd) {
> +	case VIDIOC_ENUM_FMT_ENUM:
> +		return VIDIOC_ENUM_FMT;
> +	case VIDIOC_G_FMT_ENUM:
> +		return VIDIOC_G_FMT;
> +	case VIDIOC_S_FMT_ENUM:
> +		return VIDIOC_S_FMT;
> +	case VIDIOC_REQBUFS_ENUM:
> +		return VIDIOC_REQBUFS;
> +	case VIDIOC_QUERYBUF_ENUM:
> +		return VIDIOC_QUERYBUF;
> +	case VIDIOC_G_FBUF_ENUM:
> +		return VIDIOC_G_FBUF;
> +	case VIDIOC_S_FBUF_ENUM:
> +		return VIDIOC_S_FBUF;
> +	case VIDIOC_QBUF_ENUM:
> +		return VIDIOC_QBUF;
> +	case VIDIOC_DQBUF_ENUM:
> +		return VIDIOC_DQBUF;
> +	case VIDIOC_G_PARM_ENUM:
> +		return VIDIOC_G_PARM;
> +	case VIDIOC_S_PARM_ENUM:
> +		return VIDIOC_S_PARM;
> +	case VIDIOC_G_TUNER_ENUM:
> +		return VIDIOC_G_TUNER;
> +	case VIDIOC_S_TUNER_ENUM:
> +		return VIDIOC_S_TUNER;
> +	case VIDIOC_QUERYCTRL_ENUM:
> +		return VIDIOC_QUERYCTRL;
> +	case VIDIOC_G_FREQUENCY_ENUM:
> +		return VIDIOC_G_FREQUENCY;
> +	case VIDIOC_S_FREQUENCY_ENUM:
> +		return VIDIOC_S_FREQUENCY;
> +	case VIDIOC_CROPCAP_ENUM:
> +		return VIDIOC_CROPCAP;
> +	case VIDIOC_G_CROP_ENUM:
> +		return VIDIOC_G_CROP;
> +	case VIDIOC_S_CROP_ENUM:
> +		return VIDIOC_S_CROP;
> +	case VIDIOC_TRY_FMT_ENUM:
> +		return VIDIOC_TRY_FMT;
> +	case VIDIOC_G_SLICED_VBI_CAP_ENUM:
> +		return VIDIOC_G_SLICED_VBI_CAP;
> +	case VIDIOC_S_HW_FREQ_SEEK_ENUM:
> +		return VIDIOC_S_HW_FREQ_SEEK;
> +	case VIDIOC_CREATE_BUFS_ENUM:
> +		return VIDIOC_CREATE_BUFS;
> +	case VIDIOC_PREPARE_BUF_ENUM:
> +		return VIDIOC_PREPARE_BUF;
> +	default:
> +		return cmd;
> +	}
> +}
> +
> +#define get_user_conv(x, ptr)			\
> +	({					\
> +		typeof (*ptr) tmp;		\
> +		int rval;			\
> +						\
> +		rval = get_user(tmp, ptr);	\
> +		if (!rval)			\
> +			x = (typeof (x))tmp;	\
> +						\
> +		rval;				\
> +	})
> +
> +static int get_user_pix_format(struct v4l2_pix_format *k,
> +			       struct v4l2_pix_format_enum *u)
> +{
> +	return get_user(k->width, &u->width)
> +		|| get_user(k->height, &u->height)
> +		|| get_user(k->pixelformat, &u->pixelformat)
> +		|| get_user_conv(k->field, &u->field)
> +		|| get_user(k->bytesperline, &u->bytesperline)
> +		|| get_user(k->sizeimage, &u->sizeimage)
> +		|| get_user_conv(k->colorspace, &u->colorspace)
> +		|| get_user(k->priv, &u->priv);
> +}
> +
> +static int get_user_pix_format_mplane(struct v4l2_pix_format_mplane *k,
> +				      struct v4l2_pix_format_mplane_enum *u)
> +{
> +	return get_user(k->width, &u->width)
> +		|| get_user(k->height, &u->height)
> +		|| get_user(k->pixelformat, &u->pixelformat)
> +		|| get_user_conv(k->field, &u->field)
> +		|| get_user_conv(k->colorspace, &u->colorspace)
> +		|| copy_from_user(k->plane_fmt, u->plane_fmt,
> +				  sizeof(k->plane_fmt))
> +		|| get_user(k->num_planes, &u->num_planes)
> +		|| copy_from_user(k->reserved, u->reserved,
> +				  sizeof(k->reserved));
> +}
> +
> +static int get_user_window(struct v4l2_window *k,
> +			   struct v4l2_window_enum *u)
> +{
> +	return copy_from_user(&k->w, &u->w, sizeof(k->w))
> +		|| get_user_conv(k->field, &u->field)
> +		|| get_user(k->chromakey, &u->chromakey)
> +		|| get_user(k->clips, &u->clips)
> +		|| get_user(k->clipcount, &u->clipcount)
> +		|| get_user(k->bitmap, &u->bitmap)
> +		|| get_user(k->global_alpha, &u->global_alpha);
> +}
> +
> +static int get_user_format(struct v4l2_format *k,
> +			   struct v4l2_format_enum *u)
> +{
> +	if (get_user(k->type, &u->type))
> +		return -EFAULT;
> +
> +	switch (k->type) {
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> +		if (get_user_pix_format(&k->fmt.pix, &u->fmt.pix))
> +			return -EFAULT;
> +		return 0;
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> +		if (get_user_pix_format_mplane(&k->fmt.pix_mp, &u->fmt.pix_mp))
> +			return -EFAULT;
> +		return 0;
> +	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> +		if (get_user_window(&k->fmt.win, &u->fmt.win))
> +			return -EFAULT;
> +		return 0;
> +	default:
> +		if (copy_from_user(k->fmt.raw_data, u->fmt.raw_data,
> +				   sizeof(k->fmt.raw_data)))
> +			return -EFAULT;
> +		return 0;
> +	}
> +}
> +
> +static long copy_compat_from_user(unsigned int cmd, void *parg,
> +				  void __user *arg)
> +{
> +	union {
> +		struct v4l2_fmtdesc_enum fmtdesc;
> +		struct v4l2_format_enum fmt;
> +		struct v4l2_requestbuffers_enum reqbufs;
> +		struct v4l2_framebuffer_enum fb;
> +		struct v4l2_buffer_enum buf;
> +		struct v4l2_streamparm_enum sparm;
> +		struct v4l2_tuner_enum tuner;
> +		struct v4l2_queryctrl_enum qc;
> +		struct v4l2_frequency_enum freq;
> +		struct v4l2_cropcap_enum cropcap;
> +		struct v4l2_crop_enum crop;
> +		struct v4l2_sliced_vbi_cap_enum vbi_cap;
> +		struct v4l2_hw_freq_seek_enum freq_seek;
> +		struct v4l2_create_buffers_enum create_bufs;
> +	} __user *u = arg;
> +	union {
> +		struct v4l2_fmtdesc fmtdesc;
> +		struct v4l2_format fmt;
> +		struct v4l2_requestbuffers reqbufs;
> +		struct v4l2_framebuffer fb;
> +		struct v4l2_buffer buf;
> +		struct v4l2_streamparm sparm;
> +		struct v4l2_tuner tuner;
> +		struct v4l2_queryctrl qc;
> +		struct v4l2_frequency freq;
> +		struct v4l2_cropcap cropcap;
> +		struct v4l2_crop crop;
> +		struct v4l2_sliced_vbi_cap vbi_cap;
> +		struct v4l2_hw_freq_seek freq_seek;
> +		struct v4l2_create_buffers create_bufs;
> +	} *k = parg;
> +
> +	if (!access_ok(VERIFY_READ, u, _IOC_SIZE(cmd)))
> +		return -EFAULT;
> +
> +	switch (cmd) {
> +	case VIDIOC_ENUM_FMT_ENUM:
> +		if (get_user(k->fmtdesc.index, &u->fmtdesc.index)
> +		    || get_user_conv(k->fmtdesc.type, &u->fmtdesc.type)
> +		    || get_user(k->fmtdesc.flags, &u->fmtdesc.flags)
> +		    || copy_from_user(k->fmtdesc.description,
> +				      u->fmtdesc.description,
> +				      sizeof(k->fmtdesc.description))
> +		    || get_user(k->fmtdesc.pixelformat,
> +				&u->fmtdesc.pixelformat)
> +		    || copy_from_user(k->fmtdesc.reserved,
> +				      u->fmtdesc.reserved,
> +				      sizeof(k->fmtdesc.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_FMT_ENUM:
> +	case VIDIOC_S_FMT_ENUM:
> +	case VIDIOC_TRY_FMT_ENUM:
> +		if (get_user_format(&k->fmt, &u->fmt))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_REQBUFS_ENUM:
> +		if (get_user(k->reqbufs.count, &u->reqbufs.count)
> +		    || get_user_conv(k->reqbufs.type, &u->reqbufs.type)
> +		    || get_user_conv(k->reqbufs.memory, &u->reqbufs.memory)
> +		    || copy_from_user(k->reqbufs.reserved,
> +				      u->reqbufs.reserved,
> +				      sizeof(k->reqbufs.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_QUERYBUF_ENUM:
> +	case VIDIOC_QBUF_ENUM:
> +	case VIDIOC_DQBUF_ENUM:
> +	case VIDIOC_PREPARE_BUF_ENUM:
> +		if (get_user(k->buf.index, &u->buf.index)
> +		    || get_user_conv(k->buf.type, &u->buf.type)
> +		    || get_user(k->buf.bytesused, &u->buf.bytesused)
> +		    || get_user(k->buf.flags, &u->buf.flags)
> +		    || get_user_conv(k->buf.field, &u->buf.field)
> +		    || copy_from_user(&k->buf.timestamp, &u->buf.timestamp,
> +				      sizeof(k->buf.timestamp))
> +		    || copy_from_user(&k->buf.timecode, &u->buf.timecode,
> +				      sizeof(k->buf.timecode))
> +		    || get_user(k->buf.sequence, &u->buf.sequence)
> +		    || get_user_conv(k->buf.memory, &u->buf.memory)
> +		    || copy_from_user(&k->buf.m, &u->buf.m, sizeof(k->buf.m))
> +		    || get_user(k->buf.length, &u->buf.length)
> +		    || get_user(k->buf.reserved2, &u->buf.reserved2)
> +		    || get_user(k->buf.reserved, &u->buf.reserved))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_FBUF_ENUM:
> +	case VIDIOC_S_FBUF_ENUM:
> +		if (get_user(k->fb.capability, &u->fb.capability)
> +		    || get_user(k->fb.flags, &u->fb.flags)
> +		    || get_user(k->fb.base, &u->fb.base)
> +		    || get_user_pix_format(&k->fb.fmt, &u->fb.fmt))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_PARM_ENUM:
> +	case VIDIOC_S_PARM_ENUM:
> +		if (get_user_conv(k->sparm.type, &u->sparm.type)
> +		    || copy_from_user(&k->sparm.parm, &u->sparm.parm,
> +				      sizeof(k->sparm.parm)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_TUNER_ENUM:
> +	case VIDIOC_S_TUNER_ENUM:
> +		if (get_user(k->tuner.index, &u->tuner.index)
> +		    || copy_from_user(k->tuner.name, u->tuner.name,
> +				      sizeof(k->tuner.name))
> +		    || get_user_conv(k->tuner.type, &u->tuner.type)
> +		    || get_user(k->tuner.capability, &u->tuner.capability)
> +		    || get_user(k->tuner.rangelow, &u->tuner.rangelow)
> +		    || get_user(k->tuner.rangehigh, &u->tuner.rangehigh)
> +		    || get_user(k->tuner.rxsubchans, &u->tuner.rxsubchans)
> +		    || get_user(k->tuner.audmode, &u->tuner.audmode)
> +		    || get_user(k->tuner.signal, &u->tuner.signal)
> +		    || get_user(k->tuner.afc, &u->tuner.afc)
> +		    || copy_from_user(k->tuner.reserved, u->tuner.reserved,
> +				      sizeof(k->tuner.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_QUERYCTRL_ENUM:
> +		if (get_user(k->qc.id, &u->qc.id)
> +		    || get_user_conv(k->qc.type, &u->qc.type)
> +		    || copy_from_user(k->qc.name, u->qc.name,
> +				      sizeof(k->qc.name))
> +		    || get_user(k->qc.minimum, &u->qc.minimum)
> +		    || get_user(k->qc.maximum, &u->qc.maximum)
> +		    || get_user(k->qc.step, &u->qc.step)
> +		    || get_user(k->qc.default_value, &u->qc.default_value)
> +		    || get_user(k->qc.flags, &u->qc.flags)
> +		    || copy_from_user(k->qc.reserved, u->qc.reserved,
> +				      sizeof(k->qc.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_FREQUENCY_ENUM:
> +	case VIDIOC_S_FREQUENCY_ENUM:
> +		if (get_user(k->freq.tuner, &u->freq.tuner)
> +		    || get_user_conv(k->freq.type, &u->freq.type)
> +		    || get_user(k->freq.frequency, &u->freq.frequency)
> +		    || copy_from_user(k->freq.reserved, u->freq.reserved,
> +				      sizeof(k->freq.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_CROPCAP_ENUM:
> +		if (get_user_conv(k->cropcap.type, &u->cropcap.type)
> +		    || copy_from_user(&k->cropcap.bounds, &u->cropcap.bounds,
> +				      sizeof(k->cropcap.bounds))
> +		    || copy_from_user(&k->cropcap.defrect, &u->cropcap.defrect,
> +				      sizeof(k->cropcap.bounds))
> +		    || copy_from_user(&k->cropcap.pixelaspect,
> +				      &u->cropcap.pixelaspect,
> +				      sizeof(k->cropcap.pixelaspect)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_CROP_ENUM:
> +	case VIDIOC_S_CROP_ENUM:
> +		if (get_user_conv(k->crop.type, &u->crop.type)
> +		    || copy_from_user(&k->crop.c, &u->crop.c,
> +				      sizeof(k->crop.c)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_SLICED_VBI_CAP_ENUM:
> +		if (get_user(k->vbi_cap.service_set, &u->vbi_cap.service_set)
> +		    || copy_from_user(k->vbi_cap.service_lines,
> +				      u->vbi_cap.service_lines,
> +				      sizeof(k->vbi_cap.service_lines))
> +		    || get_user_conv(k->vbi_cap.type, &u->vbi_cap.type)
> +		    || copy_from_user(k->vbi_cap.reserved, u->vbi_cap.reserved,
> +				      sizeof(k->vbi_cap.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_S_HW_FREQ_SEEK_ENUM:
> +		if (get_user(k->freq_seek.tuner, &u->freq_seek.tuner)
> +		    || get_user_conv(k->freq_seek.type, &u->freq_seek.type)
> +		    || get_user(k->freq_seek.seek_upward,
> +				&u->freq_seek.seek_upward)
> +		    || get_user(k->freq_seek.wrap_around,
> +				&u->freq_seek.wrap_around)
> +		    || get_user(k->freq_seek.spacing, &u->freq_seek.spacing)
> +		    || copy_from_user(k->freq_seek.reserved,
> +				      u->freq_seek.reserved,
> +				      sizeof(k->freq_seek.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_CREATE_BUFS_ENUM:
> +		if (get_user(k->create_bufs.index, &u->create_bufs.index)
> +		    || get_user(k->create_bufs.count, &u->create_bufs.count)
> +		    || get_user_conv(k->create_bufs.memory,
> +				     &u->create_bufs.memory)
> +		    || get_user_format(&k->create_bufs.format,
> +				       &u->create_bufs.format)
> +		    || copy_from_user(k->create_bufs.reserved,
> +				      u->create_bufs.reserved,
> +				      sizeof(k->create_bufs.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	default:
> +		WARN(1, "%s: bad compat cmd %8.8x\n", __func__, cmd);
> +		return -EINVAL;
> +	}
> +}
> +
> +#define put_user_conv(x, ptr)			\
> +	({					\
> +		typeof (*ptr) tmp;		\
> +						\
> +		tmp = (typeof (*ptr))x;		\
> +		put_user(tmp, ptr);		\
> +	})
> +
> +static int put_user_pix_format(struct v4l2_pix_format *k,
> +			       struct v4l2_pix_format_enum *u)
> +{
> +	return put_user(k->width, &u->width)
> +		|| put_user(k->height, &u->height)
> +		|| put_user(k->pixelformat, &u->pixelformat)
> +		|| put_user_conv(k->field, &u->field)
> +		|| put_user(k->bytesperline, &u->bytesperline)
> +		|| put_user(k->sizeimage, &u->sizeimage)
> +		|| put_user_conv(k->colorspace, &u->colorspace)
> +		|| put_user(k->priv, &u->priv);
> +}
> +
> +static int put_user_pix_format_mplane(struct v4l2_pix_format_mplane *k,
> +				      struct v4l2_pix_format_mplane_enum *u)
> +{
> +	return put_user(k->width, &u->width)
> +		|| put_user(k->height, &u->height)
> +		|| put_user(k->pixelformat, &u->pixelformat)
> +		|| put_user_conv(k->field, &u->field)
> +		|| put_user_conv(k->colorspace, &u->colorspace)
> +		|| copy_to_user(u->plane_fmt, k->plane_fmt,
> +				sizeof(k->plane_fmt))
> +		|| put_user(k->num_planes, &u->num_planes)
> +		|| copy_to_user(u->reserved, k->reserved,
> +				sizeof(k->reserved));
> +}
> +
> +static int put_user_window(struct v4l2_window *k,
> +			   struct v4l2_window_enum *u)
> +{
> +	return copy_to_user(&u->w, &k->w, sizeof(k->w))
> +		|| put_user_conv(k->field, &u->field)
> +		|| put_user(k->chromakey, &u->chromakey)
> +		|| put_user(k->clips, &u->clips)
> +		|| put_user(k->clipcount, &u->clipcount)
> +		|| put_user(k->bitmap, &u->bitmap)
> +		|| put_user(k->global_alpha, &u->global_alpha);
> +}
> +
> +static int put_user_format(struct v4l2_format *k,
> +			   struct v4l2_format_enum *u)
> +{
> +	if (put_user(k->type, &u->type))
> +		return -EFAULT;
> +
> +	switch (k->type) {
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> +		if (put_user_pix_format(&k->fmt.pix, &u->fmt.pix))
> +			return -EFAULT;
> +		return 0;
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> +		if (put_user_pix_format_mplane(&k->fmt.pix_mp, &u->fmt.pix_mp))
> +			return -EFAULT;
> +		return 0;
> +	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> +		if (put_user_window(&k->fmt.win, &u->fmt.win))
> +			return -EFAULT;
> +		return 0;
> +	default:
> +		if (copy_to_user(u->fmt.raw_data, k->fmt.raw_data,
> +				 sizeof(k->fmt.raw_data)))
> +			return -EFAULT;
> +		return 0;
> +	}
> +}
> +
> +static long copy_compat_to_user(unsigned int cmd, void __user *arg,
> +				void *parg)
> +{
> +	union {
> +		struct v4l2_fmtdesc_enum fmtdesc;
> +		struct v4l2_format_enum fmt;
> +		struct v4l2_requestbuffers_enum reqbufs;
> +		struct v4l2_framebuffer_enum fb;
> +		struct v4l2_buffer_enum buf;
> +		struct v4l2_streamparm_enum sparm;
> +		struct v4l2_tuner_enum tuner;
> +		struct v4l2_queryctrl_enum qc;
> +		struct v4l2_frequency_enum freq;
> +		struct v4l2_cropcap_enum cropcap;
> +		struct v4l2_crop_enum crop;
> +		struct v4l2_sliced_vbi_cap_enum vbi_cap;
> +		struct v4l2_hw_freq_seek_enum freq_seek;
> +		struct v4l2_create_buffers_enum create_bufs;
> +	} __user *u = arg;
> +	union {
> +		struct v4l2_fmtdesc fmtdesc;
> +		struct v4l2_format fmt;
> +		struct v4l2_requestbuffers reqbufs;
> +		struct v4l2_framebuffer fb;
> +		struct v4l2_buffer buf;
> +		struct v4l2_streamparm sparm;
> +		struct v4l2_tuner tuner;
> +		struct v4l2_queryctrl qc;
> +		struct v4l2_frequency freq;
> +		struct v4l2_cropcap cropcap;
> +		struct v4l2_crop crop;
> +		struct v4l2_sliced_vbi_cap vbi_cap;
> +		struct v4l2_hw_freq_seek freq_seek;
> +		struct v4l2_create_buffers create_bufs;
> +	} *k = parg;
> +
> +	if (!access_ok(VERIFY_WRITE, u, _IOC_SIZE(cmd)))
> +		return -EFAULT;
> +
> +	switch (cmd) {
> +	case VIDIOC_ENUM_FMT_ENUM:
> +		if (put_user(k->fmtdesc.index, &u->fmtdesc.index)
> +		    || put_user_conv(k->fmtdesc.type, &u->fmtdesc.type)
> +		    || put_user(k->fmtdesc.flags, &u->fmtdesc.flags)
> +		    || copy_to_user(u->fmtdesc.description,
> +				    k->fmtdesc.description,
> +				    sizeof(k->fmtdesc.description))
> +		    || put_user(k->fmtdesc.pixelformat,
> +				&u->fmtdesc.pixelformat)
> +		    || copy_to_user(u->fmtdesc.reserved,
> +				    k->fmtdesc.reserved,
> +				    sizeof(k->fmtdesc.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_FMT_ENUM:
> +	case VIDIOC_S_FMT_ENUM:
> +	case VIDIOC_TRY_FMT_ENUM:
> +		if (put_user_format(&k->fmt, &u->fmt))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_REQBUFS_ENUM:
> +		if (put_user(k->reqbufs.count, &u->reqbufs.count)
> +		    || put_user_conv(k->reqbufs.type, &u->reqbufs.type)
> +		    || put_user_conv(k->reqbufs.memory, &u->reqbufs.memory)
> +		    || copy_to_user(u->reqbufs.reserved,
> +				    k->reqbufs.reserved,
> +				    sizeof(k->reqbufs.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_QUERYBUF_ENUM:
> +	case VIDIOC_QBUF_ENUM:
> +	case VIDIOC_DQBUF_ENUM:
> +	case VIDIOC_PREPARE_BUF_ENUM:
> +		if (put_user(k->buf.index, &u->buf.index)
> +		    || put_user_conv(k->buf.type, &u->buf.type)
> +		    || put_user(k->buf.bytesused, &u->buf.bytesused)
> +		    || put_user(k->buf.flags, &u->buf.flags)
> +		    || put_user_conv(k->buf.field, &u->buf.field)
> +		    || copy_to_user(&u->buf.timestamp, &k->buf.timestamp,
> +				    sizeof(k->buf.timestamp))
> +		    || copy_to_user(&u->buf.timecode, &k->buf.timecode,
> +				    sizeof(k->buf.timecode))
> +		    || put_user(k->buf.sequence, &u->buf.sequence)
> +		    || put_user_conv(k->buf.memory, &u->buf.memory)
> +		    || copy_to_user(&u->buf.m, &k->buf.m, sizeof(k->buf.m))
> +		    || put_user(k->buf.length, &u->buf.length)
> +		    || put_user(k->buf.reserved2, &u->buf.reserved2)
> +		    || put_user(k->buf.reserved, &u->buf.reserved))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_FBUF_ENUM:
> +	case VIDIOC_S_FBUF_ENUM:
> +		if (put_user(k->fb.capability, &u->fb.capability)
> +		    || put_user(k->fb.flags, &u->fb.flags)
> +		    || put_user(k->fb.base, &u->fb.base)
> +		    || put_user_pix_format(&k->fb.fmt, &u->fb.fmt))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_PARM_ENUM:
> +	case VIDIOC_S_PARM_ENUM:
> +		if (put_user_conv(k->sparm.type, &u->sparm.type)
> +		    || copy_to_user(&u->sparm.parm, &k->sparm.parm,
> +				    sizeof(k->sparm.parm)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_TUNER_ENUM:
> +	case VIDIOC_S_TUNER_ENUM:
> +		if (put_user(k->tuner.index, &u->tuner.index)
> +		    || copy_to_user(u->tuner.name, k->tuner.name,
> +				    sizeof(k->tuner.name))
> +		    || put_user_conv(k->tuner.type, &u->tuner.type)
> +		    || put_user(k->tuner.capability, &u->tuner.capability)
> +		    || put_user(k->tuner.rangelow, &u->tuner.rangelow)
> +		    || put_user(k->tuner.rangehigh, &u->tuner.rangehigh)
> +		    || put_user(k->tuner.rxsubchans, &u->tuner.rxsubchans)
> +		    || put_user(k->tuner.audmode, &u->tuner.audmode)
> +		    || put_user(k->tuner.signal, &u->tuner.signal)
> +		    || put_user(k->tuner.afc, &u->tuner.afc)
> +		    || copy_to_user(u->tuner.reserved, k->tuner.reserved,
> +				    sizeof(k->tuner.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_QUERYCTRL_ENUM:
> +		if (put_user(k->qc.id, &u->qc.id)
> +		    || put_user_conv(k->qc.type, &u->qc.type)
> +		    || copy_to_user(u->qc.name, k->qc.name,
> +				    sizeof(k->qc.name))
> +		    || put_user(k->qc.minimum, &u->qc.minimum)
> +		    || put_user(k->qc.maximum, &u->qc.maximum)
> +		    || put_user(k->qc.step, &u->qc.step)
> +		    || put_user(k->qc.default_value, &u->qc.default_value)
> +		    || put_user(k->qc.flags, &u->qc.flags)
> +		    || copy_to_user(u->qc.reserved, k->qc.reserved,
> +				    sizeof(k->qc.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_FREQUENCY_ENUM:
> +	case VIDIOC_S_FREQUENCY_ENUM:
> +		if (put_user(k->freq.tuner, &u->freq.tuner)
> +		    || put_user_conv(k->freq.type, &u->freq.type)
> +		    || put_user(k->freq.frequency, &u->freq.frequency)
> +		    || copy_to_user(u->freq.reserved, k->freq.reserved,
> +				    sizeof(k->freq.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_CROPCAP_ENUM:
> +		if (put_user_conv(k->cropcap.type, &u->cropcap.type)
> +		    || copy_to_user(&u->cropcap.bounds, &k->cropcap.bounds,
> +				    sizeof(k->cropcap.bounds))
> +		    || copy_to_user(&u->cropcap.defrect, &k->cropcap.defrect,
> +				    sizeof(k->cropcap.bounds))
> +		    || copy_to_user(&u->cropcap.pixelaspect,
> +				    &k->cropcap.pixelaspect,
> +				    sizeof(k->cropcap.pixelaspect)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_CROP_ENUM:
> +	case VIDIOC_S_CROP_ENUM:
> +		if (put_user_conv(k->crop.type, &u->crop.type)
> +		    || copy_to_user(&u->crop.c, &k->crop.c,
> +				    sizeof(k->crop.c)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_G_SLICED_VBI_CAP_ENUM:
> +		if (put_user(k->vbi_cap.service_set, &u->vbi_cap.service_set)
> +		    || copy_to_user(u->vbi_cap.service_lines,
> +				    k->vbi_cap.service_lines,
> +				    sizeof(k->vbi_cap.service_lines))
> +		    || put_user_conv(k->vbi_cap.type, &u->vbi_cap.type)
> +		    || copy_to_user(u->vbi_cap.reserved, k->vbi_cap.reserved,
> +				    sizeof(k->vbi_cap.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_S_HW_FREQ_SEEK_ENUM:
> +		if (put_user(k->freq_seek.tuner, &u->freq_seek.tuner)
> +		    || put_user_conv(k->freq_seek.type, &u->freq_seek.type)
> +		    || put_user(k->freq_seek.seek_upward,
> +				&u->freq_seek.seek_upward)
> +		    || put_user(k->freq_seek.wrap_around,
> +				&u->freq_seek.wrap_around)
> +		    || put_user(k->freq_seek.spacing, &u->freq_seek.spacing)
> +		    || copy_to_user(u->freq_seek.reserved,
> +				    k->freq_seek.reserved,
> +				    sizeof(k->freq_seek.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	case VIDIOC_CREATE_BUFS_ENUM:
> +		if (put_user(k->create_bufs.index, &u->create_bufs.index)
> +		    || put_user(k->create_bufs.count, &u->create_bufs.count)
> +		    || put_user_conv(k->create_bufs.memory,
> +				     &u->create_bufs.memory)
> +		    || put_user_format(&k->create_bufs.format,
> +				       &u->create_bufs.format)
> +		    || copy_to_user(u->create_bufs.reserved,
> +				    k->create_bufs.reserved,
> +				    sizeof(k->create_bufs.reserved)))
> +			return -EFAULT;
> +		return 0;
> +	default:
> +		WARN(1, "%s: bad compat cmd %8.8x\n", __func__, cmd);
> +		return -EINVAL;
> +	}
> +}
> +
> +#else /* CONFIG_V4L2_COMPAT */
> +
> +static inline unsigned int get_non_compat_cmd(unsigned int cmd)
> +{
> +	return cmd;
> +}
> +
> +static inline long copy_compat_from_user(unsigned int cmd, void __user *arg,
> +					 void *parg)
> +{
> +	return 0;
> +}
> +
> +static inline long copy_compat_to_user(unsigned int cmd, void __user *arg,
> +				       void *parg)
> +{
> +	return 0;
> +}
> +
> +#endif /* CONFIG_V4L2_COMPAT */
> +
>  /* In some cases, only a few fields are used as input, i.e. when the app sets
>   * "index" and then the driver fills in the rest of the structure for the thing
>   * with that index.  We only need to copy up the first non-input field.  */
> @@ -2390,7 +3037,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>  }
>  
>  long
> -video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
> +video_usercopy(struct file *file, unsigned int compat_cmd, unsigned long arg,
>  	       v4l2_kioctl func)
>  {
>  	char	sbuf[128];
> @@ -2401,6 +3048,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>  	size_t  array_size = 0;
>  	void __user *user_ptr = NULL;
>  	void	**kernel_ptr = NULL;
> +	unsigned int cmd = get_non_compat_cmd(compat_cmd);

This will put a penalty on archs where sizeof(u32) == sizeof(enum), with covers
most of the cases.

My suggestion is to, instead, call it at the end of  __video_do_ioctl, at the
"default" clause, with a recursive call to __video_do_ioctl().

It should be noticed that UVC driver and pvrusb2 are the only two drivers that
don't use __video_do_ioctl(). So, a logic similar to it should be implemented
there.

>  
>  	/*  Copy arguments into temp kernel buffer  */
>  	if (_IOC_DIR(cmd) != _IOC_NONE) {
> @@ -2418,12 +3066,23 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>  		if (_IOC_DIR(cmd) & _IOC_WRITE) {
>  			unsigned long n = cmd_input_size(cmd);
>  
> -			if (copy_from_user(parg, (void __user *)arg, n))
> -				goto out;
> -
> -			/* zero out anything we don't copy from userspace */
> -			if (n < _IOC_SIZE(cmd))
> -				memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n);
> +			if (cmd == compat_cmd) {
> +				if (copy_from_user(
> +					    parg, (void __user *)arg, n))
> +					goto out;
> +
> +				/*
> +				 * zero out anything we don't copy
> +				 * from userspace
> +				 */
> +				if (n < _IOC_SIZE(cmd))
> +					memset((u8 *)parg + n, 0,
> +					       _IOC_SIZE(cmd) - n);
> +			} else {
> +				if (copy_compat_from_user(compat_cmd, parg,
> +							  (void __user *)arg))
> +					goto out;
> +			}
>  		} else {
>  			/* read-only ioctl */
>  			memset(parg, 0, _IOC_SIZE(cmd));
> @@ -2471,8 +3130,15 @@ out_array_args:
>  	switch (_IOC_DIR(cmd)) {
>  	case _IOC_READ:
>  	case (_IOC_WRITE | _IOC_READ):
> -		if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
> -			err = -EFAULT;
> +		if (cmd == compat_cmd) {
> +			if (copy_to_user((void __user *)arg, parg,
> +					 _IOC_SIZE(cmd)))
> +				err = -EFAULT;
> +		} else {
> +			if (copy_compat_to_user(compat_cmd, (void __user *)arg,
> +						parg))
> +				err = -EFAULT;
> +		}
>  		break;
>  	}
>  

Re: [RFC v3 2/2] v4l: Implement compat functions for enum to __u32 change

[Laurent Pinchart]

Hi Mauro,

On Wednesday 02 May 2012 19:32:23 Mauro Carvalho Chehab wrote:
> Em 02-05-2012 16:13, Sakari Ailus escreveu:
> > Implement compat functions to provide conversion between structs
> > containing enums and those not. The functions are intended to be removed
> > when the support for such old binaries is no longer necessary.
> 
> This is not a full review of this patch, as checking the full logic
> will consume some time.
> 
> This is just a few points to consider.

[snip]

> > -video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
> > +video_usercopy(struct file *file, unsigned int compat_cmd, unsigned long
> > arg,> 
> >  	       v4l2_kioctl func)
> >  
> >  {
> >  
> >  	char	sbuf[128];
> > 
> > @@ -2401,6 +3048,7 @@ video_usercopy(struct file *file, unsigned int cmd,
> > unsigned long arg,> 
> >  	size_t  array_size = 0;
> >  	void __user *user_ptr = NULL;
> >  	void	**kernel_ptr = NULL;
> > 
> > +	unsigned int cmd = get_non_compat_cmd(compat_cmd);
> 
> This will put a penalty on archs where sizeof(u32) == sizeof(enum), with
> covers most of the cases.
> 
> My suggestion is to, instead, call it at the end of  __video_do_ioctl, at
> the "default" clause, with a recursive call to __video_do_ioctl().

Would that work for "has_array_args" ioctls ? video_usercopy() won't copy the 
array. The compat code could possibly handle that though.

What about making get_non_compat_cmd() return its argument directly when 
sizeof(__u32) == sizeof(enum) ? The performance impact should be negligible.

> It should be noticed that UVC driver and pvrusb2 are the only two drivers
> that don't use __video_do_ioctl(). So, a logic similar to it should be
> implemented there.
> 
> >  	/*  Copy arguments into temp kernel buffer  */
> >  	if (_IOC_DIR(cmd) != _IOC_NONE) {
> > 
> > @@ -2418,12 +3066,23 @@ video_usercopy(struct file *file, unsigned int
> > cmd, unsigned long arg,> 
> >  		if (_IOC_DIR(cmd) & _IOC_WRITE) {
> >  		
> >  			unsigned long n = cmd_input_size(cmd);
> > 
> > -			if (copy_from_user(parg, (void __user *)arg, n))
> > -				goto out;
> > -
> > -			/* zero out anything we don't copy from userspace */
> > -			if (n < _IOC_SIZE(cmd))
> > -				memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n);
> > +			if (cmd == compat_cmd) {
> > +				if (copy_from_user(
> > +					    parg, (void __user *)arg, n))
> > +					goto out;
> > +
> > +				/*
> > +				 * zero out anything we don't copy
> > +				 * from userspace
> > +				 */
> > +				if (n < _IOC_SIZE(cmd))
> > +					memset((u8 *)parg + n, 0,
> > +					       _IOC_SIZE(cmd) - n);
> > +			} else {
> > +				if (copy_compat_from_user(compat_cmd, parg,
> > +							  (void __user *)arg))
> > +					goto out;
> > +			}
> > 
> >  		} else {
> >  		
> >  			/* read-only ioctl */
> >  			memset(parg, 0, _IOC_SIZE(cmd));
> > 
> > @@ -2471,8 +3130,15 @@ out_array_args:
> >  	switch (_IOC_DIR(cmd)) {
> >  	case _IOC_READ:
> > 
> >  	case (_IOC_WRITE | _IOC_READ):
> > -		if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
> > -			err = -EFAULT;
> > +		if (cmd == compat_cmd) {
> > +			if (copy_to_user((void __user *)arg, parg,
> > +					 _IOC_SIZE(cmd)))
> > +				err = -EFAULT;
> > +		} else {
> > +			if (copy_compat_to_user(compat_cmd, (void __user *)arg,
> > +						parg))
> > +				err = -EFAULT;
> > +		}
> > 
> >  		break;
> >  	
> >  	}

-- 
Regards,

Laurent Pinchart

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Andy Walls]

On Wed, 2012-05-02 at 19:17 -0300, Mauro Carvalho Chehab wrote:

> We can speed-up the conversions, with something like:
> 
> enum foo {
> 	BAR
> };
> 
> if (sizeof(foo) != sizeof(u32))
> 	call_compat_logic().
> 
> I suspect that sizeof() won't work inside a macro. 

sizeof() is evaluated at compile time, after preprocessing. 
It should work inside of a macro.

See the ARRAY_SIZE() macro in include/linux/kernel.h for a well tested
example.

Regards,
Andy

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Hans Verkuil]

On Wed May 2 2012 23:39:15 Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, May 02, 2012 at 10:45:22PM +0200, Hans Verkuil wrote:
> > On Wed May 2 2012 21:13:47 Sakari Ailus wrote:
> > > Replace enums in IOCTL structs by __u32. The size of enums is variable and
> > > thus problematic. Compatibility structs having exactly the same as original
> > > definition are provided for compatibility with old binaries with the
> > > required conversion code.
> > 
> > Does someone actually have hard proof that there really is a problem? You know,
> > demonstrate it with actual example code?
> > 
> > It's pretty horrible that you have to do all those conversions and that code
> > will be with us for years to come.
> > 
> > For most (if not all!) architectures sizeof(enum) == sizeof(u32), so there is
> > no need for any compat code for those.
> 
> Cases I know where this can go wrong are, but there may well be others:
> 
> - ppc64: int is 64 bits there, and thus also enums,

Are you really, really certain that's the case? If I look at
arch/powerpc/include/asm/types.h it includes either asm-generic/int-l64.h
or asm-generic/int-ll64.h and both of those headers define u32 as unsigned int.
Also, if sizeof(int) != 4, then how would you define u32?

Ask a ppc64 kernel maintainer what sizeof(int) and sizeof(enum) are in the kernel
before we start doing lots of work for no reason.

Looking at arch/*/include/asm/types.h it seems all architectures define sizeof(int)
as 4.

What sizeof(long) is will actually differ between architectures, but char, short
and int seem to be fixed everywhere.

Regards,

	Hans

> 
> - Enums are quite a different concept in C++ than in C --- the compiler may
>   make assumpton based on the value range of the enums --- videodev2.h should
>   be included with extern "C" in that case, though,
> 
> - C does not specify which integer type enums actually use; this is what GCC
>   manual says about it:
> 
>   <URL:http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Enumerations>
> 
>   So a compiler other than GCC should use 16-bit enums and conform to C
>   while breaking V4L2. This might be a theoretical issue, though.
> 
> More discussion took place in this thread:
> 
> <URL:http://www.spinics.net/lists/linux-media/msg46167.html>
> 
> Regards,
> 
> 

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Mauro Carvalho Chehab]

Em 02-05-2012 21:42, Andy Walls escreveu:
> On Wed, 2012-05-02 at 19:17 -0300, Mauro Carvalho Chehab wrote:
> 
>> We can speed-up the conversions, with something like:
>>
>> enum foo {
>> 	BAR
>> };
>>
>> if (sizeof(foo) != sizeof(u32))
>> 	call_compat_logic().
>>
>> I suspect that sizeof() won't work inside a macro. 
> 
> sizeof() is evaluated at compile time, after preprocessing. 
> It should work inside of a macro.

I tried to compile this small piece of code:

enum foo { BAR };
#if sizeof(foo) != sizeof(int)
void main(void) { printf("different sizes\n"); }
#else
void main(void) { printf("same size\n"); }
#endif

It gives an error:

/tmp/foo.c:2:11: error: missing binary operator before token "("

So, either this doesn't work, because sizeof() is evaluated too late,
or some trick is needed.

Weird enough, cpp generates the error, but the expression is well-evaluated:

$ cpp /tmp/foo.c
# 1 "/tmp/foo.c"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "/tmp/foo.c"
/tmp/foo.c:2:11: error: missing binary operator before token "("
enum foo { BAR };



void main(void) { printf("same size\n"); }


Changing from "sizeof(foo)" to "sizeof foo" also doesn't solve:

/tmp/foo.c:2:12: error: missing binary operator before token "foo"

Maybe some trick is needed for it to work.

> See the ARRAY_SIZE() macro in include/linux/kernel.h for a well tested
> example.

ARRAY_SIZE() doesn't have an #if on it.

Regards,
Mauro

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Sakari Ailus]

Hi Mauro,

Mauro Carvalho Chehab wrote:
> Em 02-05-2012 21:42, Andy Walls escreveu:
>> On Wed, 2012-05-02 at 19:17 -0300, Mauro Carvalho Chehab wrote:
>>
>>> We can speed-up the conversions, with something like:
>>>
>>> enum foo {
>>> 	BAR
>>> };
>>>
>>> if (sizeof(foo) != sizeof(u32))
>>> 	call_compat_logic().
>>>
>>> I suspect that sizeof() won't work inside a macro.
>>
>> sizeof() is evaluated at compile time, after preprocessing.
>> It should work inside of a macro.
>
> I tried to compile this small piece of code:
>
> enum foo { BAR };
> #if sizeof(foo) != sizeof(int)
> void main(void) { printf("different sizes\n"); }
> #else
> void main(void) { printf("same size\n"); }
> #endif
>
> It gives an error:
>
> /tmp/foo.c:2:11: error: missing binary operator before token "("
>
> So, either this doesn't work, because sizeof() is evaluated too late,
> or some trick is needed.
>
> Weird enough, cpp generates the error, but the expression is well-evaluated:
>
> $ cpp /tmp/foo.c
> # 1 "/tmp/foo.c"
> # 1 "<built-in>"
> # 1 "<command-line>"
> # 1 "/tmp/foo.c"
> /tmp/foo.c:2:11: error: missing binary operator before token "("
> enum foo { BAR };

sizeof() is processed by C compiler while #if is preprocessor directive, 
and its arguments have to be evaluable by the preprocessor, which is the 
problem here.

The C compiler can also optimise away things like that but it's more 
difficult to see whether that takes place or not; one would need to look 
at the resulting assembly code.

Regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Mauro Carvalho Chehab]

Em 02-05-2012 21:42, Andy Walls escreveu:
> On Wed, 2012-05-02 at 19:17 -0300, Mauro Carvalho Chehab wrote:
> 
>> We can speed-up the conversions, with something like:
>>
>> enum foo {
>> 	BAR
>> };
>>
>> if (sizeof(foo) != sizeof(u32))
>> 	call_compat_logic().
>>
>> I suspect that sizeof() won't work inside a macro. 
> 
> sizeof() is evaluated at compile time, after preprocessing. 
> It should work inside of a macro.

According with Dennis Ritchie, testing for sizeof on a macro never worked:
	http://groups.google.com/group/comp.std.c/msg/4852afc61a060d89?dmode=source&pli=1

Regards,
Mauro

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Sylwester Nawrocki]

On 05/03/2012 12:22 PM, Mauro Carvalho Chehab wrote:
> Em 02-05-2012 21:42, Andy Walls escreveu:
>> On Wed, 2012-05-02 at 19:17 -0300, Mauro Carvalho Chehab wrote:
>>
>>> We can speed-up the conversions, with something like:
>>>
>>> enum foo {
>>> 	BAR
>>> };
>>>
>>> if (sizeof(foo) != sizeof(u32))
>>> 	call_compat_logic().
>>>
>>> I suspect that sizeof() won't work inside a macro.
>>
>> sizeof() is evaluated at compile time, after preprocessing.
>> It should work inside of a macro.
>
> I tried to compile this small piece of code:
>
> enum foo { BAR };
> #if sizeof(foo) != sizeof(int)
> void main(void) { printf("different sizes\n"); }
> #else
> void main(void) { printf("same size\n"); }
> #endif
>
> It gives an error:
>
> /tmp/foo.c:2:11: error: missing binary operator before token "("
>
> So, either this doesn't work, because sizeof() is evaluated too late,
> or some trick is needed.

The GCC C preprocessor documentation [1] states it won't work that way:

"The preprocessor does not know anything about types in the language. 
Therefore, sizeof operators are not recognized in `#if', and neither are 
enum constants. They will be taken as identifiers which are not macros, 
and replaced by zero. In the case of sizeof, this is likely to cause the 
expression to be invalid."

[1] http://gcc.gnu.org/onlinedocs/cpp/If.html#If


Regards,
Sylwester

> Weird enough, cpp generates the error, but the expression is well-evaluated:
>
> $ cpp /tmp/foo.c
> # 1 "/tmp/foo.c"
> # 1 "<built-in>"
> # 1 "<command-line>"
> # 1 "/tmp/foo.c"
> /tmp/foo.c:2:11: error: missing binary operator before token "("
> enum foo { BAR };
>
>
>
> void main(void) { printf("same size\n"); }
>
>
> Changing from "sizeof(foo)" to "sizeof foo" also doesn't solve:
>
> /tmp/foo.c:2:12: error: missing binary operator before token "foo"
>
> Maybe some trick is needed for it to work.
>
>> See the ARRAY_SIZE() macro in include/linux/kernel.h for a well tested
>> example.
>
> ARRAY_SIZE() doesn't have an #if on it.
>
> Regards,
> Mauro

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Rémi Denis-Courmont]

On Thu, 3 May 2012 00:39:15 +0300, Sakari Ailus <sakari.ailus@iki.fi>

wrote:

> - ppc64: int is 64 bits there, and thus also enums,



Really?



(e)glibc assumes that signed int and unsigned int are 32-bits on all

platforms. From bits/types.h:



typedef signed int __int32_t;

typedef unsigned int __uint32_t;



> - C does not specify which integer type enums actually use; this is what

> GCC manual says about it:



The Linux ABI, at least as defined in GCC, requires 'short enums' be

disabled, even on ARM.

So enums should always be unsigned or int with gcc, thus with V4L2 code.



-- 

Rémi Denis-Courmont

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Rémi Denis-Courmont]

Answering myself.



On Thu, 03 May 2012 12:57:00 +0200, Rémi Denis-Courmont <remi@remlab.net>

wrote:

> On Thu, 3 May 2012 00:39:15 +0300, Sakari Ailus <sakari.ailus@iki.fi>

> wrote:

>> - ppc64: int is 64 bits there, and thus also enums,

> 

> Really?



No, really not:

http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#FUND-TYPE



-- 

Rémi Denis-Courmont

Sent from my collocated server

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Mauro Carvalho Chehab]

Em 03-05-2012 07:35, Sakari Ailus escreveu:
> Hi Mauro,
> 
> Mauro Carvalho Chehab wrote:
>> Em 02-05-2012 21:42, Andy Walls escreveu:
>>> On Wed, 2012-05-02 at 19:17 -0300, Mauro Carvalho Chehab wrote:
>>>
>>>> We can speed-up the conversions, with something like:
>>>>
>>>> enum foo {
>>>>     BAR
>>>> };
>>>>
>>>> if (sizeof(foo) != sizeof(u32))
>>>>     call_compat_logic().
>>>>
>>>> I suspect that sizeof() won't work inside a macro.
>>>
>>> sizeof() is evaluated at compile time, after preprocessing.
>>> It should work inside of a macro.
>>
>> I tried to compile this small piece of code:
>>
>> enum foo { BAR };
>> #if sizeof(foo) != sizeof(int)
>> void main(void) { printf("different sizes\n"); }
>> #else
>> void main(void) { printf("same size\n"); }
>> #endif
>>
>> It gives an error:
>>
>> /tmp/foo.c:2:11: error: missing binary operator before token "("
>>
>> So, either this doesn't work, because sizeof() is evaluated too late,
>> or some trick is needed.
>>
>> Weird enough, cpp generates the error, but the expression is well-evaluated:
>>
>> $ cpp /tmp/foo.c
>> # 1 "/tmp/foo.c"
>> # 1 "<built-in>"
>> # 1 "<command-line>"
>> # 1 "/tmp/foo.c"
>> /tmp/foo.c:2:11: error: missing binary operator before token "("
>> enum foo { BAR };
> 
> sizeof() is processed by C compiler while #if is preprocessor directive, and its arguments have to be evaluable by the preprocessor, which is the problem here.
> 
> The C compiler can also optimise away things like that but it's more difficult to see whether that takes place or not; one would need to look at the resulting assembly code.

This code:

void main(void) {
	if (sizeof(int) == sizeof(char))
	   	printf("same size\n");
	else
		printf("different sizes\n"); 
}

should be evaluated by the compiler as if (0) and should not generate any code.

The assembler for it is:

	.file	"foo.c"
	.section	.rodata
.LC0:
	.string	"different sizes"
	.text
	.globl	main
	.type	main, @function
main:
.LFB0:
	.cfi_startproc
	pushq	%rbp
	.cfi_def_cfa_offset 16
	.cfi_offset 6, -16
	movq	%rsp, %rbp
	.cfi_def_cfa_register 6
	movl	$.LC0, %edi
	call	puts
	popq	%rbp
	.cfi_def_cfa 7, 8
	ret
	.cfi_endproc
.LFE0:
	.size	main, .-main
	.ident	"GCC: (GNU) 4.6.3 20120306 (Red Hat 4.6.3-2)"
	.section	.note.GNU-stack,"",@progbits

So, gcc will remove the dead code, as expected.

So, the trick is to do something similar to that on the compat code, in order
to avoid any penalties when sizeof(enum) is 32 bits.

Regards,
Mauro

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Sakari Ailus]

Rémi Denis-Courmont wrote:
> Answering myself.
>
> On Thu, 03 May 2012 12:57:00 +0200, Rémi Denis-Courmont<remi@remlab.net>
> wrote:
>> On Thu, 3 May 2012 00:39:15 +0300, Sakari Ailus<sakari.ailus@iki.fi>
>> wrote:
>>> - ppc64: int is 64 bits there, and thus also enums,
>>
>> Really?
>
> No, really not:
> http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi-1.9.html#FUND-TYPE

Right. Someone brought that up AFAIR and I didn't check it from other 
sources. Thanks for the correction.

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [RFC v3 2/2] v4l: Implement compat functions for enum to __u32 change

[Mauro Carvalho Chehab]

Em 02-05-2012 20:38, Laurent Pinchart escreveu:
> Hi Mauro,
> 
> On Wednesday 02 May 2012 19:32:23 Mauro Carvalho Chehab wrote:
>> Em 02-05-2012 16:13, Sakari Ailus escreveu:
>>> Implement compat functions to provide conversion between structs
>>> containing enums and those not. The functions are intended to be removed
>>> when the support for such old binaries is no longer necessary.
>>
>> This is not a full review of this patch, as checking the full logic
>> will consume some time.
>>
>> This is just a few points to consider.
> 
> [snip]
> 
>>> -video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>>> +video_usercopy(struct file *file, unsigned int compat_cmd, unsigned long
>>> arg,> 
>>>  	       v4l2_kioctl func)
>>>  
>>>  {
>>>  
>>>  	char	sbuf[128];
>>>
>>> @@ -2401,6 +3048,7 @@ video_usercopy(struct file *file, unsigned int cmd,
>>> unsigned long arg,> 
>>>  	size_t  array_size = 0;
>>>  	void __user *user_ptr = NULL;
>>>  	void	**kernel_ptr = NULL;
>>>
>>> +	unsigned int cmd = get_non_compat_cmd(compat_cmd);
>>
>> This will put a penalty on archs where sizeof(u32) == sizeof(enum), with
>> covers most of the cases.
>>
>> My suggestion is to, instead, call it at the end of  __video_do_ioctl, at
>> the "default" clause, with a recursive call to __video_do_ioctl().
> 
> Would that work for "has_array_args" ioctls ? video_usercopy() won't copy the 
> array. The compat code could possibly handle that though.
> 
> What about making get_non_compat_cmd() return its argument directly when 
> sizeof(__u32) == sizeof(enum) ? The performance impact should be negligible.

Good idea. GCC optimizer will probably just discard the entire code, by merging
'compat_cmd' var with 'cmd' var.

Regards,
Mauro

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Mauro Carvalho Chehab]

Em 03-05-2012 04:02, Hans Verkuil escreveu:
> On Wed May 2 2012 23:39:15 Sakari Ailus wrote:
>> Hi Hans,
>>
>> On Wed, May 02, 2012 at 10:45:22PM +0200, Hans Verkuil wrote:
>>> On Wed May 2 2012 21:13:47 Sakari Ailus wrote:
>>>> Replace enums in IOCTL structs by __u32. The size of enums is variable and
>>>> thus problematic. Compatibility structs having exactly the same as original
>>>> definition are provided for compatibility with old binaries with the
>>>> required conversion code.
>>>
>>> Does someone actually have hard proof that there really is a problem? You know,
>>> demonstrate it with actual example code?
>>>
>>> It's pretty horrible that you have to do all those conversions and that code
>>> will be with us for years to come.
>>>
>>> For most (if not all!) architectures sizeof(enum) == sizeof(u32), so there is
>>> no need for any compat code for those.
>>
>> Cases I know where this can go wrong are, but there may well be others:
>>
>> - ppc64: int is 64 bits there, and thus also enums,
> 
> Are you really, really certain that's the case? If I look at
> arch/powerpc/include/asm/types.h it includes either asm-generic/int-l64.h
> or asm-generic/int-ll64.h and both of those headers define u32 as unsigned int.
> Also, if sizeof(int) != 4, then how would you define u32?
> 
> Ask a ppc64 kernel maintainer what sizeof(int) and sizeof(enum) are in the kernel
> before we start doing lots of work for no reason.
> 
> Looking at arch/*/include/asm/types.h it seems all architectures define sizeof(int)
> as 4.
> 
> What sizeof(long) is will actually differ between architectures, but char, short
> and int seem to be fixed everywhere.

Yes, it seems that, inside the Kernel, "int" it will always be 32 bits. It doesn't
necessarily means that "enum" will be 32 bits.

Also, as it is recommended to not use "int/unsigned int/long/unsigned long" at 
kernel<->userspace API, I bet that this will cause problems on userspace (maybe
with non-gcc compilers?)

Regards,
Mauro

[PATCH] edac: Change internal representation to work with layers

Change the EDAC internal representation to work with non-csrow
based memory controllers.

There are lots of those memory controllers nowadays, and more
are coming. So, the EDAC internal representation needs to be
changed, in order to work with those memory controllers, while
preserving backward compatibility with the old ones.

The edac core was written with the idea that memory controllers
are able to directly access csrows.

This is not true for FB-DIMM and RAMBUS memory controllers.

Also, some recent advanced memory controllers don't present a per-csrows
view. Instead, they view memories as DIMMs, instead of ranks.

So, change the allocation and error report routines to allow
them to work with all types of architectures.

This will allow the removal of several hacks with FB-DIMM and RAMBUS
memory controllers.

Also, several tests were done on different platforms using different
x86 drivers.

TODO: a multi-rank DIMMs are currently represented by multiple DIMM
entries in struct dimm_info. That means that changing a label for one
rank won't change the same label for the other ranks at the same DIMM.
This bug is present since the beginning of the EDAC, so it is not a big
deal. However, on several drivers, it is possible to fix this issue, but
it should be a per-driver fix, as the csrow => DIMM arrangement may not
be equal for all. So, don't try to fix it here yet.

I tried to make this patch as short as possible, preceding it with
several other patches that simplified the logic here. Yet, as the
internal API changes, all drivers need changes. The changes are
generally bigger in the drivers for FB-DIMMs.

Cc: Aristeu Rozanski <arozansk@redhat.com>
Cc: Doug Thompson <norsk5@yahoo.com>
Cc: Borislav Petkov <borislav.petkov@amd.com>
Cc: Mark Gross <mark.gross@intel.com>
Cc: Jason Uhlenkott <juhlenko@akamai.com>
Cc: Tim Small <tim@buttersideup.com>
Cc: Ranganathan Desikan <ravi@jetztechnologies.com>
Cc: "Arvind R." <arvino55@gmail.com>
Cc: Olof Johansson <olof@lixom.net>
Cc: Egor Martovetsky <egor@pasemi.com>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Joe Perches <joe@perches.com>
Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Hitoshi Mitake <h.mitake@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Niklas Söderlund" <niklas.soderlund@ericsson.com>
Cc: Shaohui Xie <Shaohui.Xie@freescale.com>
Cc: Josh Boyer <jwboyer@gmail.com>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/edac/edac_core.h b/drivers/edac/edac_core.h
index e48ab31..1286c5e 100644
--- a/drivers/edac/edac_core.h
+++ b/drivers/edac/edac_core.h
@@ -447,8 +447,12 @@ static inline void pci_write_bits32(struct pci_dev *pdev, int offset,
 
 #endif				/* CONFIG_PCI */
 
-extern struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
-					  unsigned nr_chans, int edac_index);
+struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
+				   unsigned nr_chans, int edac_index);
+struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
+				   unsigned n_layers,
+				   struct edac_mc_layer *layers,
+				   unsigned sz_pvt);
 extern int edac_mc_add_mc(struct mem_ctl_info *mci);
 extern void edac_mc_free(struct mem_ctl_info *mci);
 extern struct mem_ctl_info *edac_mc_find(int idx);
@@ -467,24 +471,78 @@ extern int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci,
  * reporting logic and function interface - reduces conditional
  * statement clutter and extra function arguments.
  */
-extern void edac_mc_handle_ce(struct mem_ctl_info *mci,
-			      unsigned long page_frame_number,
-			      unsigned long offset_in_page,
-			      unsigned long syndrome, int row, int channel,
-			      const char *msg);
-extern void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci,
-				      const char *msg);
-extern void edac_mc_handle_ue(struct mem_ctl_info *mci,
-			      unsigned long page_frame_number,
-			      unsigned long offset_in_page, int row,
-			      const char *msg);
-extern void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci,
-				      const char *msg);
-extern void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci, unsigned int csrow,
-				  unsigned int channel0, unsigned int channel1,
-				  char *msg);
-extern void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci, unsigned int csrow,
-				  unsigned int channel, char *msg);
+
+void edac_mc_handle_error(const enum hw_event_mc_err_type type,
+			  struct mem_ctl_info *mci,
+			  const unsigned long page_frame_number,
+			  const unsigned long offset_in_page,
+			  const unsigned long syndrome,
+			  const int layer0,
+			  const int layer1,
+			  const int layer2,
+			  const char *msg,
+			  const char *other_detail,
+			  const void *mcelog);
+
+static inline void edac_mc_handle_ce(struct mem_ctl_info *mci,
+				     unsigned long page_frame_number,
+				     unsigned long offset_in_page,
+				     unsigned long syndrome, int row, int channel,
+				     const char *msg)
+{
+	 edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+			      page_frame_number, offset_in_page, syndrome,
+			      row, channel, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci,
+					     const char *msg)
+{
+	 edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+			      0, 0, 0, -1, -1, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_ue(struct mem_ctl_info *mci,
+				     unsigned long page_frame_number,
+				     unsigned long offset_in_page, int row,
+				     const char *msg)
+{
+	 edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+			      page_frame_number, offset_in_page, 0,
+			      row, -1, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci,
+					     const char *msg)
+{
+	 edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+			      0, 0, 0, -1, -1, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
+					 unsigned int csrow,
+					 unsigned int channel0,
+					 unsigned int channel1,
+					 char *msg)
+{
+	/*
+	 *FIXME: The error can also be at channel1 (e. g. at the second
+	 *	  channel of the same branch). The fix is to push
+	 *	  edac_mc_handle_error() call into each driver
+	 */
+	 edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+			      0, 0, 0,
+			      csrow, channel0, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
+					 unsigned int csrow,
+					 unsigned int channel, char *msg)
+{
+	 edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+			      0, 0, 0,
+			      csrow, channel, -1, msg, NULL, NULL);
+}
 
 /*
  * edac_device APIs
@@ -496,6 +554,7 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
 extern void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
 				int inst_nr, int block_nr, const char *msg);
 extern int edac_device_alloc_index(void);
+extern const char *edac_layer_name[];
 
 /*
  * edac_pci APIs
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 6ec967a..10cebb8 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -44,9 +44,25 @@ static void edac_mc_dump_channel(struct rank_info *chan)
 	debugf4("\tchannel = %p\n", chan);
 	debugf4("\tchannel->chan_idx = %d\n", chan->chan_idx);
 	debugf4("\tchannel->csrow = %p\n\n", chan->csrow);
-	debugf4("\tdimm->ce_count = %d\n", chan->dimm->ce_count);
-	debugf4("\tdimm->label = '%s'\n", chan->dimm->label);
-	debugf4("\tdimm->nr_pages = 0x%x\n", chan->dimm->nr_pages);
+	debugf4("\tchannel->dimm = %p\n", chan->dimm);
+}
+
+static void edac_mc_dump_dimm(struct dimm_info *dimm)
+{
+	int i;
+
+	debugf4("\tdimm = %p\n", dimm);
+	debugf4("\tdimm->label = '%s'\n", dimm->label);
+	debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
+	debugf4("\tdimm location ");
+	for (i = 0; i < dimm->mci->n_layers; i++) {
+		printk(KERN_CONT "%d", dimm->location[i]);
+		if (i < dimm->mci->n_layers - 1)
+			printk(KERN_CONT ".");
+	}
+	printk(KERN_CONT "\n");
+	debugf4("\tdimm->grain = %d\n", dimm->grain);
+	debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
 }
 
 static void edac_mc_dump_csrow(struct csrow_info *csrow)
@@ -70,6 +86,8 @@ static void edac_mc_dump_mci(struct mem_ctl_info *mci)
 	debugf4("\tmci->edac_check = %p\n", mci->edac_check);
 	debugf3("\tmci->nr_csrows = %d, csrows = %p\n",
 		mci->nr_csrows, mci->csrows);
+	debugf3("\tmci->nr_dimms = %d, dimms = %p\n",
+		mci->tot_dimms, mci->dimms);
 	debugf3("\tdev = %p\n", mci->dev);
 	debugf3("\tmod_name:ctl_name = %s:%s\n", mci->mod_name, mci->ctl_name);
 	debugf3("\tpvt_info = %p\n\n", mci->pvt_info);
@@ -157,10 +175,20 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
 }
 
 /**
- * edac_mc_alloc: Allocate a struct mem_ctl_info structure
- * @size_pvt:	size of private storage needed
- * @nr_csrows:	Number of CWROWS needed for this MC
- * @nr_chans:	Number of channels for the MC
+ * edac_mc_alloc: Allocate and partially fill a struct mem_ctl_info structure
+ * @mc_num:		Memory controller number
+ * @n_layers:		Number of MC hierarchy layers
+ * layers:		Describes each layer as seen by the Memory Controller
+ * @size_pvt:		size of private storage needed
+ *
+ *
+ * Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
+ * such DIMMS properly, but the CSROWS-based ones will likely do the wrong
+ * thing, as two chip select values are used for dual-rank memories (and 4, for
+ * quad-rank ones). I suspect that this issue could be solved inside the EDAC
+ * core for SDRAM memories, but it requires further study at JEDEC JESD 21C.
+ *
+ * In summary, solving this issue is not easy, as it requires a lot of testing.
  *
  * Everything is kmalloc'ed as one big chunk - more efficient.
  * Only can be used if all structures have the same lifetime - otherwise
@@ -168,22 +196,49 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
  *
  * Use edac_mc_free() to free mc structures allocated by this function.
  *
+ * NOTE: drivers handle multi-rank memories in different ways: in some
+ * drivers, one multi-rank memory stick is mapped as one entry, while, in
+ * others, a single multi-rank memory stick would be mapped into several
+ * entries. Currently, this function will allocate multiple struct dimm_info
+ * on such scenarios, as grouping the multiple ranks require drivers change.
+ *
  * Returns:
  *	NULL allocation failed
  *	struct mem_ctl_info pointer
  */
-struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
-				unsigned nr_chans, int edac_index)
+struct mem_ctl_info *new_edac_mc_alloc(unsigned mc_num,
+				       unsigned n_layers,
+				       struct edac_mc_layer *layers,
+				       unsigned sz_pvt)
 {
-	void *ptr = NULL;
 	struct mem_ctl_info *mci;
-	struct csrow_info *csi, *csrow;
+	struct edac_mc_layer *layer;
+	struct csrow_info *csi, *csr;
 	struct rank_info *chi, *chp, *chan;
 	struct dimm_info *dimm;
-	void *pvt;
-	unsigned size;
-	int row, chn;
-	int err;
+	u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
+	unsigned pos[EDAC_MAX_LAYERS];
+	void *pvt, *ptr = NULL;
+	unsigned size, tot_dimms = 1, count = 1;
+	unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;
+	int i, j, err, row, chn;
+	bool per_rank = false;
+
+	BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
+	/*
+	 * Calculate the total amount of dimms and csrows/cschannels while
+	 * in the old API emulation mode
+	 */
+	for (i = 0; i < n_layers; i++) {
+		tot_dimms *= layers[i].size;
+		if (layers[i].is_virt_csrow)
+			tot_csrows *= layers[i].size;
+		else
+			tot_channels *= layers[i].size;
+
+		if (layers[i].type == EDAC_MC_LAYER_CHIP_SELECT)
+			per_rank = true;
+	}
 
 	/* Figure out the offsets of the various items from the start of an mc
 	 * structure.  We want the alignment of each item to be at least as
@@ -191,12 +246,27 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
 	 * hardcode everything into a single struct.
 	 */
 	mci = edac_align_ptr(&ptr, sizeof(*mci), 1);
-	csi = edac_align_ptr(&ptr, sizeof(*csi), nr_csrows);
-	chi = edac_align_ptr(&ptr, sizeof(*chi), nr_csrows * nr_chans);
-	dimm = edac_align_ptr(&ptr, sizeof(*dimm), nr_csrows * nr_chans);
+	layer = edac_align_ptr(&ptr, sizeof(*layer), n_layers);
+	csi = edac_align_ptr(&ptr, sizeof(*csi), tot_csrows);
+	chi = edac_align_ptr(&ptr, sizeof(*chi), tot_csrows * tot_channels);
+	dimm = edac_align_ptr(&ptr, sizeof(*dimm), tot_dimms);
+	for (i = 0; i < n_layers; i++) {
+		count *= layers[i].size;
+		debugf4("%s: errcount layer %d size %d\n", __func__, i, count);
+		ce_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
+		ue_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
+		tot_errcount += 2 * count;
+	}
+
+	debugf4("%s: allocating %d error counters\n", __func__, tot_errcount);
 	pvt = edac_align_ptr(&ptr, sz_pvt, 1);
 	size = ((unsigned long)pvt) + sz_pvt;
 
+	debugf1("%s(): allocating %u bytes for mci data (%d %s, %d csrows/channels)\n",
+		__func__, size,
+		tot_dimms,
+		per_rank ? "ranks" : "dimms",
+		tot_csrows * tot_channels);
 	mci = kzalloc(size, GFP_KERNEL);
 	if (mci == NULL)
 		return NULL;
@@ -204,42 +274,90 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
 	/* Adjust pointers so they point within the memory we just allocated
 	 * rather than an imaginary chunk of memory located at address 0.
 	 */
+	layer = (struct edac_mc_layer *)(((char *)mci) + ((unsigned long)layer));
 	csi = (struct csrow_info *)(((char *)mci) + ((unsigned long)csi));
 	chi = (struct rank_info *)(((char *)mci) + ((unsigned long)chi));
 	dimm = (struct dimm_info *)(((char *)mci) + ((unsigned long)dimm));
+	for (i = 0; i < n_layers; i++) {
+		mci->ce_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ce_per_layer[i]));
+		mci->ue_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ue_per_layer[i]));
+	}
 	pvt = sz_pvt ? (((char *)mci) + ((unsigned long)pvt)) : NULL;
 
 	/* setup index and various internal pointers */
-	mci->mc_idx = edac_index;
+	mci->mc_idx = mc_num;
 	mci->csrows = csi;
 	mci->dimms  = dimm;
+	mci->tot_dimms = tot_dimms;
 	mci->pvt_info = pvt;
-	mci->nr_csrows = nr_csrows;
+	mci->n_layers = n_layers;
+	mci->layers = layer;
+	memcpy(mci->layers, layers, sizeof(*layer) * n_layers);
+	mci->nr_csrows = tot_csrows;
+	mci->num_cschannel = tot_channels;
+	mci->mem_is_per_rank = per_rank;
 
 	/*
-	 * For now, assumes that a per-csrow arrangement for dimms.
-	 * This will be latter changed.
+	 * Fill the csrow struct
 	 */
-	dimm = mci->dimms;
-
-	for (row = 0; row < nr_csrows; row++) {
-		csrow = &csi[row];
-		csrow->csrow_idx = row;
-		csrow->mci = mci;
-		csrow->nr_channels = nr_chans;
-		chp = &chi[row * nr_chans];
-		csrow->channels = chp;
-
-		for (chn = 0; chn < nr_chans; chn++) {
+	for (row = 0; row < tot_csrows; row++) {
+		csr = &csi[row];
+		csr->csrow_idx = row;
+		csr->mci = mci;
+		csr->nr_channels = tot_channels;
+		chp = &chi[row * tot_channels];
+		csr->channels = chp;
+
+		for (chn = 0; chn < tot_channels; chn++) {
 			chan = &chp[chn];
 			chan->chan_idx = chn;
-			chan->csrow = csrow;
+			chan->csrow = csr;
+		}
+	}
 
-			mci->csrows[row].channels[chn].dimm = dimm;
-			dimm->csrow = row;
-			dimm->csrow_channel = chn;
-			dimm++;
-			mci->nr_dimms++;
+	/*
+	 * Fill the dimm struct
+	 */
+	memset(&pos, 0, sizeof(pos));
+	row = 0;
+	chn = 0;
+	debugf4("%s: initializing %d %s\n", __func__, tot_dimms,
+		per_rank ? "ranks" : "dimms");
+	for (i = 0; i < tot_dimms; i++) {
+		chan = &csi[row].channels[chn];
+		dimm = EDAC_DIMM_PTR(layer, mci->dimms, n_layers,
+			       pos[0], pos[1], pos[2]);
+		dimm->mci = mci;
+
+		debugf2("%s: %d: %s%zd (%d:%d:%d): row %d, chan %d\n", __func__,
+			i, per_rank ? "rank" : "dimm", (dimm - mci->dimms),
+			pos[0], pos[1], pos[2], row, chn);
+
+		/* Copy DIMM location */
+		for (j = 0; j < n_layers; j++)
+			dimm->location[j] = pos[j];
+
+		/* Link it to the csrows old API data */
+		chan->dimm = dimm;
+		dimm->csrow = row;
+		dimm->cschannel = chn;
+
+		/* Increment csrow location */
+		for (j = n_layers - 1; j >= 0; j--)
+			if (layers[j].is_virt_csrow)
+				break;
+		row++;
+		if (row == tot_csrows) {
+			row = 0;
+			chn++;
+		}
+
+		/* Increment dimm location */
+		for (j = n_layers - 1; j >= 0; j--) {
+			pos[j]++;
+			if (pos[j] < layers[j].size)
+				break;
+			pos[j] = 0;
 		}
 	}
 
@@ -263,6 +381,46 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
 	 */
 	return mci;
 }
+EXPORT_SYMBOL_GPL(new_edac_mc_alloc);
+
+/**
+ * edac_mc_alloc: Allocate and partially fill a struct mem_ctl_info structure
+ * @mc_num:		Memory controller number
+ * @n_layers:		Number of layers at the MC hierarchy
+ * layers:		Describes each layer as seen by the Memory Controller
+ * @size_pvt:		Size of private storage needed
+ *
+ *
+ * FIXME: drivers handle multi-rank memories in different ways: some
+ * drivers map multi-ranked DIMMs as one DIMM while others
+ * as several DIMMs.
+ *
+ * Everything is kmalloc'ed as one big chunk - more efficient.
+ * It can only be used if all structures have the same lifetime - otherwise
+ * you have to allocate and initialize your own structures.
+ *
+ * Use edac_mc_free() to free mc structures allocated by this function.
+ *
+ * Returns:
+ *	On failure: NULL
+ *	On success: struct mem_ctl_info pointer
+ */
+
+struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
+				   unsigned nr_chans, int mc_num)
+{
+	unsigned n_layers = 2;
+	struct edac_mc_layer layers[n_layers];
+
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = nr_csrows;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = nr_chans;
+	layers[1].is_virt_csrow = false;
+
+	return new_edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers, sz_pvt);
+}
 EXPORT_SYMBOL_GPL(edac_mc_alloc);
 
 /**
@@ -528,7 +686,6 @@ EXPORT_SYMBOL(edac_mc_find);
  * edac_mc_add_mc: Insert the 'mci' structure into the mci global list and
  *                 create sysfs entries associated with mci structure
  * @mci: pointer to the mci structure to be added to the list
- * @mc_idx: A unique numeric identifier to be assigned to the 'mci' structure.
  *
  * Return:
  *	0	Success
@@ -555,6 +712,8 @@ int edac_mc_add_mc(struct mem_ctl_info *mci)
 				edac_mc_dump_channel(&mci->csrows[i].
 						channels[j]);
 		}
+		for (i = 0; i < mci->tot_dimms; i++)
+			edac_mc_dump_dimm(&mci->dimms[i]);
 	}
 #endif
 	mutex_lock(&mem_ctls_mutex);
@@ -712,261 +871,289 @@ int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci, unsigned long page)
 }
 EXPORT_SYMBOL_GPL(edac_mc_find_csrow_by_page);
 
-/* FIXME - setable log (warning/emerg) levels */
-/* FIXME - integrate with evlog: http://evlog.sourceforge.net/ */
-void edac_mc_handle_ce(struct mem_ctl_info *mci,
-		unsigned long page_frame_number,
-		unsigned long offset_in_page, unsigned long syndrome,
-		int row, int channel, const char *msg)
+const char *edac_layer_name[] = {
+	[EDAC_MC_LAYER_BRANCH] = "branch",
+	[EDAC_MC_LAYER_CHANNEL] = "channel",
+	[EDAC_MC_LAYER_SLOT] = "slot",
+	[EDAC_MC_LAYER_CHIP_SELECT] = "csrow",
+};
+EXPORT_SYMBOL_GPL(edac_layer_name);
+
+static void edac_inc_ce_error(struct mem_ctl_info *mci,
+				    bool enable_per_layer_report,
+				    const int pos[EDAC_MAX_LAYERS])
 {
-	unsigned long remapped_page;
-	char *label = NULL;
-	u32 grain;
+	int i, index = 0;
 
-	debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
+	mci->ce_count++;
 
-	/* FIXME - maybe make panic on INTERNAL ERROR an option */
-	if (row >= mci->nr_csrows || row < 0) {
-		/* something is wrong */
-		edac_mc_printk(mci, KERN_ERR,
-			"INTERNAL ERROR: row out of range "
-			"(%d >= %d)\n", row, mci->nr_csrows);
-		edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
+	if (!enable_per_layer_report) {
+		mci->ce_noinfo_count++;
 		return;
 	}
 
-	if (channel >= mci->csrows[row].nr_channels || channel < 0) {
-		/* something is wrong */
-		edac_mc_printk(mci, KERN_ERR,
-			"INTERNAL ERROR: channel out of range "
-			"(%d >= %d)\n", channel,
-			mci->csrows[row].nr_channels);
-		edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
+	for (i = 0; i < mci->n_layers; i++) {
+		if (pos[i] < 0)
+			break;
+		index += pos[i];
+		mci->ce_per_layer[i][index]++;
+
+		if (i < mci->n_layers - 1)
+			index *= mci->layers[i + 1].size;
+	}
+}
+
+static void edac_inc_ue_error(struct mem_ctl_info *mci,
+				    bool enable_per_layer_report,
+				    const int pos[EDAC_MAX_LAYERS])
+{
+	int i, index = 0;
+
+	mci->ue_count++;
+
+	if (!enable_per_layer_report) {
+		mci->ce_noinfo_count++;
 		return;
 	}
 
-	label = mci->csrows[row].channels[channel].dimm->label;
-	grain = mci->csrows[row].channels[channel].dimm->grain;
+	for (i = 0; i < mci->n_layers; i++) {
+		if (pos[i] < 0)
+			break;
+		index += pos[i];
+		mci->ue_per_layer[i][index]++;
+
+		if (i < mci->n_layers - 1)
+			index *= mci->layers[i + 1].size;
+	}
+}
+
+static void edac_ce_error(struct mem_ctl_info *mci,
+			  const int pos[EDAC_MAX_LAYERS],
+			  const char *msg,
+			  const char *location,
+			  const char *label,
+			  const char *detail,
+			  const char *other_detail,
+			  const bool enable_per_layer_report,
+			  const unsigned long page_frame_number,
+			  const unsigned long offset_in_page,
+			  u32 grain)
+{
+	unsigned long remapped_page;
 
 	if (edac_mc_get_log_ce())
-		/* FIXME - put in DIMM location */
 		edac_mc_printk(mci, KERN_WARNING,
-			"CE page 0x%lx, offset 0x%lx, grain %d, syndrome "
-			"0x%lx, row %d, channel %d, label \"%s\": %s\n",
-			page_frame_number, offset_in_page,
-			grain, syndrome, row, channel,
-			label, msg);
-
-	mci->ce_count++;
-	mci->csrows[row].ce_count++;
-	mci->csrows[row].channels[channel].dimm->ce_count++;
-	mci->csrows[row].channels[channel].ce_count++;
+				"CE %s on %s (%s%s %s)\n",
+				msg, label, location,
+				detail, other_detail);
+	edac_inc_ce_error(mci, enable_per_layer_report, pos);
 
 	if (mci->scrub_mode & SCRUB_SW_SRC) {
 		/*
-		 * Some MC's can remap memory so that it is still available
-		 * at a different address when PCI devices map into memory.
-		 * MC's that can't do this lose the memory where PCI devices
-		 * are mapped.  This mapping is MC dependent and so we call
-		 * back into the MC driver for it to map the MC page to
-		 * a physical (CPU) page which can then be mapped to a virtual
-		 * page - which can then be scrubbed.
-		 */
+			* Some memory controllers (called MCs below) can remap
+			* memory so that it is still available at a different
+			* address when PCI devices map into memory.
+			* MC's that can't do this, lose the memory where PCI
+			* devices are mapped. This mapping is MC-dependent
+			* and so we call back into the MC driver for it to
+			* map the MC page to a physical (CPU) page which can
+			* then be mapped to a virtual page - which can then
+			* be scrubbed.
+			*/
 		remapped_page = mci->ctl_page_to_phys ?
 			mci->ctl_page_to_phys(mci, page_frame_number) :
 			page_frame_number;
 
-		edac_mc_scrub_block(remapped_page, offset_in_page, grain);
+		edac_mc_scrub_block(remapped_page,
+					offset_in_page, grain);
 	}
 }
-EXPORT_SYMBOL_GPL(edac_mc_handle_ce);
 
-void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci, const char *msg)
+static void edac_ue_error(struct mem_ctl_info *mci,
+			  const int pos[EDAC_MAX_LAYERS],
+			  const char *msg,
+			  const char *location,
+			  const char *label,
+			  const char *detail,
+			  const char *other_detail,
+			  const bool enable_per_layer_report)
 {
-	if (edac_mc_get_log_ce())
+	if (edac_mc_get_log_ue())
 		edac_mc_printk(mci, KERN_WARNING,
-			"CE - no information available: %s\n", msg);
+			"UE %s on %s (%s%s %s)\n",
+			msg, label, location, detail, other_detail);
 
-	mci->ce_noinfo_count++;
-	mci->ce_count++;
+	if (edac_mc_get_panic_on_ue())
+		panic("UE %s on %s (%s%s %s)\n",
+			msg, label, location, detail, other_detail);
+
+	edac_inc_ue_error(mci, enable_per_layer_report, pos);
 }
-EXPORT_SYMBOL_GPL(edac_mc_handle_ce_no_info);
 
-void edac_mc_handle_ue(struct mem_ctl_info *mci,
-		unsigned long page_frame_number,
-		unsigned long offset_in_page, int row, const char *msg)
+#define OTHER_LABEL " or "
+void edac_mc_handle_error(const enum hw_event_mc_err_type type,
+			  struct mem_ctl_info *mci,
+			  const unsigned long page_frame_number,
+			  const unsigned long offset_in_page,
+			  const unsigned long syndrome,
+			  const int layer0,
+			  const int layer1,
+			  const int layer2,
+			  const char *msg,
+			  const char *other_detail,
+			  const void *mcelog)
 {
-	int len = EDAC_MC_LABEL_LEN * 4;
-	char labels[len + 1];
-	char *pos = labels;
-	int chan;
-	int chars;
-	char *label = NULL;
+	/* FIXME: too much for stack: move it to some pre-alocated area */
+	char detail[80], location[80];
+	char label[(EDAC_MC_LABEL_LEN + 1 + sizeof(OTHER_LABEL)) * mci->tot_dimms];
+	char *p;
+	int row = -1, chan = -1;
+	int pos[EDAC_MAX_LAYERS] = { layer0, layer1, layer2 };
+	int i;
 	u32 grain;
+	bool enable_per_layer_report = false;
 
 	debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
 
-	/* FIXME - maybe make panic on INTERNAL ERROR an option */
-	if (row >= mci->nr_csrows || row < 0) {
-		/* something is wrong */
-		edac_mc_printk(mci, KERN_ERR,
-			"INTERNAL ERROR: row out of range "
-			"(%d >= %d)\n", row, mci->nr_csrows);
-		edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
-		return;
-	}
-
-	grain = mci->csrows[row].channels[0].dimm->grain;
-	label = mci->csrows[row].channels[0].dimm->label;
-	chars = snprintf(pos, len + 1, "%s", label);
-	len -= chars;
-	pos += chars;
-
-	for (chan = 1; (chan < mci->csrows[row].nr_channels) && (len > 0);
-		chan++) {
-		label = mci->csrows[row].channels[chan].dimm->label;
-		chars = snprintf(pos, len + 1, ":%s", label);
-		len -= chars;
-		pos += chars;
+	/*
+	 * Check if the event report is consistent and if the memory
+	 * location is known. If it is known, enable_per_layer_report will be
+	 * true, the DIMM(s) label info will be filled and the per-layer
+	 * error counters will be incremented.
+	 */
+	for (i = 0; i < mci->n_layers; i++) {
+		if (pos[i] >= (int)mci->layers[i].size) {
+			if (type == HW_EVENT_ERR_CORRECTED)
+				p = "CE";
+			else
+				p = "UE";
+
+			edac_mc_printk(mci, KERN_ERR,
+				       "INTERNAL ERROR: %s value is out of range (%d >= %d)\n",
+				       edac_layer_name[mci->layers[i].type],
+				       pos[i], mci->layers[i].size);
+			/*
+			 * Instead of just returning it, let's use what's
+			 * known about the error. The increment routines and
+			 * the DIMM filter logic will do the right thing by
+			 * pointing the likely damaged DIMMs.
+			 */
+			pos[i] = -1;
+		}
+		if (pos[i] >= 0)
+			enable_per_layer_report = true;
 	}
 
-	if (edac_mc_get_log_ue())
-		edac_mc_printk(mci, KERN_EMERG,
-			"UE page 0x%lx, offset 0x%lx, grain %d, row %d, "
-			"labels \"%s\": %s\n", page_frame_number,
-			offset_in_page, grain, row, labels, msg);
-
-	if (edac_mc_get_panic_on_ue())
-		panic("EDAC MC%d: UE page 0x%lx, offset 0x%lx, grain %d, "
-			"row %d, labels \"%s\": %s\n", mci->mc_idx,
-			page_frame_number, offset_in_page,
-			grain, row, labels, msg);
-
-	mci->ue_count++;
-	mci->csrows[row].ue_count++;
-}
-EXPORT_SYMBOL_GPL(edac_mc_handle_ue);
-
-void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci, const char *msg)
-{
-	if (edac_mc_get_panic_on_ue())
-		panic("EDAC MC%d: Uncorrected Error", mci->mc_idx);
+	/*
+	 * Get the dimm label/grain that applies to the match criteria.
+	 * As the error algorithm may not be able to point to just one memory
+	 * stick, the logic here will get all possible labels that could
+	 * pottentially be affected by the error.
+	 * On FB-DIMM memory controllers, for uncorrected errors, it is common
+	 * to have only the MC channel and the MC dimm (also called "branch")
+	 * but the channel is not known, as the memory is arranged in pairs,
+	 * where each memory belongs to a separate channel within the same
+	 * branch.
+	 */
+	grain = 0;
+	p = label;
+	*p = '\0';
+	for (i = 0; i < mci->tot_dimms; i++) {
+		struct dimm_info *dimm = &mci->dimms[i];
 
-	if (edac_mc_get_log_ue())
-		edac_mc_printk(mci, KERN_WARNING,
-			"UE - no information available: %s\n", msg);
-	mci->ue_noinfo_count++;
-	mci->ue_count++;
-}
-EXPORT_SYMBOL_GPL(edac_mc_handle_ue_no_info);
+		if (layer0 >= 0 && layer0 != dimm->location[0])
+			continue;
+		if (layer1 >= 0 && layer1 != dimm->location[1])
+			continue;
+		if (layer2 >= 0 && layer2 != dimm->location[2])
+			continue;
 
-/*************************************************************
- * On Fully Buffered DIMM modules, this help function is
- * called to process UE events
- */
-void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
-			unsigned int csrow,
-			unsigned int channela,
-			unsigned int channelb, char *msg)
-{
-	int len = EDAC_MC_LABEL_LEN * 4;
-	char labels[len + 1];
-	char *pos = labels;
-	int chars;
-	char *label;
-
-	if (csrow >= mci->nr_csrows) {
-		/* something is wrong */
-		edac_mc_printk(mci, KERN_ERR,
-			"INTERNAL ERROR: row out of range (%d >= %d)\n",
-			csrow, mci->nr_csrows);
-		edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
-		return;
-	}
+		/* get the max grain, over the error match range */
+		if (dimm->grain > grain)
+			grain = dimm->grain;
 
-	if (channela >= mci->csrows[csrow].nr_channels) {
-		/* something is wrong */
-		edac_mc_printk(mci, KERN_ERR,
-			"INTERNAL ERROR: channel-a out of range "
-			"(%d >= %d)\n",
-			channela, mci->csrows[csrow].nr_channels);
-		edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
-		return;
+		/*
+		 * If the error is memory-controller wide, there's no need to
+		 * seek for the affected DIMMs because the whole
+		 * channel/memory controller/...  may be affected.
+		 * Also, don't show errors for empty DIMM slots.
+		 */
+		if (enable_per_layer_report && dimm->nr_pages) {
+			if (p != label) {
+				strcpy(p, OTHER_LABEL);
+				p += strlen(OTHER_LABEL);
+			}
+			strcpy(p, dimm->label);
+			p += strlen(p);
+			*p = '\0';
+
+			/*
+			 * get csrow/channel of the DIMM, in order to allow
+			 * incrementing the compat API counters
+			 */
+			debugf4("%s: %s csrows map: (%d,%d)\n",
+				__func__,
+				mci->mem_is_per_rank ? "rank" : "dimm",
+				dimm->csrow, dimm->cschannel);
+
+			if (row == -1)
+				row = dimm->csrow;
+			else if (row >= 0 && row != dimm->csrow)
+				row = -2;
+
+			if (chan == -1)
+				chan = dimm->cschannel;
+			else if (chan >= 0 && chan != dimm->cschannel)
+				chan = -2;
+		}
 	}
 
-	if (channelb >= mci->csrows[csrow].nr_channels) {
-		/* something is wrong */
-		edac_mc_printk(mci, KERN_ERR,
-			"INTERNAL ERROR: channel-b out of range "
-			"(%d >= %d)\n",
-			channelb, mci->csrows[csrow].nr_channels);
-		edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
-		return;
+	if (!enable_per_layer_report) {
+		strcpy(label, "any memory");
+	} else {
+		debugf4("%s: csrow/channel to increment: (%d,%d)\n",
+			__func__, row, chan);
+		if (p == label)
+			strcpy(label, "unknown memory");
+		if (type == HW_EVENT_ERR_CORRECTED) {
+			if (row >= 0) {
+				mci->csrows[row].ce_count++;
+				if (chan >= 0)
+					mci->csrows[row].channels[chan].ce_count++;
+			}
+		} else
+			if (row >= 0)
+				mci->csrows[row].ue_count++;
 	}
 
-	mci->ue_count++;
-	mci->csrows[csrow].ue_count++;
-
-	/* Generate the DIMM labels from the specified channels */
-	label = mci->csrows[csrow].channels[channela].dimm->label;
-	chars = snprintf(pos, len + 1, "%s", label);
-	len -= chars;
-	pos += chars;
-
-	chars = snprintf(pos, len + 1, "-%s",
-			mci->csrows[csrow].channels[channelb].dimm->label);
-
-	if (edac_mc_get_log_ue())
-		edac_mc_printk(mci, KERN_EMERG,
-			"UE row %d, channel-a= %d channel-b= %d "
-			"labels \"%s\": %s\n", csrow, channela, channelb,
-			labels, msg);
-
-	if (edac_mc_get_panic_on_ue())
-		panic("UE row %d, channel-a= %d channel-b= %d "
-			"labels \"%s\": %s\n", csrow, channela,
-			channelb, labels, msg);
-}
-EXPORT_SYMBOL(edac_mc_handle_fbd_ue);
-
-/*************************************************************
- * On Fully Buffered DIMM modules, this help function is
- * called to process CE events
- */
-void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
-			unsigned int csrow, unsigned int channel, char *msg)
-{
-	char *label = NULL;
+	/* Fill the RAM location data */
+	p = location;
+	for (i = 0; i < mci->n_layers; i++) {
+		if (pos[i] < 0)
+			continue;
 
-	/* Ensure boundary values */
-	if (csrow >= mci->nr_csrows) {
-		/* something is wrong */
-		edac_mc_printk(mci, KERN_ERR,
-			"INTERNAL ERROR: row out of range (%d >= %d)\n",
-			csrow, mci->nr_csrows);
-		edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
-		return;
-	}
-	if (channel >= mci->csrows[csrow].nr_channels) {
-		/* something is wrong */
-		edac_mc_printk(mci, KERN_ERR,
-			"INTERNAL ERROR: channel out of range (%d >= %d)\n",
-			channel, mci->csrows[csrow].nr_channels);
-		edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
-		return;
+		p += sprintf(p, "%s %d ",
+			     edac_layer_name[mci->layers[i].type],
+			     pos[i]);
 	}
 
-	label = mci->csrows[csrow].channels[channel].dimm->label;
+	/* Memory type dependent details about the error */
+	if (type == HW_EVENT_ERR_CORRECTED) {
+		snprintf(detail, sizeof(detail),
+			"page 0x%lx offset 0x%lx grain %d syndrome 0x%lx",
+			page_frame_number, offset_in_page,
+			grain, syndrome);
 
-	if (edac_mc_get_log_ce())
-		/* FIXME - put in DIMM location */
-		edac_mc_printk(mci, KERN_WARNING,
-			"CE row %d, channel %d, label \"%s\": %s\n",
-			csrow, channel, label, msg);
+		edac_ce_error(mci, pos, msg, location, label, detail,
+			      other_detail, enable_per_layer_report,
+			      page_frame_number, offset_in_page, grain);
+	} else {
+		snprintf(detail, sizeof(detail),
+			"page 0x%lx offset 0x%lx grain %d",
+			page_frame_number, offset_in_page, grain);
 
-	mci->ce_count++;
-	mci->csrows[csrow].ce_count++;
-	mci->csrows[csrow].channels[channel].dimm->ce_count++;
-	mci->csrows[csrow].channels[channel].ce_count++;
+		edac_ue_error(mci, pos, msg, location, label, detail,
+			      other_detail, enable_per_layer_report);
+	}
 }
-EXPORT_SYMBOL(edac_mc_handle_fbd_ce);
+EXPORT_SYMBOL_GPL(edac_mc_handle_error);
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 3b8798d..c8f507d 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -412,18 +412,20 @@ struct edac_mc_layer {
 /* FIXME: add the proper per-location error counts */
 struct dimm_info {
 	char label[EDAC_MC_LABEL_LEN + 1];	/* DIMM label on motherboard */
-	unsigned memory_controller;
-	unsigned csrow;
-	unsigned csrow_channel;
+
+	/* Memory location data */
+	unsigned location[EDAC_MAX_LAYERS];
+
+	struct mem_ctl_info *mci;	/* the parent */
 
 	u32 grain;		/* granularity of reported error in bytes */
 	enum dev_type dtype;	/* memory device type */
 	enum mem_type mtype;	/* memory dimm type */
 	enum edac_type edac_mode;	/* EDAC mode for this dimm */
 
-	u32 nr_pages;			/* number of pages in csrow */
+	u32 nr_pages;			/* number of pages on this dimm */
 
-	u32 ce_count;		/* Correctable Errors for this dimm */
+	unsigned csrow, cschannel;	/* Points to the old API data */
 };
 
 /**
@@ -443,9 +445,10 @@ struct dimm_info {
  */
 struct rank_info {
 	int chan_idx;
-	u32 ce_count;
 	struct csrow_info *csrow;
 	struct dimm_info *dimm;
+
+	u32 ce_count;		/* Correctable Errors for this csrow */
 };
 
 struct csrow_info {
@@ -541,13 +544,18 @@ struct mem_ctl_info {
 	unsigned long (*ctl_page_to_phys) (struct mem_ctl_info * mci,
 					   unsigned long page);
 	int mc_idx;
-	int nr_csrows;
 	struct csrow_info *csrows;
+	unsigned nr_csrows, num_cschannel;
+
+	/* Memory Controller hierarchy */
+	unsigned n_layers;
+	struct edac_mc_layer *layers;
+	bool mem_is_per_rank;
 
 	/*
 	 * DIMM info. Will eventually remove the entire csrows_info some day
 	 */
-	unsigned nr_dimms;
+	unsigned tot_dimms;
 	struct dimm_info *dimms;
 
 	/*
@@ -562,12 +570,16 @@ struct mem_ctl_info {
 	const char *dev_name;
 	char proc_name[MC_PROC_NAME_MAX_LEN + 1];
 	void *pvt_info;
-	u32 ue_noinfo_count;	/* Uncorrectable Errors w/o info */
-	u32 ce_noinfo_count;	/* Correctable Errors w/o info */
-	u32 ue_count;		/* Total Uncorrectable Errors for this MC */
-	u32 ce_count;		/* Total Correctable Errors for this MC */
 	unsigned long start_time;	/* mci load start time (in jiffies) */
 
+	/*
+	 * drivers shouldn't access those fields directly, as the core
+	 * already handles that.
+	 */
+	u32 ce_noinfo_count, ue_noinfo_count;
+	u32 ue_count, ce_count;
+	u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
+
 	struct completion complete;
 
 	/* edac sysfs device control */
@@ -580,7 +592,7 @@ struct mem_ctl_info {
 	 * by the low level driver.
 	 *
 	 * Set by the low level driver to provide attributes at the
-	 * controller level, same level as 'ue_count' and 'ce_count' above.
+	 * controller level.
 	 * An array of structures, NULL terminated
 	 *
 	 * If attributes are desired, then set to array of attributes

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Hans Verkuil]

On Thursday 03 May 2012 15:42:46 Mauro Carvalho Chehab wrote:
> Em 03-05-2012 04:02, Hans Verkuil escreveu:
> > On Wed May 2 2012 23:39:15 Sakari Ailus wrote:
> >> Hi Hans,
> >>
> >> On Wed, May 02, 2012 at 10:45:22PM +0200, Hans Verkuil wrote:
> >>> On Wed May 2 2012 21:13:47 Sakari Ailus wrote:
> >>>> Replace enums in IOCTL structs by __u32. The size of enums is variable and
> >>>> thus problematic. Compatibility structs having exactly the same as original
> >>>> definition are provided for compatibility with old binaries with the
> >>>> required conversion code.
> >>>
> >>> Does someone actually have hard proof that there really is a problem? You know,
> >>> demonstrate it with actual example code?
> >>>
> >>> It's pretty horrible that you have to do all those conversions and that code
> >>> will be with us for years to come.
> >>>
> >>> For most (if not all!) architectures sizeof(enum) == sizeof(u32), so there is
> >>> no need for any compat code for those.
> >>
> >> Cases I know where this can go wrong are, but there may well be others:
> >>
> >> - ppc64: int is 64 bits there, and thus also enums,
> > 
> > Are you really, really certain that's the case? If I look at
> > arch/powerpc/include/asm/types.h it includes either asm-generic/int-l64.h
> > or asm-generic/int-ll64.h and both of those headers define u32 as unsigned int.
> > Also, if sizeof(int) != 4, then how would you define u32?
> > 
> > Ask a ppc64 kernel maintainer what sizeof(int) and sizeof(enum) are in the kernel
> > before we start doing lots of work for no reason.
> > 
> > Looking at arch/*/include/asm/types.h it seems all architectures define sizeof(int)
> > as 4.
> > 
> > What sizeof(long) is will actually differ between architectures, but char, short
> > and int seem to be fixed everywhere.
> 
> Yes, it seems that, inside the Kernel, "int" it will always be 32 bits. It doesn't
> necessarily means that "enum" will be 32 bits.

Actually, I believe it is. It is my understanding that --short-enums is not allowed
inside the kernel and so enums are the same size as int. But I'd like to have some
confirmation about that first. That compiler option isn't used anywhere in the kernel
in any case and gcc on ARM will default to int-sized enums on linux.

So just changing all the enums in videodev2.h to u32 should almost certainly be all
we need to do.

> Also, as it is recommended to not use "int/unsigned int/long/unsigned long" at 
> kernel<->userspace API, I bet that this will cause problems on userspace (maybe
> with non-gcc compilers?)

'long' is certainly known to change size depending on the compiler. 'int' can be
two bytes on *really* old hardware/compilers.

struct v4l2_jpegcompression is the only place where int is still used. I wouldn't
mind if that changes to u32 at the same time (those ints should have been unsigned
anyway).

Regards,

	Hans

Re: [RFC v3 1/2] v4l: Do not use enums in IOCTL structs

[Andy Walls]

On Thu, 2012-05-03 at 07:22 -0300, Mauro Carvalho Chehab wrote:
> Em 02-05-2012 21:42, Andy Walls escreveu:
> > On Wed, 2012-05-02 at 19:17 -0300, Mauro Carvalho Chehab wrote:

> >> I suspect that sizeof() won't work inside a macro. 
> > 
> > sizeof() is evaluated at compile time, after preprocessing. 
> > It should work inside of a macro.
> 
> I tried to compile this small piece of code:
> 
> enum foo { BAR };
> #if sizeof(foo) != sizeof(int)
> void main(void) { printf("different sizes\n"); }
> #else
> void main(void) { printf("same size\n"); }
> #endif

Oh, I misunderstood what you intended when you said "work inside a
macro".

You are correct.  I would not expect sizeof() to work in a condition
evaluated by the preprocessor.  Only the compiler can properly compute
sizeof(), after the preprocessor has done it's work -- unless someone
builds a much smarter, multiple-pass preprocessor.



> > See the ARRAY_SIZE() macro in include/linux/kernel.h for a well tested
> > example.
> 
> ARRAY_SIZE() doesn't have an #if on it.

Correct.  My example is irrelevant, because I didn't understand what you
meant.

Sorry for the noise.

Regards,
Andy