[PATCH v2 0/6] media: video-i2c: support changing frame interval and runtime PM

[PATCH v2 0/6] media: video-i2c: support changing frame interval and runtime PM

[Akinobu Mita]

This patchset adds support for changing frame interval and runtime PM for
video-i2c driver.  This also adds an helper macro to v4l2 common
internal API that is used to to find a suitable frame interval.

There are a couple of unrelated changes that are included for simplifying
driver initialization code and register accesses.

* v2
- Add Acked-by and Reviewed-by tags
- Update commit log to clarify the use after free
- Add thermistor and termperature register address definisions
- Stop adding v4l2_find_closest_fract() in v4l2 common internal API
- Add V4L2_FRACT_COMPARE() macro in v4l2 common internal API
- Use V4L2_FRACT_COMPARE() to find suitable frame interval instead of
  v4l2_find_closest_fract()
- Add comment for register address definisions

Akinobu Mita (6):
  media: video-i2c: avoid accessing released memory area when removing
    driver
  media: video-i2c: use i2c regmap
  media: v4l2-common: add V4L2_FRACT_COMPARE
  media: vivid: use V4L2_FRACT_COMPARE
  media: video-i2c: support changing frame interval
  media: video-i2c: support runtime PM

 drivers/media/i2c/video-i2c.c                | 286 ++++++++++++++++++++++-----
 drivers/media/platform/vivid/vivid-vid-cap.c |   9 +-
 include/media/v4l2-common.h                  |   5 +
 3 files changed, 247 insertions(+), 53 deletions(-)

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
-- 
2.7.4

[PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Akinobu Mita]

The video_i2c_data is allocated by kzalloc and released by the video
device's release callback.  The release callback is called when
video_unregister_device() is called, but it will still be accessed after
calling video_unregister_device().

Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
i2c_client->dev so that it will automatically be released when the i2c
driver is removed.

Fixes: 5cebaac60974 ("media: video-i2c: add video-i2c driver")
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- Update commit log to clarify the use after free
- Add Acked-by tag

 drivers/media/i2c/video-i2c.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 06d29d8..b7a2af9 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -508,20 +508,15 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
 	.vidioc_streamoff		= vb2_ioctl_streamoff,
 };
 
-static void video_i2c_release(struct video_device *vdev)
-{
-	kfree(video_get_drvdata(vdev));
-}
-
 static int video_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
 	struct video_i2c_data *data;
 	struct v4l2_device *v4l2_dev;
 	struct vb2_queue *queue;
