[PATCH 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[PATCH 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Mauro Carvalho Chehab]

In the past, only string controls were pointers. That changed when compounded
types got added, but the compat32 code was not updated.

We could just add those controls there, but maintaining it is flaw, as we
often forget about the compat code. So, instead, rely on the control type,
as this is always updated when new controls are added.

As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
move the ctrl_is_pointer() helper function to v4l2-ctrl.c.

Mauro Carvalho Chehab (3):
  media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill
  media: v4l2-ctrls: prepare the function to be used by compat32 code
  media: compat32: reimplement ctrl_is_pointer()

 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +---------
 drivers/media/v4l2-core/v4l2-ctrls.c          | 49 +++++++++++++++++++++++++--
 include/media/v4l2-ctrls.h                    | 28 ++++++++++-----
 3 files changed, 67 insertions(+), 28 deletions(-)

-- 
2.13.3

[PATCH 1/3] media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill

[Mauro Carvalho Chehab]

The arguments for this function are pointers. Make it clear at
its documentation.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 include/media/v4l2-ctrls.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 2d2aed56922f..6ba30acf06aa 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -339,18 +339,18 @@ struct v4l2_ctrl_config {
 /**
  * v4l2_ctrl_fill - Fill in the control fields based on the control ID.
  *
- * @id: ID of the control
- * @name: name of the control
- * @type: type of the control
- * @min: minimum value for the control
- * @max: maximum value for the control
- * @step: control step
- * @def: default value for the control
- * @flags: flags to be used on the control
+ * @id: pointer for storing the ID of the control
+ * @name: pointer for storing the name of the control
+ * @type: pointer for storing the type of the control
+ * @min: pointer for storing the minimum value for the control
+ * @max: pointer for storing the maximum value for the control
+ * @step: pointer for storing the control step
+ * @def: pointer for storing the default value for the control
+ * @flags: pointer for storing the flags to be used on the control
  *
  * This works for all standard V4L2 controls.
  * For non-standard controls it will only fill in the given arguments
- * and @name will be %NULL.
+ * and @name content will be filled with %NULL.
  *
  * This function will overwrite the contents of @name, @type and @flags.
  * The contents of @min, @max, @step and @def may be modified depending on
-- 
2.13.3

[PATCH 2/3] media: v4l2-ctrls: prepare the function to be used by compat32 code

[Mauro Carvalho Chehab]

Right now, both v4l2_ctrl_fill() and compat32 code need to know
the type of the control. As new controls are added, this cause
troubles at compat32, as it won't be able to discover what
functions are pointers or not.

Change v4l2_ctrl_fill() function for it to be called with just
one argument: the control type. This way, the compat32 code can
use it internally.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 31 +++++++++++++++++++++++++++++--
 include/media/v4l2-ctrls.h           |  2 ++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index dd1db678718c..c512b7539077 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -939,8 +939,10 @@ EXPORT_SYMBOL(v4l2_ctrl_get_name);
 void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		    s64 *min, s64 *max, u64 *step, s64 *def, u32 *flags)
 {
-	*name = v4l2_ctrl_get_name(id);
-	*flags = 0;
+	if (name) {
+		*name = v4l2_ctrl_get_name(id);
+		*flags = 0;
+	}
 
 	switch (id) {
 	case V4L2_CID_AUDIO_MUTE:
@@ -996,11 +998,15 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_RDS_RX_TRAFFIC_PROGRAM:
 	case V4L2_CID_RDS_RX_MUSIC_SPEECH:
 		*type = V4L2_CTRL_TYPE_BOOLEAN;
+		if (!name)
+			break;
 		*min = 0;
 		*max = *step = 1;
 		break;
 	case V4L2_CID_ROTATE:
 		*type = V4L2_CTRL_TYPE_INTEGER;
+		if (!name)
+			break;
 		*flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 		break;
 	case V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE:
@@ -1015,6 +1021,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_AUTO_FOCUS_START:
 	case V4L2_CID_AUTO_FOCUS_STOP:
 		*type = V4L2_CTRL_TYPE_BUTTON;
+		if (!name)
+			break;
 		*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
 			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
 		*min = *max = *step = *def = 0;
@@ -1099,12 +1107,16 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_RF_TUNER_CLASS:
 	case V4L2_CID_DETECT_CLASS:
 		*type = V4L2_CTRL_TYPE_CTRL_CLASS;
+		if (!name)
+			break;
 		/* You can neither read not write these */
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;
 		*min = *max = *step = *def = 0;
 		break;
 	case V4L2_CID_BG_COLOR:
 		*type = V4L2_CTRL_TYPE_INTEGER;
+		if (!name)
+			break;
 		*step = 1;
 		*min = 0;
 		/* Max is calculated as RGB888 that is 2^24 */
@@ -1123,10 +1135,14 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
 		*type = V4L2_CTRL_TYPE_INTEGER;
+		if (!name)
+			break;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
 	case V4L2_CID_MPEG_VIDEO_DEC_PTS:
 		*type = V4L2_CTRL_TYPE_INTEGER64;
+		if (!name)
+			break;
 		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
 		*min = *def = 0;
 		*max = 0x1ffffffffLL;
@@ -1134,6 +1150,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_MPEG_VIDEO_DEC_FRAME:
 		*type = V4L2_CTRL_TYPE_INTEGER64;
+		if (!name)
+			break;
+
 		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
 		*min = *def = 0;
 		*max = 0x7fffffffffffffffLL;
@@ -1141,6 +1160,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_PIXEL_RATE:
 		*type = V4L2_CTRL_TYPE_INTEGER64;
+		if (!name)
+			break;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
 	case V4L2_CID_DETECT_MD_REGION_GRID:
@@ -1156,6 +1177,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;
 	}
+
+	if (!name)
+		return;
+
+	/* Update flags for some controls */
+
 	switch (id) {
 	case V4L2_CID_MPEG_AUDIO_ENCODING:
 	case V4L2_CID_MPEG_AUDIO_MODE:
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 6ba30acf06aa..e22dea218a4c 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -352,6 +352,8 @@ struct v4l2_ctrl_config {
  * For non-standard controls it will only fill in the given arguments
  * and @name content will be filled with %NULL.
  *
+ * if @name is NULL, only the @type will be filled.
+ *
  * This function will overwrite the contents of @name, @type and @flags.
  * The contents of @min, @max, @step and @def may be modified depending on
  * the type.
-- 
2.13.3

[PATCH 3/3] media: compat32: reimplement ctrl_is_pointer()

[Mauro Carvalho Chehab]

The current way that this function works is subject to problems
as new controls gets added. Move it to v4l2-ctrls and use the
knowledge that v4l2_ctrl_fill() has about controls, in order to
detect if a given control is a pointer.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +-----------------
 drivers/media/v4l2-core/v4l2-ctrls.c          | 18 ++++++++++++++++++
 include/media/v4l2-ctrls.h                    |  8 ++++++++
 3 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 6f52970f8b54..1105e04dec3d 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -19,6 +19,7 @@
 #include <linux/v4l2-subdev.h>
 #include <media/v4l2-dev.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-ctrls.h>
 
 static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
@@ -665,23 +666,6 @@ struct v4l2_ext_control32 {
 	};
 } __attribute__ ((packed));
 
-/* The following function really belong in v4l2-common, but that causes
-   a circular dependency between modules. We need to think about this, but
-   for now this will do. */
-
-/* Return non-zero if this control is a pointer type. Currently only
-   type STRING is a pointer type. */
-static inline int ctrl_is_pointer(u32 id)
-{
-	switch (id) {
-	case V4L2_CID_RDS_TX_PS_NAME:
-	case V4L2_CID_RDS_TX_RADIO_TEXT:
-		return 1;
-	default:
-		return 0;
-	}
-}
-
 static int get_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext_controls32 __user *up)
 {
 	struct v4l2_ext_control32 __user *ucontrols;
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index c512b7539077..0d5dab485723 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1254,6 +1254,24 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 }
 EXPORT_SYMBOL(v4l2_ctrl_fill);
 
+bool ctrl_is_pointer(u32 id)
+{
+	enum v4l2_ctrl_type type;
+
+	v4l2_ctrl_fill(id, NULL, &type, NULL, NULL, NULL, NULL, NULL);
+
+	switch (type) {
+	case V4L2_CTRL_TYPE_STRING:
+	case V4L2_CTRL_TYPE_U8:
+	case V4L2_CTRL_TYPE_U16:
+	case V4L2_CTRL_TYPE_U32:
+		return true;
+	default:
+		return false;
+	}
+}
+EXPORT_SYMBOL(ctrl_is_pointer);
+
 static void fill_event(struct v4l2_event *ev, struct v4l2_ctrl *ctrl, u32 changes)
 {
 	memset(ev->reserved, 0, sizeof(ev->reserved));
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index e22dea218a4c..bc6772f50956 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -367,6 +367,14 @@ struct v4l2_ctrl_config {
 void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		    s64 *min, s64 *max, u64 *step, s64 *def, u32 *flags);
 
+/**
+ * ctrl_is_pointer - Returns non-zero if this control is a pointer type.
+ *
+ * @id: ID of the control
+ *
+ * Currently only STRING and compound types are pointers.
+ */
+bool ctrl_is_pointer(u32 id);
 
 /**
  * v4l2_ctrl_handler_init_class() - Initialize the control handler.
-- 
2.13.3

Re: [PATCH 0/3] v4l2-compat-ioctl32.c: better detect pointer controls

[Mauro Carvalho Chehab]

Em Thu, 10 Aug 2017 20:45:10 -0300
Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:

> In the past, only string controls were pointers. That changed when compounded
> types got added, but the compat32 code was not updated.
> 
> We could just add those controls there, but maintaining it is flaw, as we
> often forget about the compat code. So, instead, rely on the control type,
> as this is always updated when new controls are added.
> 
> As both v4l2-ctrl and compat32 code are at videodev.ko module, we can
> move the ctrl_is_pointer() helper function to v4l2-ctrl.c.
> 
> Mauro Carvalho Chehab (3):
>   media: v4l2-ctrls.h: better document the arguments for v4l2_ctrl_fill
>   media: v4l2-ctrls: prepare the function to be used by compat32 code
>   media: compat32: reimplement ctrl_is_pointer()
> 
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 18 +---------
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 49 +++++++++++++++++++++++++--
>  include/media/v4l2-ctrls.h                    | 28 ++++++++++-----
>  3 files changed, 67 insertions(+), 28 deletions(-)
> 

Sorry, sent this series to the wrong mailing list. Will resend
to the right one.


Thanks,
Mauro