-	int ret = -ENODEV;
+	int ret;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -530,7 +525,7 @@ static int video_i2c_probe(struct i2c_client *client,
 	else if (id)
 		data->chip = &video_i2c_chip[id->driver_data];
 	else
-		goto error_free_device;
+		return -ENODEV;
 
 	data->client = client;
 	v4l2_dev = &data->v4l2_dev;
@@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
 
 	ret = v4l2_device_register(&client->dev, v4l2_dev);
 	if (ret < 0)
-		goto error_free_device;
+		return ret;
 
 	mutex_init(&data->lock);
 	mutex_init(&data->queue_lock);
@@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
 	data->vdev.fops = &video_i2c_fops;
 	data->vdev.lock = &data->lock;
 	data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
-	data->vdev.release = video_i2c_release;
+	data->vdev.release = video_device_release_empty;
 	data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
 				 V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
 
@@ -597,9 +592,6 @@ static int video_i2c_probe(struct i2c_client *client,
 	mutex_destroy(&data->lock);
 	mutex_destroy(&data->queue_lock);
 
-error_free_device:
-	kfree(data);
-
 	return ret;
 }
 
-- 
2.7.4

[PATCH v2 2/6] media: video-i2c: use i2c regmap

[Akinobu Mita]

Use regmap for i2c register access.  This simplifies register accesses and
chooses suitable access commands based on the functionality that the
adapter supports.

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- Add thermistor and termperature register address definisions

 drivers/media/i2c/video-i2c.c | 60 ++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index b7a2af9..fb8509e 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
+#include <linux/regmap.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
@@ -38,7 +39,7 @@ struct video_i2c_buffer {
 };
 
 struct video_i2c_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 	const struct video_i2c_chip *chip;
 	struct mutex lock;
 	spinlock_t slock;
@@ -62,6 +63,12 @@ static const struct v4l2_frmsize_discrete amg88xx_size = {
 	.height = 8,
 };
 
+static const struct regmap_config amg88xx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xff
+};
+
 struct video_i2c_chip {
 	/* video dimensions */
 	const struct v4l2_fmtdesc *format;
@@ -76,6 +83,8 @@ struct video_i2c_chip {
 	/* pixel size in bits */
 	unsigned int bpp;
 
+	const struct regmap_config *regmap_config;
+
 	/* xfer function */
 	int (*xfer)(struct video_i2c_data *data, char *buf);
 
@@ -83,26 +92,16 @@ struct video_i2c_chip {
 	int (*hwmon_init)(struct video_i2c_data *data);
 };
 
-static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
-{
-	struct i2c_client *client = data->client;
-	struct i2c_msg msg[2];
-	u8 reg = 0x80;
-	int ret;
-
-	msg[0].addr = client->addr;
-	msg[0].flags = 0;
-	msg[0].len = 1;
-	msg[0].buf  = (char *)&reg;
-
-	msg[1].addr = client->addr;
-	msg[1].flags = I2C_M_RD;
-	msg[1].len = data->chip->buffer_size;
-	msg[1].buf = (char *)buf;
+/* Thermistor register */
+#define AMG88XX_REG_TTHL	0x0e
 
-	ret = i2c_transfer(client->adapter, msg, 2);
+/* Temperature register */
+#define AMG88XX_REG_T01L	0x80
 
-	return (ret == 2) ? 0 : -EIO;
+static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
+{
+	return regmap_bulk_read(data->regmap, AMG88XX_REG_T01L, buf,
+				data->chip->buffer_size);
 }
 
 #if IS_ENABLED(CONFIG_HWMON)
@@ -133,12 +132,15 @@ static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type,
 			u32 attr, int channel, long *val)
 {
 	struct video_i2c_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int tmp = i2c_smbus_read_word_data(client, 0x0e);
+	__le16 buf;
+	int tmp;
 
-	if (tmp < 0)
+	tmp = regmap_bulk_read(data->regmap, AMG88XX_REG_TTHL, &buf, 2);
+	if (tmp)
 		return tmp;
 
+	tmp = le16_to_cpu(buf);
+
 	/*
 	 * Check for sign bit, this isn't a two's complement value but an
 	 * absolute temperature that needs to be inverted in the case of being
@@ -164,8 +166,9 @@ static const struct hwmon_chip_info amg88xx_chip_info = {
 
 static int amg88xx_hwmon_init(struct video_i2c_data *data)
 {
-	void *hwmon = devm_hwmon_device_register_with_info(&data->client->dev,
-				"amg88xx", data, &amg88xx_chip_info, NULL);
+	struct device *dev = regmap_get_device(data->regmap);
+	void *hwmon = devm_hwmon_device_register_with_info(dev, "amg88xx", data,
+						&amg88xx_chip_info, NULL);
 
 	return PTR_ERR_OR_ZERO(hwmon);
 }
@@ -182,6 +185,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
 		.max_fps	= 10,
 		.buffer_size	= 128,
 		.bpp		= 16,
+		.regmap_config	= &amg88xx_regmap_config,
 		.xfer		= &amg88xx_xfer,
 		.hwmon_init	= amg88xx_hwmon_init,
 	},
@@ -350,7 +354,8 @@ static int video_i2c_querycap(struct file *file, void  *priv,
 				struct v4l2_capability *vcap)
 {
 	struct video_i2c_data *data = video_drvdata(file);
-	struct i2c_client *client = data->client;
+	struct device *dev = regmap_get_device(data->regmap);
+	struct i2c_client *client = to_i2c_client(dev);
 
 	strlcpy(vcap->driver, data->v4l2_dev.name, sizeof(vcap->driver));
 	strlcpy(vcap->card, data->vdev.name, sizeof(vcap->card));
@@ -527,7 +532,10 @@ static int video_i2c_probe(struct i2c_client *client,
 	else
 		return -ENODEV;
 
-	data->client = client;
+	data->regmap = devm_regmap_init_i2c(client, data->chip->regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
 	v4l2_dev = &data->v4l2_dev;
 	strlcpy(v4l2_dev->name, VIDEO_I2C_DRIVER, sizeof(v4l2_dev->name));
 
-- 
2.7.4

[PATCH v2 3/6] media: v4l2-common: add V4L2_FRACT_COMPARE

[Akinobu Mita]

Add macro to compare two v4l2_fract values in v4l2 common internal API.
The same macro FRACT_CMP() is used by vivid and bcm2835-camera.  This just
renames it to V4L2_FRACT_COMPARE in order to avoid namespace collision.

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- New patch

 include/media/v4l2-common.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index cdc87ec..eafb8a3 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -384,4 +384,9 @@ int v4l2_g_parm_cap(struct video_device *vdev,
 int v4l2_s_parm_cap(struct video_device *vdev,
 		    struct v4l2_subdev *sd, struct v4l2_streamparm *a);
 
+/* Compare two v4l2_fract structs */
+#define V4L2_FRACT_COMPARE(a, OP, b)			\
+	((u64)(a).numerator * (b).denominator OP	\
+	(u64)(b).numerator * (a).denominator)
+
 #endif /* V4L2_COMMON_H_ */
-- 
2.7.4

[PATCH v2 4/6] media: vivid: use V4L2_FRACT_COMPARE

[Akinobu Mita]

Now the equivalent of FRACT_CMP() is added in v4l2 common internal API
header.

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- New patch

 drivers/media/platform/vivid/vivid-vid-cap.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c b/drivers/media/platform/vivid/vivid-vid-cap.c
index 1599159..f19c701 100644
--- a/drivers/media/platform/vivid/vivid-vid-cap.c
+++ b/drivers/media/platform/vivid/vivid-vid-cap.c
@@ -1824,9 +1824,6 @@ int vivid_vid_cap_g_parm(struct file *file, void *priv,
 	return 0;
 }
 
-#define FRACT_CMP(a, OP, b)	\
-	((u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator)
-
 int vivid_vid_cap_s_parm(struct file *file, void *priv,
 			  struct v4l2_streamparm *parm)
 {
@@ -1847,14 +1844,14 @@ int vivid_vid_cap_s_parm(struct file *file, void *priv,
 	if (tpf.denominator == 0)
 		tpf = webcam_intervals[ival_sz - 1];
 	for (i = 0; i < ival_sz; i++)
-		if (FRACT_CMP(tpf, >=, webcam_intervals[i]))
+		if (V4L2_FRACT_COMPARE(tpf, >=, webcam_intervals[i]))
 			break;
 	if (i == ival_sz)
 		i = ival_sz - 1;
 	dev->webcam_ival_idx = i;
 	tpf = webcam_intervals[dev->webcam_ival_idx];
-	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
-	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
+	tpf = V4L2_FRACT_COMPARE(tpf, <, tpf_min) ? tpf_min : tpf;
+	tpf = V4L2_FRACT_COMPARE(tpf, >, tpf_max) ? tpf_max : tpf;
 
 	/* resync the thread's timings */
 	dev->cap_seq_resync = true;
-- 
2.7.4

[PATCH v2 5/6] media: video-i2c: support changing frame interval

[Akinobu Mita]

AMG88xx has a register for setting frame rate 1 or 10 FPS.
This adds support changing frame interval.

Reference specifications:
https://docid81hrs3j1.cloudfront.net/medialibrary/2017/11/PANA-S-A0002141979-1.pdf

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- Add Acked-by tag
- Add comment for frame rate register address definision
- Use V4L2_FRACT_COMPARE() to find suitable frame interval

 drivers/media/i2c/video-i2c.c | 78 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index fb8509e..6dd1929 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -52,6 +52,8 @@ struct video_i2c_data {
 
 	struct task_struct *kthread_vid_cap;
 	struct list_head vid_cap_active;
+
+	struct v4l2_fract frame_interval;
 };
 
 static const struct v4l2_fmtdesc amg88xx_format = {
@@ -74,8 +76,9 @@ struct video_i2c_chip {
 	const struct v4l2_fmtdesc *format;
 	const struct v4l2_frmsize_discrete *size;
 
-	/* max frames per second */
-	unsigned int max_fps;
+	/* available frame intervals */
+	const struct v4l2_fract *frame_intervals;
+	unsigned int num_frame_intervals;
 
 	/* pixel buffer size */
 	unsigned int buffer_size;
@@ -85,6 +88,9 @@ struct video_i2c_chip {
 
 	const struct regmap_config *regmap_config;
 
+	/* setup function */
+	int (*setup)(struct video_i2c_data *data);
+
 	/* xfer function */
 	int (*xfer)(struct video_i2c_data *data, char *buf);
 
@@ -92,6 +98,10 @@ struct video_i2c_chip {
 	int (*hwmon_init)(struct video_i2c_data *data);
 };
 
+/* Frame rate register */
+#define AMG88XX_REG_FPSC	0x02
+#define AMG88XX_FPSC_1FPS		BIT(0)
+
 /* Thermistor register */
 #define AMG88XX_REG_TTHL	0x0e
 
@@ -104,6 +114,19 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
 				data->chip->buffer_size);
 }
 
+static int amg88xx_setup(struct video_i2c_data *data)
+{
+	unsigned int mask = AMG88XX_FPSC_1FPS;
+	unsigned int val;
+
+	if (data->frame_interval.numerator == data->frame_interval.denominator)
+		val = mask;
+	else
+		val = 0;
+
+	return regmap_update_bits(data->regmap, AMG88XX_REG_FPSC, mask, val);
+}
+
 #if IS_ENABLED(CONFIG_HWMON)
 
 static const u32 amg88xx_temp_config[] = {
@@ -178,14 +201,21 @@ static int amg88xx_hwmon_init(struct video_i2c_data *data)
 
 #define AMG88XX		0
 
+static const struct v4l2_fract amg88xx_frame_intervals[] = {
+	{ 1, 10 },
+	{ 1, 1 },
+};
+
 static const struct video_i2c_chip video_i2c_chip[] = {
 	[AMG88XX] = {
 		.size		= &amg88xx_size,
 		.format		= &amg88xx_format,
-		.max_fps	= 10,
+		.frame_intervals	= amg88xx_frame_intervals,
+		.num_frame_intervals	= ARRAY_SIZE(amg88xx_frame_intervals),
 		.buffer_size	= 128,
 		.bpp		= 16,
 		.regmap_config	= &amg88xx_regmap_config,
+		.setup		= &amg88xx_setup,
 		.xfer		= &amg88xx_xfer,
 		.hwmon_init	= amg88xx_hwmon_init,
 	},
@@ -250,7 +280,8 @@ static void buffer_queue(struct vb2_buffer *vb)
 static int video_i2c_thread_vid_cap(void *priv)
 {
 	struct video_i2c_data *data = priv;
-	unsigned int delay = msecs_to_jiffies(1000 / data->chip->max_fps);
+	unsigned int delay = mult_frac(HZ, data->frame_interval.numerator,
+				       data->frame_interval.denominator);
 
 	set_freezable();
 
@@ -312,19 +343,26 @@ static void video_i2c_del_list(struct vb2_queue *vq, enum vb2_buffer_state state
 static int start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct video_i2c_data *data = vb2_get_drv_priv(vq);
+	int ret;
 
 	if (data->kthread_vid_cap)
 		return 0;
 
+	ret = data->chip->setup(data);
+	if (ret)
+		goto error_del_list;
+
 	data->sequence = 0;
 	data->kthread_vid_cap = kthread_run(video_i2c_thread_vid_cap, data,
 					    "%s-vid-cap", data->v4l2_dev.name);
-	if (!IS_ERR(data->kthread_vid_cap))
+	ret = PTR_ERR_OR_ZERO(data->kthread_vid_cap);
+	if (!ret)
 		return 0;
 
+error_del_list:
 	video_i2c_del_list(vq, VB2_BUF_STATE_QUEUED);
 
-	return PTR_ERR(data->kthread_vid_cap);
+	return ret;
 }
 
 static void stop_streaming(struct vb2_queue *vq)
@@ -431,15 +469,14 @@ static int video_i2c_enum_frameintervals(struct file *file, void *priv,
 	const struct video_i2c_data *data = video_drvdata(file);
 	const struct v4l2_frmsize_discrete *size = data->chip->size;
 
-	if (fe->index > 0)
+	if (fe->index >= data->chip->num_frame_intervals)
 		return -EINVAL;
 
 	if (fe->width != size->width || fe->height != size->height)
 		return -EINVAL;
 
 	fe->type = V4L2_FRMIVAL_TYPE_DISCRETE;
-	fe->discrete.numerator = 1;
-	fe->discrete.denominator = data->chip->max_fps;
+	fe->discrete = data->chip->frame_intervals[fe->index];
 
 	return 0;
 }
@@ -484,12 +521,27 @@ static int video_i2c_g_parm(struct file *filp, void *priv,
 
 	parm->parm.capture.readbuffers = 1;
 	parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
-	parm->parm.capture.timeperframe.numerator = 1;
-	parm->parm.capture.timeperframe.denominator = data->chip->max_fps;
+	parm->parm.capture.timeperframe = data->frame_interval;
 
 	return 0;
 }
 
+static int video_i2c_s_parm(struct file *filp, void *priv,
+			      struct v4l2_streamparm *parm)
+{
+	struct video_i2c_data *data = video_drvdata(filp);
+	int i;
+
+	for (i = 0; i < data->chip->num_frame_intervals - 1; i++) {
+		if (V4L2_FRACT_COMPARE(parm->parm.capture.timeperframe, <=,
+				       data->chip->frame_intervals[i]))
+			break;
+	}
+	data->frame_interval = data->chip->frame_intervals[i];
+
+	return video_i2c_g_parm(filp, priv, parm);
+}
+
 static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
 	.vidioc_querycap		= video_i2c_querycap,
 	.vidioc_g_input			= video_i2c_g_input,
@@ -501,7 +553,7 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
 	.vidioc_g_fmt_vid_cap		= video_i2c_try_fmt_vid_cap,
 	.vidioc_s_fmt_vid_cap		= video_i2c_s_fmt_vid_cap,
 	.vidioc_g_parm			= video_i2c_g_parm,
-	.vidioc_s_parm			= video_i2c_g_parm,
+	.vidioc_s_parm			= video_i2c_s_parm,
 	.vidioc_try_fmt_vid_cap		= video_i2c_try_fmt_vid_cap,
 	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
 	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
@@ -578,6 +630,8 @@ static int video_i2c_probe(struct i2c_client *client,
 	spin_lock_init(&data->slock);
 	INIT_LIST_HEAD(&data->vid_cap_active);
 
+	data->frame_interval = data->chip->frame_intervals[0];
+
 	video_set_drvdata(&data->vdev, data);
 	i2c_set_clientdata(client, data);
 
-- 
2.7.4

[PATCH v2 6/6] media: video-i2c: support runtime PM

[Akinobu Mita]

AMG88xx has a register for setting operating mode.  This adds support
runtime PM by changing the operating mode.

The instruction for changing sleep mode to normal mode is from the
reference specifications.

https://docid81hrs3j1.cloudfront.net/medialibrary/2017/11/PANA-S-A0002141979-1.pdf

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- Add Reviewed-by tag
- Add comment for register address definisions

 drivers/media/i2c/video-i2c.c | 142 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 140 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 6dd1929..f7058cf 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -94,10 +95,23 @@ struct video_i2c_chip {
 	/* xfer function */
 	int (*xfer)(struct video_i2c_data *data, char *buf);
 
+	/* power control function */
+	int (*set_power)(struct video_i2c_data *data, bool on);
+
 	/* hwmon init function */
 	int (*hwmon_init)(struct video_i2c_data *data);
 };
 
+/* Power control register */
+#define AMG88XX_REG_PCTL	0x00
+#define AMG88XX_PCTL_NORMAL		0x00
+#define AMG88XX_PCTL_SLEEP		0x10
+
+/* Reset register */
+#define AMG88XX_REG_RST		0x01
+#define AMG88XX_RST_FLAG		0x30
+#define AMG88XX_RST_INIT		0x3f
+
 /* Frame rate register */
 #define AMG88XX_REG_FPSC	0x02
 #define AMG88XX_FPSC_1FPS		BIT(0)
@@ -127,6 +141,59 @@ static int amg88xx_setup(struct video_i2c_data *data)
 	return regmap_update_bits(data->regmap, AMG88XX_REG_FPSC, mask, val);
 }
 
+static int amg88xx_set_power_on(struct video_i2c_data *data)
+{
+	int ret;
+
+	ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_NORMAL);
+	if (ret)
+		return ret;
+
+	msleep(50);
+
+	ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_INIT);
+	if (ret)
+		return ret;
+
+	msleep(2);
+
+	ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_FLAG);
+	if (ret)
+		return ret;
+
+	/*
+	 * Wait two frames before reading thermistor and temperature registers
+	 */
+	msleep(200);
+
+	return 0;
+}
+
+static int amg88xx_set_power_off(struct video_i2c_data *data)
+{
+	int ret;
+
+	ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_SLEEP);
+	if (ret)
+		return ret;
+	/*
+	 * Wait for a while to avoid resuming normal mode immediately after
+	 * entering sleep mode, otherwise the device occasionally goes wrong
+	 * (thermistor and temperature registers are not updated at all)
+	 */
+	msleep(100);
+
+	return 0;
+}
+
+static int amg88xx_set_power(struct video_i2c_data *data, bool on)
+{
+	if (on)
+		return amg88xx_set_power_on(data);
+
+	return amg88xx_set_power_off(data);
+}
+
 #if IS_ENABLED(CONFIG_HWMON)
 
 static const u32 amg88xx_temp_config[] = {
@@ -158,7 +225,15 @@ static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type,
 	__le16 buf;
 	int tmp;
 
+	tmp = pm_runtime_get_sync(regmap_get_device(data->regmap));
+	if (tmp < 0) {
+		pm_runtime_put_noidle(regmap_get_device(data->regmap));
+		return tmp;
+	}
+
 	tmp = regmap_bulk_read(data->regmap, AMG88XX_REG_TTHL, &buf, 2);
+	pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
+	pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
 	if (tmp)
 		return tmp;
 
@@ -217,6 +292,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
 		.regmap_config	= &amg88xx_regmap_config,
 		.setup		= &amg88xx_setup,
 		.xfer		= &amg88xx_xfer,
+		.set_power	= amg88xx_set_power,
 		.hwmon_init	= amg88xx_hwmon_init,
 	},
 };
@@ -343,14 +419,21 @@ static void video_i2c_del_list(struct vb2_queue *vq, enum vb2_buffer_state state
 static int start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct video_i2c_data *data = vb2_get_drv_priv(vq);
+	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 
 	if (data->kthread_vid_cap)
 		return 0;
 
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
+		goto error_del_list;
+	}
+
 	ret = data->chip->setup(data);
 	if (ret)
-		goto error_del_list;
+		goto error_rpm_put;
 
 	data->sequence = 0;
 	data->kthread_vid_cap = kthread_run(video_i2c_thread_vid_cap, data,
@@ -359,6 +442,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (!ret)
 		return 0;
 
+error_rpm_put:
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 error_del_list:
 	video_i2c_del_list(vq, VB2_BUF_STATE_QUEUED);
 
@@ -374,6 +460,8 @@ static void stop_streaming(struct vb2_queue *vq)
 
 	kthread_stop(data->kthread_vid_cap);
 	data->kthread_vid_cap = NULL;
+	pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
+	pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
 
 	video_i2c_del_list(vq, VB2_BUF_STATE_ERROR);
 }
@@ -635,6 +723,18 @@ static int video_i2c_probe(struct i2c_client *client,
 	video_set_drvdata(&data->vdev, data);
 	i2c_set_clientdata(client, data);
 
+	ret = data->chip->set_power(data, true);
+	if (ret)
+		goto error_unregister_device;
+
+	pm_runtime_get_noresume(&client->dev);
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_enable(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev, 2000);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_mark_last_busy(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
+
 	if (data->chip->hwmon_init) {
 		ret = data->chip->hwmon_init(data);
 		if (ret < 0) {
@@ -645,10 +745,17 @@ static int video_i2c_probe(struct i2c_client *client,
 
 	ret = video_register_device(&data->vdev, VFL_TYPE_GRABBER, -1);
 	if (ret < 0)
-		goto error_unregister_device;
+		goto error_pm_disable;
 
 	return 0;
 
+error_pm_disable:
+	pm_runtime_get_sync(&client->dev);
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+	data->chip->set_power(data, false);
+
 error_unregister_device:
 	v4l2_device_unregister(v4l2_dev);
 	mutex_destroy(&data->lock);
@@ -662,6 +769,13 @@ static int video_i2c_remove(struct i2c_client *client)
 	struct video_i2c_data *data = i2c_get_clientdata(client);
 
 	video_unregister_device(&data->vdev);
+
+	pm_runtime_get_sync(&client->dev);
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+	data->chip->set_power(data, false);
+
 	v4l2_device_unregister(&data->v4l2_dev);
 
 	mutex_destroy(&data->lock);
@@ -670,6 +784,29 @@ static int video_i2c_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+
+static int video_i2c_pm_runtime_suspend(struct device *dev)
+{
+	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
+
+	return data->chip->set_power(data, false);
+}
+
+static int video_i2c_pm_runtime_resume(struct device *dev)
+{
+	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
+
+	return data->chip->set_power(data, true);
+}
+
+#endif
+
+static const struct dev_pm_ops video_i2c_pm_ops = {
+	SET_RUNTIME_PM_OPS(video_i2c_pm_runtime_suspend,
+			   video_i2c_pm_runtime_resume, NULL)
+};
+
 static const struct i2c_device_id video_i2c_id_table[] = {
 	{ "amg88xx", AMG88XX },
 	{}
@@ -686,6 +823,7 @@ static struct i2c_driver video_i2c_driver = {
 	.driver = {
 		.name	= VIDEO_I2C_DRIVER,
 		.of_match_table = video_i2c_of_match,
+		.pm	= &video_i2c_pm_ops,
 	},
 	.probe		= video_i2c_probe,
 	.remove		= video_i2c_remove,
-- 
2.7.4

Re: [PATCH v2 2/6] media: video-i2c: use i2c regmap

[Matt Ranostay]

On Mon, Sep 24, 2018 at 12:35 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> Use regmap for i2c register access.  This simplifies register accesses and
> chooses suitable access commands based on the functionality that the
> adapter supports.
>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Hans Verkuil <hansverk@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>

> ---
> * v2
> - Add thermistor and termperature register address definisions
>
>  drivers/media/i2c/video-i2c.c | 60 ++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index b7a2af9..fb8509e 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
> +#include <linux/regmap.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  #include <linux/videodev2.h>
> @@ -38,7 +39,7 @@ struct video_i2c_buffer {
>  };
>
>  struct video_i2c_data {
> -       struct i2c_client *client;
> +       struct regmap *regmap;
>         const struct video_i2c_chip *chip;
>         struct mutex lock;
>         spinlock_t slock;
> @@ -62,6 +63,12 @@ static const struct v4l2_frmsize_discrete amg88xx_size = {
>         .height = 8,
>  };
>
> +static const struct regmap_config amg88xx_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .max_register = 0xff
> +};
> +
>  struct video_i2c_chip {
>         /* video dimensions */
>         const struct v4l2_fmtdesc *format;
> @@ -76,6 +83,8 @@ struct video_i2c_chip {
>         /* pixel size in bits */
>         unsigned int bpp;
>
> +       const struct regmap_config *regmap_config;
> +
>         /* xfer function */
>         int (*xfer)(struct video_i2c_data *data, char *buf);
>
> @@ -83,26 +92,16 @@ struct video_i2c_chip {
>         int (*hwmon_init)(struct video_i2c_data *data);
>  };
>
> -static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
> -{
> -       struct i2c_client *client = data->client;
> -       struct i2c_msg msg[2];
> -       u8 reg = 0x80;
> -       int ret;
> -
> -       msg[0].addr = client->addr;
> -       msg[0].flags = 0;
> -       msg[0].len = 1;
> -       msg[0].buf  = (char *)&reg;
> -
> -       msg[1].addr = client->addr;
> -       msg[1].flags = I2C_M_RD;
> -       msg[1].len = data->chip->buffer_size;
> -       msg[1].buf = (char *)buf;
> +/* Thermistor register */
> +#define AMG88XX_REG_TTHL       0x0e
>
> -       ret = i2c_transfer(client->adapter, msg, 2);
> +/* Temperature register */
> +#define AMG88XX_REG_T01L       0x80
>
> -       return (ret == 2) ? 0 : -EIO;
> +static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
> +{
> +       return regmap_bulk_read(data->regmap, AMG88XX_REG_T01L, buf,
> +                               data->chip->buffer_size);
>  }
>
>  #if IS_ENABLED(CONFIG_HWMON)
> @@ -133,12 +132,15 @@ static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type,
>                         u32 attr, int channel, long *val)
>  {
>         struct video_i2c_data *data = dev_get_drvdata(dev);
> -       struct i2c_client *client = data->client;
> -       int tmp = i2c_smbus_read_word_data(client, 0x0e);
> +       __le16 buf;
> +       int tmp;
>
> -       if (tmp < 0)
> +       tmp = regmap_bulk_read(data->regmap, AMG88XX_REG_TTHL, &buf, 2);
> +       if (tmp)
>                 return tmp;
>
> +       tmp = le16_to_cpu(buf);
> +
>         /*
>          * Check for sign bit, this isn't a two's complement value but an
>          * absolute temperature that needs to be inverted in the case of being
> @@ -164,8 +166,9 @@ static const struct hwmon_chip_info amg88xx_chip_info = {
>
>  static int amg88xx_hwmon_init(struct video_i2c_data *data)
>  {
> -       void *hwmon = devm_hwmon_device_register_with_info(&data->client->dev,
> -                               "amg88xx", data, &amg88xx_chip_info, NULL);
> +       struct device *dev = regmap_get_device(data->regmap);
> +       void *hwmon = devm_hwmon_device_register_with_info(dev, "amg88xx", data,
> +                                               &amg88xx_chip_info, NULL);
>
>         return PTR_ERR_OR_ZERO(hwmon);
>  }
> @@ -182,6 +185,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
>                 .max_fps        = 10,
>                 .buffer_size    = 128,
>                 .bpp            = 16,
> +               .regmap_config  = &amg88xx_regmap_config,
>                 .xfer           = &amg88xx_xfer,
>                 .hwmon_init     = amg88xx_hwmon_init,
>         },
> @@ -350,7 +354,8 @@ static int video_i2c_querycap(struct file *file, void  *priv,
>                                 struct v4l2_capability *vcap)
>  {
>         struct video_i2c_data *data = video_drvdata(file);
> -       struct i2c_client *client = data->client;
> +       struct device *dev = regmap_get_device(data->regmap);
> +       struct i2c_client *client = to_i2c_client(dev);
>
>         strlcpy(vcap->driver, data->v4l2_dev.name, sizeof(vcap->driver));
>         strlcpy(vcap->card, data->vdev.name, sizeof(vcap->card));
> @@ -527,7 +532,10 @@ static int video_i2c_probe(struct i2c_client *client,
>         else
>                 return -ENODEV;
>
> -       data->client = client;
> +       data->regmap = devm_regmap_init_i2c(client, data->chip->regmap_config);
> +       if (IS_ERR(data->regmap))
> +               return PTR_ERR(data->regmap);
> +
>         v4l2_dev = &data->v4l2_dev;
>         strlcpy(v4l2_dev->name, VIDEO_I2C_DRIVER, sizeof(v4l2_dev->name));
>
> --
> 2.7.4
>

Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Hans Verkuil]

On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> The video_i2c_data is allocated by kzalloc and released by the video
> device's release callback.  The release callback is called when
> video_unregister_device() is called, but it will still be accessed after
> calling video_unregister_device().
> 
> Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
> i2c_client->dev so that it will automatically be released when the i2c
> driver is removed.

Hmm. The patch is right, but the explanation isn't. The core problem is
that vdev.release was set to video_i2c_release, but that should only be
used if struct video_device was kzalloc'ed. But in this case it is embedded
in a larger struct, and then vdev.release should always be set to
video_device_release_empty.

That was the real reason for the invalid access.

Regards,

	Hans

> 
> Fixes: 5cebaac60974 ("media: video-i2c: add video-i2c driver")
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Hans Verkuil <hansverk@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v2
> - Update commit log to clarify the use after free
> - Add Acked-by tag
> 
>  drivers/media/i2c/video-i2c.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 06d29d8..b7a2af9 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -508,20 +508,15 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
>  	.vidioc_streamoff		= vb2_ioctl_streamoff,
>  };
>  
> -static void video_i2c_release(struct video_device *vdev)
> -{
> -	kfree(video_get_drvdata(vdev));
> -}
> -
>  static int video_i2c_probe(struct i2c_client *client,
>  			     const struct i2c_device_id *id)
>  {
>  	struct video_i2c_data *data;
>  	struct v4l2_device *v4l2_dev;
>  	struct vb2_queue *queue;
> -	int ret = -ENODEV;
> +	int ret;
>  
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> @@ -530,7 +525,7 @@ static int video_i2c_probe(struct i2c_client *client,
>  	else if (id)
>  		data->chip = &video_i2c_chip[id->driver_data];
>  	else
> -		goto error_free_device;
> +		return -ENODEV;
>  
>  	data->client = client;
>  	v4l2_dev = &data->v4l2_dev;
> @@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
>  
>  	ret = v4l2_device_register(&client->dev, v4l2_dev);
>  	if (ret < 0)
> -		goto error_free_device;
> +		return ret;
>  
>  	mutex_init(&data->lock);
>  	mutex_init(&data->queue_lock);
> @@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
>  	data->vdev.fops = &video_i2c_fops;
>  	data->vdev.lock = &data->lock;
>  	data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
> -	data->vdev.release = video_i2c_release;
> +	data->vdev.release = video_device_release_empty;
>  	data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
>  				 V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
>  
> @@ -597,9 +592,6 @@ static int video_i2c_probe(struct i2c_client *client,
>  	mutex_destroy(&data->lock);
>  	mutex_destroy(&data->queue_lock);
>  
> -error_free_device:
> -	kfree(data);
> -
>  	return ret;
>  }
>  
> 

Re: [PATCH v2 0/6] media: video-i2c: support changing frame interval and runtime PM

[Hans Verkuil]

On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> This patchset adds support for changing frame interval and runtime PM for
> video-i2c driver.  This also adds an helper macro to v4l2 common
> internal API that is used to to find a suitable frame interval.
> 
> There are a couple of unrelated changes that are included for simplifying
> driver initialization code and register accesses.
> 
> * v2
> - Add Acked-by and Reviewed-by tags
> - Update commit log to clarify the use after free
> - Add thermistor and termperature register address definisions
> - Stop adding v4l2_find_closest_fract() in v4l2 common internal API
> - Add V4L2_FRACT_COMPARE() macro in v4l2 common internal API
> - Use V4L2_FRACT_COMPARE() to find suitable frame interval instead of
>   v4l2_find_closest_fract()

1) What's wrong with v4l2_find_closest_fract()?

2) Please test this with the latest v4l2-compliance: I recently improved
   the S_PARM checks, so I want to make sure this driver passes those tests.

Regards,

	Hans

> - Add comment for register address definisions
> 
> Akinobu Mita (6):
>   media: video-i2c: avoid accessing released memory area when removing
>     driver
>   media: video-i2c: use i2c regmap
>   media: v4l2-common: add V4L2_FRACT_COMPARE
>   media: vivid: use V4L2_FRACT_COMPARE
>   media: video-i2c: support changing frame interval
>   media: video-i2c: support runtime PM
> 
>  drivers/media/i2c/video-i2c.c                | 286 ++++++++++++++++++++++-----
>  drivers/media/platform/vivid/vivid-vid-cap.c |   9 +-
>  include/media/v4l2-common.h                  |   5 +
>  3 files changed, 247 insertions(+), 53 deletions(-)
> 
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Hans Verkuil <hansverk@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> 

Re: [PATCH v2 0/6] media: video-i2c: support changing frame interval and runtime PM

[Akinobu Mita]

2018年10月1日(月) 18:41 Hans Verkuil <hverkuil@xs4all.nl>:
>
> On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> > This patchset adds support for changing frame interval and runtime PM for
> > video-i2c driver.  This also adds an helper macro to v4l2 common
> > internal API that is used to to find a suitable frame interval.
> >
> > There are a couple of unrelated changes that are included for simplifying
> > driver initialization code and register accesses.
> >
> > * v2
> > - Add Acked-by and Reviewed-by tags
> > - Update commit log to clarify the use after free
> > - Add thermistor and termperature register address definisions
> > - Stop adding v4l2_find_closest_fract() in v4l2 common internal API
> > - Add V4L2_FRACT_COMPARE() macro in v4l2 common internal API
> > - Use V4L2_FRACT_COMPARE() to find suitable frame interval instead of
> >   v4l2_find_closest_fract()
>
> 1) What's wrong with v4l2_find_closest_fract()?

My implementation of v4l2_find_closest_fract() in v1 didn't care about
u32 overflow.  Even if the overflow problem is fixed, there are only a
few drivers (video-i2c and vivid) that can make use of it.  So I feel
it adds more lines of code than it can remove.

> 2) Please test this with the latest v4l2-compliance: I recently improved
>    the S_PARM checks, so I want to make sure this driver passes those tests.

v4l2-compliance SHA: 7bde5ef172bd4a09b9544788ba9c5dbb1aa9994a, 32 bits

Compliance test for device /dev/video2:

Driver Info:
Driver name      : video-i2c
Card type        : I2C 1-104 Transport Video
Bus info         : I2C:1-104
Driver version   : 4.19.0
Capabilities     : 0x85200001
Video Capture
Read/Write
Streaming
Extended Pix Format
Device Capabilities
Device Caps      : 0x05200001
Video Capture
Read/Write
Streaming
Extended Pix Format

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second /dev/video2 open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK
test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0

Format ioctls (Input 0):
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK (Not Supported)

Total: 43, Succeeded: 43, Failed: 0, Warnings: 0

Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Akinobu Mita]

2018年10月1日(月) 18:40 Hans Verkuil <hverkuil@xs4all.nl>:
>
> On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> > The video_i2c_data is allocated by kzalloc and released by the video
> > device's release callback.  The release callback is called when
> > video_unregister_device() is called, but it will still be accessed after
> > calling video_unregister_device().
> >
> > Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
> > i2c_client->dev so that it will automatically be released when the i2c
> > driver is removed.
>
> Hmm. The patch is right, but the explanation isn't. The core problem is
> that vdev.release was set to video_i2c_release, but that should only be
> used if struct video_device was kzalloc'ed. But in this case it is embedded
> in a larger struct, and then vdev.release should always be set to
> video_device_release_empty.
>
> That was the real reason for the invalid access.

How about the commit log below?

struct video_device for video-i2c is embedded in a struct video_i2c_data,
and then vdev.release should always be set to video_device_release_empty.

However, the vdev.release is currently set to video_i2c_release which
frees the whole struct video_i2c_data.  In video_i2c_remove(), some fields
(v4l2_dev, lock, and queue_lock) in struct video_i2c_data are still
accessed after video_unregister_device().

This fixes the use after free by setting the vdev.release to
video_device_release_empty.  Also, convert to use devm_kzalloc() for
allocating a struct video_i2c_data in order to simplify the code.

Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Hans Verkuil]

On 10/02/18 18:17, Akinobu Mita wrote:
> 2018年10月1日(月) 18:40 Hans Verkuil <hverkuil@xs4all.nl>:
>>
>> On 09/23/2018 06:34 PM, Akinobu Mita wrote:
>>> The video_i2c_data is allocated by kzalloc and released by the video
>>> device's release callback.  The release callback is called when
>>> video_unregister_device() is called, but it will still be accessed after
>>> calling video_unregister_device().
>>>
>>> Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
>>> i2c_client->dev so that it will automatically be released when the i2c
>>> driver is removed.
>>
>> Hmm. The patch is right, but the explanation isn't. The core problem is
>> that vdev.release was set to video_i2c_release, but that should only be
>> used if struct video_device was kzalloc'ed. But in this case it is embedded
>> in a larger struct, and then vdev.release should always be set to
>> video_device_release_empty.
>>
>> That was the real reason for the invalid access.
> 
> How about the commit log below?
> 
> struct video_device for video-i2c is embedded in a struct video_i2c_data,
> and then vdev.release should always be set to video_device_release_empty.
> 
> However, the vdev.release is currently set to video_i2c_release which
> frees the whole struct video_i2c_data.  In video_i2c_remove(), some fields
> (v4l2_dev, lock, and queue_lock) in struct video_i2c_data are still
> accessed after video_unregister_device().
> 
> This fixes the use after free by setting the vdev.release to
> video_device_release_empty.  Also, convert to use devm_kzalloc() for
> allocating a struct video_i2c_data in order to simplify the code.
> 

Looks good.

Regards,

	Hans

Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Sakari Ailus]

Hi Hans,

On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
> On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> > The video_i2c_data is allocated by kzalloc and released by the video
> > device's release callback.  The release callback is called when
> > video_unregister_device() is called, but it will still be accessed after
> > calling video_unregister_device().
> > 
> > Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
> > i2c_client->dev so that it will automatically be released when the i2c
> > driver is removed.
> 
> Hmm. The patch is right, but the explanation isn't. The core problem is
> that vdev.release was set to video_i2c_release, but that should only be
> used if struct video_device was kzalloc'ed. But in this case it is embedded
> in a larger struct, and then vdev.release should always be set to
> video_device_release_empty.

When the driver is unbound, what's acquired using the devm_() family of
functions is released. At the same time, the user still holds a file
handle, and can issue IOCTLs --- but the device's data structures no longer
exist.

That's not ok, and also the reason why we have the release callback.

While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
fine.

Or am I missing something?

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Akinobu Mita]

2018年10月5日(金) 18:36 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Hans,
>
> On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
> > On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> > > The video_i2c_data is allocated by kzalloc and released by the video
> > > device's release callback.  The release callback is called when
> > > video_unregister_device() is called, but it will still be accessed after
> > > calling video_unregister_device().
> > >
> > > Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
> > > i2c_client->dev so that it will automatically be released when the i2c
> > > driver is removed.
> >
> > Hmm. The patch is right, but the explanation isn't. The core problem is
> > that vdev.release was set to video_i2c_release, but that should only be
> > used if struct video_device was kzalloc'ed. But in this case it is embedded
> > in a larger struct, and then vdev.release should always be set to
> > video_device_release_empty.
>
> When the driver is unbound, what's acquired using the devm_() family of
> functions is released. At the same time, the user still holds a file
> handle, and can issue IOCTLs --- but the device's data structures no longer
> exist.
>
> That's not ok, and also the reason why we have the release callback.
>
> While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
> fine.
>
> Or am I missing something?

How about moving the lines causing use-after-free to release callback
like below?

static void video_i2c_release(struct video_device *vdev)
{
        struct video_i2c_data *data = video_get_drvdata(vdev);

        v4l2_device_unregister(&data->v4l2_dev);
        mutex_destroy(&data->lock);
        mutex_destroy(&data->queue_lock);
        kfree(data);
}

Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Sakari Ailus]

Hi Mita-san,

On Fri, Oct 05, 2018 at 11:59:20PM +0900, Akinobu Mita wrote:
> 2018年10月5日(金) 18:36 Sakari Ailus <sakari.ailus@linux.intel.com>:
> >
> > Hi Hans,
> >
> > On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
> > > On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> > > > The video_i2c_data is allocated by kzalloc and released by the video
> > > > device's release callback.  The release callback is called when
> > > > video_unregister_device() is called, but it will still be accessed after
> > > > calling video_unregister_device().
> > > >
> > > > Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
> > > > i2c_client->dev so that it will automatically be released when the i2c
> > > > driver is removed.
> > >
> > > Hmm. The patch is right, but the explanation isn't. The core problem is
> > > that vdev.release was set to video_i2c_release, but that should only be
> > > used if struct video_device was kzalloc'ed. But in this case it is embedded
> > > in a larger struct, and then vdev.release should always be set to
> > > video_device_release_empty.
> >
> > When the driver is unbound, what's acquired using the devm_() family of
> > functions is released. At the same time, the user still holds a file
> > handle, and can issue IOCTLs --- but the device's data structures no longer
> > exist.
> >
> > That's not ok, and also the reason why we have the release callback.
> >
> > While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
> > fine.
> >
> > Or am I missing something?
> 
> How about moving the lines causing use-after-free to release callback
> like below?
> 
> static void video_i2c_release(struct video_device *vdev)
> {
>         struct video_i2c_data *data = video_get_drvdata(vdev);
> 
>         v4l2_device_unregister(&data->v4l2_dev);
>         mutex_destroy(&data->lock);
>         mutex_destroy(&data->queue_lock);
>         kfree(data);
> }


So the remove function would only contain video_unregister_device()? That's
the way it should be, as far as I can see.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 0/6] media: video-i2c: support changing frame interval and runtime PM

[Sakari Ailus]

On Mon, Sep 24, 2018 at 01:34:46AM +0900, Akinobu Mita wrote:
> This patchset adds support for changing frame interval and runtime PM for
> video-i2c driver.  This also adds an helper macro to v4l2 common
> internal API that is used to to find a suitable frame interval.
> 
> There are a couple of unrelated changes that are included for simplifying
> driver initialization code and register accesses.
> 
> * v2
> - Add Acked-by and Reviewed-by tags
> - Update commit log to clarify the use after free
> - Add thermistor and termperature register address definisions
> - Stop adding v4l2_find_closest_fract() in v4l2 common internal API
> - Add V4L2_FRACT_COMPARE() macro in v4l2 common internal API
> - Use V4L2_FRACT_COMPARE() to find suitable frame interval instead of
>   v4l2_find_closest_fract()
> - Add comment for register address definisions
> 
> Akinobu Mita (6):
>   media: video-i2c: avoid accessing released memory area when removing
>     driver
>   media: video-i2c: use i2c regmap
>   media: v4l2-common: add V4L2_FRACT_COMPARE
>   media: vivid: use V4L2_FRACT_COMPARE
>   media: video-i2c: support changing frame interval
>   media: video-i2c: support runtime PM

For patches 2--6:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Hans Verkuil]

On 10/05/2018 04:59 PM, Akinobu Mita wrote:
> 2018年10月5日(金) 18:36 Sakari Ailus <sakari.ailus@linux.intel.com>:
>>
>> Hi Hans,
>>
>> On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
>>> On 09/23/2018 06:34 PM, Akinobu Mita wrote:
>>>> The video_i2c_data is allocated by kzalloc and released by the video
>>>> device's release callback.  The release callback is called when
>>>> video_unregister_device() is called, but it will still be accessed after
>>>> calling video_unregister_device().
>>>>
>>>> Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
>>>> i2c_client->dev so that it will automatically be released when the i2c
>>>> driver is removed.
>>>
>>> Hmm. The patch is right, but the explanation isn't. The core problem is
>>> that vdev.release was set to video_i2c_release, but that should only be
>>> used if struct video_device was kzalloc'ed. But in this case it is embedded
>>> in a larger struct, and then vdev.release should always be set to
>>> video_device_release_empty.
>>
>> When the driver is unbound, what's acquired using the devm_() family of
>> functions is released. At the same time, the user still holds a file
>> handle, and can issue IOCTLs --- but the device's data structures no longer
>> exist.
>>
>> That's not ok, and also the reason why we have the release callback.
>>
>> While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
>> fine.
>>
>> Or am I missing something?
> 
> How about moving the lines causing use-after-free to release callback
> like below?
> 
> static void video_i2c_release(struct video_device *vdev)
> {
>         struct video_i2c_data *data = video_get_drvdata(vdev);
> 
>         v4l2_device_unregister(&data->v4l2_dev);
>         mutex_destroy(&data->lock);
>         mutex_destroy(&data->queue_lock);
>         kfree(data);
> }
> 

You can test this with v4l2-ctl:

v4l2-ctl --sleep 10

This sleeps 10s, then calls QUERYCAP and closes the file handle.

In another shell you can unbind the driver while v4l2-ctl is sleeping.

Hopefully this works without crashing anything :-)

Regards,

	Hans

Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Akinobu Mita]

2018年10月8日(月) 20:21 Hans Verkuil <hverkuil@xs4all.nl>:
>
> On 10/05/2018 04:59 PM, Akinobu Mita wrote:
> > 2018年10月5日(金) 18:36 Sakari Ailus <sakari.ailus@linux.intel.com>:
> >>
> >> Hi Hans,
> >>
> >> On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
> >>> On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> >>>> The video_i2c_data is allocated by kzalloc and released by the video
> >>>> device's release callback.  The release callback is called when
> >>>> video_unregister_device() is called, but it will still be accessed after
> >>>> calling video_unregister_device().
> >>>>
> >>>> Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
> >>>> i2c_client->dev so that it will automatically be released when the i2c
> >>>> driver is removed.
> >>>
> >>> Hmm. The patch is right, but the explanation isn't. The core problem is
> >>> that vdev.release was set to video_i2c_release, but that should only be
> >>> used if struct video_device was kzalloc'ed. But in this case it is embedded
> >>> in a larger struct, and then vdev.release should always be set to
> >>> video_device_release_empty.
> >>
> >> When the driver is unbound, what's acquired using the devm_() family of
> >> functions is released. At the same time, the user still holds a file
> >> handle, and can issue IOCTLs --- but the device's data structures no longer
> >> exist.
> >>
> >> That's not ok, and also the reason why we have the release callback.
> >>
> >> While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
> >> fine.
> >>
> >> Or am I missing something?
> >
> > How about moving the lines causing use-after-free to release callback
> > like below?
> >
> > static void video_i2c_release(struct video_device *vdev)
> > {
> >         struct video_i2c_data *data = video_get_drvdata(vdev);
> >
> >         v4l2_device_unregister(&data->v4l2_dev);
> >         mutex_destroy(&data->lock);
> >         mutex_destroy(&data->queue_lock);
> >         kfree(data);
> > }
> >
>
> You can test this with v4l2-ctl:
>
> v4l2-ctl --sleep 10
>
> This sleeps 10s, then calls QUERYCAP and closes the file handle.
>
> In another shell you can unbind the driver while v4l2-ctl is sleeping.
>
> Hopefully this works without crashing anything :-)

I tried that and the command finished without crash.

$ v4l2-ctl --sleep 10 -d /dev/video2
Test VIDIOC_QUERYCAP:
                VIDIOC_QUERYCAP returned -1 (No such device)
VIDIOC_QUERYCAP: No such device

This -ENODEV should be ok as V4L2_FL_REGISTERED flag has already been
cleared by video_unregister_device().