[PATCH 0/6] Media controller and omap3isp cleanups

[PATCH 0/6] Media controller and omap3isp cleanups

[Sakari Ailus]

Hi all,

These patches prepare for media device refcounting but do not begin those
changes yet. They're worthwhile on their own merits.

-- 
Kind regards,
Sakari

[PATCH 1/6] omap3isp: Don't rely on devm for memory resource management

[Sakari Ailus]

devm functions are fine for managing resources that are directly related
to the device at hand and that have no other dependencies. However, a
process holding a file handle to a device created by a driver for a device
may result in the file handle left behind after the device is long gone.
This will result in accessing released (and potentially reallocated)
memory.

Instead, manage the memory resources in the driver. Releasing the
resources can be later on bound to e.g. by releasing a reference.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c         | 18 ++++++++++++------
 drivers/media/platform/omap3isp/isph3a_aewb.c | 24 +++++++++++++++++-------
 drivers/media/platform/omap3isp/isph3a_af.c   | 24 +++++++++++++++++-------
 drivers/media/platform/omap3isp/isphist.c     | 11 +++++++----
 drivers/media/platform/omap3isp/ispstat.c     |  2 ++
 5 files changed, 55 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 084ecf4a..874f6fc 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2015,6 +2015,8 @@ static int isp_remove(struct platform_device *pdev)
 
 	media_entity_enum_cleanup(&isp->crashed);
 
+	kfree(isp);
+
 	return 0;
 }
 
@@ -2203,7 +2205,7 @@ static int isp_probe(struct platform_device *pdev)
 	int ret;
 	int i, m;
 
-	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
+	isp = kzalloc(sizeof(*isp), GFP_KERNEL);
 	if (!isp) {
 		dev_err(&pdev->dev, "could not allocate memory\n");
 		return -ENOMEM;
@@ -2212,21 +2214,23 @@ static int isp_probe(struct platform_device *pdev)
 	ret = of_property_read_u32(pdev->dev.of_node, "ti,phy-type",
 				   &isp->phy_type);
 	if (ret)
-		return ret;
+		goto error_release_isp;
 
 	isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
 						      "syscon");
-	if (IS_ERR(isp->syscon))
-		return PTR_ERR(isp->syscon);
+	if (IS_ERR(isp->syscon)) {
+		ret = PTR_ERR(isp->syscon);
+		goto error_release_isp;
+	}
 
 	ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1,
 					 &isp->syscon_offset);
 	if (ret)
-		return ret;
+		goto error_release_isp;
 
 	ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier);
 	if (ret < 0)
-		return ret;
+		goto error_release_isp;
 
 	isp->autoidle = autoidle;
 
@@ -2376,6 +2380,8 @@ static int isp_probe(struct platform_device *pdev)
 	__omap3isp_put(isp, false);
 error:
 	mutex_destroy(&isp->isp_mutex);
+error_release_isp:
+	kfree(isp);
 
 	return ret;
 }
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c
index d44626f2..efddafd 100644
--- a/drivers/media/platform/omap3isp/isph3a_aewb.c
+++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
@@ -289,9 +289,10 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 {
 	struct ispstat *aewb = &isp->isp_aewb;
 	struct omap3isp_h3a_aewb_config *aewb_cfg;
-	struct omap3isp_h3a_aewb_config *aewb_recover_cfg;
+	struct omap3isp_h3a_aewb_config *aewb_recover_cfg = NULL;
+	int ret;
 
-	aewb_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_cfg), GFP_KERNEL);
+	aewb_cfg = kzalloc(sizeof(*aewb_cfg), GFP_KERNEL);
 	if (!aewb_cfg)
 		return -ENOMEM;
 
@@ -301,12 +302,12 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 	aewb->isp = isp;
 
 	/* Set recover state configuration */
-	aewb_recover_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_recover_cfg),
-					GFP_KERNEL);
+	aewb_recover_cfg = kzalloc(sizeof(*aewb_recover_cfg), GFP_KERNEL);
 	if (!aewb_recover_cfg) {
 		dev_err(aewb->isp->dev,
 			"AEWB: cannot allocate memory for recover configuration.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	aewb_recover_cfg->saturation_limit = OMAP3ISP_AEWB_MAX_SATURATION_LIM;
@@ -323,13 +324,22 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 	if (h3a_aewb_validate_params(aewb, aewb_recover_cfg)) {
 		dev_err(aewb->isp->dev,
 			"AEWB: recover configuration is invalid.\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	aewb_recover_cfg->buf_size = h3a_aewb_get_buf_size(aewb_recover_cfg);
 	aewb->recover_priv = aewb_recover_cfg;
 
-	return omap3isp_stat_init(aewb, "AEWB", &h3a_aewb_subdev_ops);
+	ret = omap3isp_stat_init(aewb, "AEWB", &h3a_aewb_subdev_ops);
+
+err:
+	if (ret) {
+		kfree(aewb_cfg);
+		kfree(aewb_recover_cfg);
+	}
+
+	return ret;
 }
 
 /*
diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c
index 99bd6cc..fee3d19 100644
--- a/drivers/media/platform/omap3isp/isph3a_af.c
+++ b/drivers/media/platform/omap3isp/isph3a_af.c
@@ -352,9 +352,10 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 {
 	struct ispstat *af = &isp->isp_af;
 	struct omap3isp_h3a_af_config *af_cfg;
-	struct omap3isp_h3a_af_config *af_recover_cfg;
+	struct omap3isp_h3a_af_config *af_recover_cfg = NULL;
+	int ret;
 
-	af_cfg = devm_kzalloc(isp->dev, sizeof(*af_cfg), GFP_KERNEL);
+	af_cfg = kzalloc(sizeof(*af_cfg), GFP_KERNEL);
 	if (af_cfg == NULL)
 		return -ENOMEM;
 
@@ -364,12 +365,12 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 	af->isp = isp;
 
 	/* Set recover state configuration */
-	af_recover_cfg = devm_kzalloc(isp->dev, sizeof(*af_recover_cfg),
-				      GFP_KERNEL);
+	af_recover_cfg = kzalloc(sizeof(*af_recover_cfg), GFP_KERNEL);
 	if (!af_recover_cfg) {
 		dev_err(af->isp->dev,
 			"AF: cannot allocate memory for recover configuration.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	af_recover_cfg->paxel.h_start = OMAP3ISP_AF_PAXEL_HZSTART_MIN;
@@ -381,13 +382,22 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 	if (h3a_af_validate_params(af, af_recover_cfg)) {
 		dev_err(af->isp->dev,
 			"AF: recover configuration is invalid.\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	af_recover_cfg->buf_size = h3a_af_get_buf_size(af_recover_cfg);
 	af->recover_priv = af_recover_cfg;
 
-	return omap3isp_stat_init(af, "AF", &h3a_af_subdev_ops);
+	ret = omap3isp_stat_init(af, "AF", &h3a_af_subdev_ops);
+
+err:
+	if (ret) {
+		kfree(af_cfg);
+		kfree(af_recover_cfg);
+	}
+
+	return ret;
 }
 
 void omap3isp_h3a_af_cleanup(struct isp_device *isp)
diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c
index a4ed5d1..9e448c6 100644
--- a/drivers/media/platform/omap3isp/isphist.c
+++ b/drivers/media/platform/omap3isp/isphist.c
@@ -476,9 +476,9 @@ int omap3isp_hist_init(struct isp_device *isp)
 {
 	struct ispstat *hist = &isp->isp_hist;
 	struct omap3isp_hist_config *hist_cfg;
-	int ret = -1;
+	int ret;
 
-	hist_cfg = devm_kzalloc(isp->dev, sizeof(*hist_cfg), GFP_KERNEL);
+	hist_cfg = kzalloc(sizeof(*hist_cfg), GFP_KERNEL);
 	if (hist_cfg == NULL)
 		return -ENOMEM;
 
@@ -500,7 +500,7 @@ int omap3isp_hist_init(struct isp_device *isp)
 		if (IS_ERR(hist->dma_ch)) {
 			ret = PTR_ERR(hist->dma_ch);
 			if (ret == -EPROBE_DEFER)
-				return ret;
+				goto err;
 
 			hist->dma_ch = NULL;
 			dev_warn(isp->dev,
@@ -516,9 +516,12 @@ int omap3isp_hist_init(struct isp_device *isp)
 	hist->event_type = V4L2_EVENT_OMAP3ISP_HIST;
 
 	ret = omap3isp_stat_init(hist, "histogram", &hist_subdev_ops);
+
+err:
 	if (ret) {
-		if (hist->dma_ch)
+		if (!IS_ERR(hist->dma_ch))
 			dma_release_channel(hist->dma_ch);
+		kfree(hist_cfg);
 	}
 
 	return ret;
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index 47cbc7e..aad0bc3 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -1067,4 +1067,6 @@ void omap3isp_stat_cleanup(struct ispstat *stat)
 	mutex_destroy(&stat->ioctl_lock);
 	isp_stat_bufs_free(stat);
 	kfree(stat->buf);
+	kfree(stat->priv);
+	kfree(stat->recover_priv);
 }
-- 
2.1.4

[PATCH 2/6] omap3isp: Call video_unregister_device() unconditionally

[Sakari Ailus]

video_unregister_device() can be called on a never or an already
unregistered device. Drop the redundant check.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/ispvideo.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 218e6d7..9e9b18c 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1495,6 +1495,5 @@ int omap3isp_video_register(struct isp_video *video, struct v4l2_device *vdev)
 
 void omap3isp_video_unregister(struct isp_video *video)
 {
-	if (video_is_registered(&video->video))
-		video_unregister_device(&video->video);
+	video_unregister_device(&video->video);
 }
-- 
2.1.4

[PATCH 3/6] omap3isp: Remove misleading comment

[Sakari Ailus]

The intent of the check the comment is related to is to ensure we are
streaming --- still. Not that streaming wouldn't be enabled yet. Remove
it.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/ispvideo.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 9e9b18c..a3ca2a4 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1205,7 +1205,6 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 
 	mutex_lock(&video->stream_lock);
 
-	/* Make sure we're not streaming yet. */
 	mutex_lock(&video->queue_lock);
 	streaming = vb2_is_streaming(&vfh->queue);
 	mutex_unlock(&video->queue_lock);
-- 
2.1.4

[PATCH 4/6] omap3isp: Disable streaming at driver unbind time

[Sakari Ailus]

Once the driver is unbound accessing the hardware is not allowed anymore.
Due to this, disable streaming when the device driver is unbound. The
states of the associated objects related to Media controller and videobuf2
frameworks are updated as well, just like if the application disabled
streaming explicitly.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/ispvideo.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index a3ca2a4..c04d357 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1191,22 +1191,17 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 }
 
 static int
-isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
+__isp_video_streamoff(struct isp_video *video)
 {
-	struct isp_video_fh *vfh = to_isp_video_fh(fh);
-	struct isp_video *video = video_drvdata(file);
 	struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
 	enum isp_pipeline_state state;
 	unsigned int streaming;
 	unsigned long flags;
 
-	if (type != video->type)
-		return -EINVAL;
-
 	mutex_lock(&video->stream_lock);
 
 	mutex_lock(&video->queue_lock);
-	streaming = vb2_is_streaming(&vfh->queue);
+	streaming = video->queue && vb2_is_streaming(video->queue);
 	mutex_unlock(&video->queue_lock);
 
 	if (!streaming)
@@ -1229,7 +1224,7 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	omap3isp_video_cancel_stream(video);
 
 	mutex_lock(&video->queue_lock);
-	vb2_streamoff(&vfh->queue, type);
+	vb2_streamoff(video->queue, video->queue->type);
 	mutex_unlock(&video->queue_lock);
 	video->queue = NULL;
 	video->error = false;
@@ -1245,6 +1240,17 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 }
 
 static int
+isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
+{
+	struct isp_video *video = video_drvdata(file);
+
+	if (type != video->type)
+		return -EINVAL;
+
+	return __isp_video_streamoff(video);
+}
+
+static int
 isp_video_enum_input(struct file *file, void *fh, struct v4l2_input *input)
 {
 	if (input->index > 0)
@@ -1494,5 +1500,6 @@ int omap3isp_video_register(struct isp_video *video, struct v4l2_device *vdev)
 
 void omap3isp_video_unregister(struct isp_video *video)
 {
+	__isp_video_streamoff(video);
 	video_unregister_device(&video->video);
 }
-- 
2.1.4

[PATCH 5/6] media: Remove useless curly braces and parentheses

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 760e3e4..c51e2e5 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -591,9 +591,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 			       &entity->pads[i].graph_obj);
 
 	/* invoke entity_notify callbacks */
-	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
-		(notify)->notify(entity, notify->notify_data);
-	}
+	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
+		notify->notify(entity, notify->notify_data);
 
 	if (mdev->entity_internal_idx_max
 	    >= mdev->pm_count_walk.ent_enum.idx_max) {
-- 
2.1.4

[PATCH 6/6] media: devnode: Rename mdev argument as devnode

[Sakari Ailus]

Historically, mdev argument name was being used on both struct
media_device and struct media_devnode. Recently most occurrences of mdev
referring to struct media_devnode were replaced by devnode, which makes
more sense. Fix the last remaining occurrence.

Fixes: 163f1e93e9950 ("[media] media-devnode: fix namespace mess")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index c51e2e5..fce91b5 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -537,9 +537,9 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
  * Registration/unregistration
  */
 
-static void media_device_release(struct media_devnode *mdev)
+static void media_device_release(struct media_devnode *devnode)
 {
-	dev_dbg(mdev->parent, "Media device released\n");
+	dev_dbg(devnode->parent, "Media device released\n");
 }
 
 /**
-- 
2.1.4

Re: [PATCH 0/6] Media controller and omap3isp cleanups

[Hans Verkuil]

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

	Hans

On 02/20/2017 07:22 AM, Sakari Ailus wrote:
> Hi all,
> 
> These patches prepare for media device refcounting but do not begin those
> changes yet. They're worthwhile on their own merits.
> 

Re: [PATCH 4/6] omap3isp: Disable streaming at driver unbind time

[Sakari Ailus]

On Tue, Feb 28, 2017 at 04:53:11PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday 28 Feb 2017 16:00:01 Sakari Ailus wrote:
> > > On Monday 20 Feb 2017 17:22:20 Sakari Ailus wrote:
> > >> Once the driver is unbound accessing the hardware is not allowed anymore.
> > >> Due to this, disable streaming when the device driver is unbound. The
> > >> states of the associated objects related to Media controller and
> > >> videobuf2 frameworks are updated as well, just like if the application
> > >> disabled streaming explicitly.
> > >> 
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > 
> > > This looks mostly good to me, although I'm a bit concerned about race
> > > conditions related to buffer handling. I don't think this patch introduces
> > > any new one though, so
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > We'll have to go through buffer management at some point in the near
> > > future, including from a V4L2 API point of view I think.
> > 
> > Thanks for the review!
> > 
> > Are you happy with me sending a pull request on the set, or would you
> > prefer to pick the omap3isp patches? In the latter case I'll send a fix
> > for the issue in the first patch.
> 
> Feel free to send a pull request, I don't have anything conflicting queued for 
> v4.12.

Ack. I'll send a pull request then.

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

Re: [PATCH 1/6] omap3isp: Don't rely on devm for memory resource management

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Monday 20 Feb 2017 17:22:17 Sakari Ailus wrote:
> devm functions are fine for managing resources that are directly related
> to the device at hand and that have no other dependencies. However, a
> process holding a file handle to a device created by a driver for a device
> may result in the file handle left behind after the device is long gone.
> This will result in accessing released (and potentially reallocated)
> memory.
> 
> Instead, manage the memory resources in the driver. Releasing the
> resources can be later on bound to e.g. by releasing a reference.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/omap3isp/isp.c         | 18 ++++++++++++------
>  drivers/media/platform/omap3isp/isph3a_aewb.c | 24 +++++++++++++++++-------
> drivers/media/platform/omap3isp/isph3a_af.c   | 24 +++++++++++++++++-------
> drivers/media/platform/omap3isp/isphist.c     | 11 +++++++----
>  drivers/media/platform/omap3isp/ispstat.c     |  2 ++
>  5 files changed, 55 insertions(+), 24 deletions(-)

[snip]

> diff --git a/drivers/media/platform/omap3isp/isphist.c
> b/drivers/media/platform/omap3isp/isphist.c index a4ed5d1..9e448c6 100644
> --- a/drivers/media/platform/omap3isp/isphist.c
> +++ b/drivers/media/platform/omap3isp/isphist.c
> @@ -476,9 +476,9 @@ int omap3isp_hist_init(struct isp_device *isp)
>  {
>  	struct ispstat *hist = &isp->isp_hist;
>  	struct omap3isp_hist_config *hist_cfg;
> -	int ret = -1;
> +	int ret;
> 
> -	hist_cfg = devm_kzalloc(isp->dev, sizeof(*hist_cfg), GFP_KERNEL);
> +	hist_cfg = kzalloc(sizeof(*hist_cfg), GFP_KERNEL);
>  	if (hist_cfg == NULL)
>  		return -ENOMEM;
> 
> @@ -500,7 +500,7 @@ int omap3isp_hist_init(struct isp_device *isp)
>  		if (IS_ERR(hist->dma_ch)) {
>  			ret = PTR_ERR(hist->dma_ch);
>  			if (ret == -EPROBE_DEFER)
> -				return ret;
> +				goto err;
> 
>  			hist->dma_ch = NULL;
>  			dev_warn(isp->dev,
> @@ -516,9 +516,12 @@ int omap3isp_hist_init(struct isp_device *isp)
>  	hist->event_type = V4L2_EVENT_OMAP3ISP_HIST;
> 
>  	ret = omap3isp_stat_init(hist, "histogram", &hist_subdev_ops);
> +
> +err:
>  	if (ret) {
> -		if (hist->dma_ch)
> +		if (!IS_ERR(hist->dma_ch))

I think this change is wrong. dma_ch is initialize to NULL by kzalloc(). You 
will end up calling dma_release_channel() on a NULL channel if 
omap3isp_stat_init() fails and HIST_CONFIG_DMA is false. The check should be

	if (!IS_ERR_OR_NULL(hist->dma_ch))

Apart from that,

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

>  			dma_release_channel(hist->dma_ch);
> +		kfree(hist_cfg);
>  	}
> 
>  	return ret;

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] omap3isp: Don't rely on devm for memory resource management

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Monday 20 Feb 2017 17:22:17 Sakari Ailus wrote:
...
>> @@ -516,9 +516,12 @@ int omap3isp_hist_init(struct isp_device *isp)
>>  	hist->event_type = V4L2_EVENT_OMAP3ISP_HIST;
>>
>>  	ret = omap3isp_stat_init(hist, "histogram", &hist_subdev_ops);
>> +
>> +err:
>>  	if (ret) {
>> -		if (hist->dma_ch)
>> +		if (!IS_ERR(hist->dma_ch))
>
> I think this change is wrong. dma_ch is initialize to NULL by kzalloc(). You
> will end up calling dma_release_channel() on a NULL channel if
> omap3isp_stat_init() fails and HIST_CONFIG_DMA is false. The check should be
>
> 	if (!IS_ERR_OR_NULL(hist->dma_ch))

Good catch! I'll fix that.

>
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!


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

Re: [PATCH 2/6] omap3isp: Call video_unregister_device() unconditionally

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Monday 20 Feb 2017 17:22:18 Sakari Ailus wrote:
> video_unregister_device() can be called on a never or an already
> unregistered device. Drop the redundant check.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

> ---
>  drivers/media/platform/omap3isp/ispvideo.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index 218e6d7..9e9b18c 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1495,6 +1495,5 @@ int omap3isp_video_register(struct isp_video *video,
> struct v4l2_device *vdev)
> 
>  void omap3isp_video_unregister(struct isp_video *video)
>  {
> -	if (video_is_registered(&video->video))
> -		video_unregister_device(&video->video);
> +	video_unregister_device(&video->video);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] omap3isp: Remove misleading comment

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Monday 20 Feb 2017 17:22:19 Sakari Ailus wrote:
> The intent of the check the comment is related to is to ensure we are
> streaming --- still. Not that streaming wouldn't be enabled yet. Remove
> it.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/omap3isp/ispvideo.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index 9e9b18c..a3ca2a4 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1205,7 +1205,6 @@ isp_video_streamoff(struct file *file, void *fh, enum
> v4l2_buf_type type)
> 
>  	mutex_lock(&video->stream_lock);
> 
> -	/* Make sure we're not streaming yet. */
>  	mutex_lock(&video->queue_lock);
>  	streaming = vb2_is_streaming(&vfh->queue);
>  	mutex_unlock(&video->queue_lock);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] media: Remove useless curly braces and parentheses

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Monday 20 Feb 2017 17:22:21 Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

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

> ---
>  drivers/media/media-device.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 760e3e4..c51e2e5 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -591,9 +591,8 @@ int __must_check media_device_register_entity(struct
> media_device *mdev, &entity->pads[i].graph_obj);
> 
>  	/* invoke entity_notify callbacks */
> -	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
> -		(notify)->notify(entity, notify->notify_data);
> -	}
> +	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
> +		notify->notify(entity, notify->notify_data);
> 
>  	if (mdev->entity_internal_idx_max
> 
>  	    >= mdev->pm_count_walk.ent_enum.idx_max) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/6] media: devnode: Rename mdev argument as devnode

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Monday 20 Feb 2017 17:22:22 Sakari Ailus wrote:
> Historically, mdev argument name was being used on both struct
> media_device and struct media_devnode. Recently most occurrences of mdev
> referring to struct media_devnode were replaced by devnode, which makes
> more sense. Fix the last remaining occurrence.
> 
> Fixes: 163f1e93e9950 ("[media] media-devnode: fix namespace mess")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

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

> ---
>  drivers/media/media-device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c51e2e5..fce91b5 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -537,9 +537,9 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
>   * Registration/unregistration
>   */
> 
> -static void media_device_release(struct media_devnode *mdev)
> +static void media_device_release(struct media_devnode *devnode)
>  {
> -	dev_dbg(mdev->parent, "Media device released\n");
> +	dev_dbg(devnode->parent, "Media device released\n");
>  }
> 
>  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] omap3isp: Disable streaming at driver unbind time

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Monday 20 Feb 2017 17:22:20 Sakari Ailus wrote:
> Once the driver is unbound accessing the hardware is not allowed anymore.
> Due to this, disable streaming when the device driver is unbound. The
> states of the associated objects related to Media controller and videobuf2
> frameworks are updated as well, just like if the application disabled
> streaming explicitly.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

This looks mostly good to me, although I'm a bit concerned about race 
conditions related to buffer handling. I don't think this patch introduces any 
new one though, so

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

We'll have to go through buffer management at some point in the near future, 
including from a V4L2 API point of view I think.

> ---
>  drivers/media/platform/omap3isp/ispvideo.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index a3ca2a4..c04d357 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1191,22 +1191,17 @@ isp_video_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type)
>  }
> 
>  static int
> -isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
> +__isp_video_streamoff(struct isp_video *video)
>  {
> -	struct isp_video_fh *vfh = to_isp_video_fh(fh);
> -	struct isp_video *video = video_drvdata(file);
>  	struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
>  	enum isp_pipeline_state state;
>  	unsigned int streaming;
>  	unsigned long flags;
> 
> -	if (type != video->type)
> -		return -EINVAL;
> -
>  	mutex_lock(&video->stream_lock);
> 
>  	mutex_lock(&video->queue_lock);
> -	streaming = vb2_is_streaming(&vfh->queue);
> +	streaming = video->queue && vb2_is_streaming(video->queue);
>  	mutex_unlock(&video->queue_lock);
> 
>  	if (!streaming)
> @@ -1229,7 +1224,7 @@ isp_video_streamoff(struct file *file, void *fh, enum
> v4l2_buf_type type) omap3isp_video_cancel_stream(video);
> 
>  	mutex_lock(&video->queue_lock);
> -	vb2_streamoff(&vfh->queue, type);
> +	vb2_streamoff(video->queue, video->queue->type);
>  	mutex_unlock(&video->queue_lock);
>  	video->queue = NULL;
>  	video->error = false;
> @@ -1245,6 +1240,17 @@ isp_video_streamoff(struct file *file, void *fh, enum
> v4l2_buf_type type) }
> 
>  static int
> +isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
> +{
> +	struct isp_video *video = video_drvdata(file);
> +
> +	if (type != video->type)
> +		return -EINVAL;
> +
> +	return __isp_video_streamoff(video);
> +}
> +
> +static int
>  isp_video_enum_input(struct file *file, void *fh, struct v4l2_input *input)
> {
>  	if (input->index > 0)
> @@ -1494,5 +1500,6 @@ int omap3isp_video_register(struct isp_video *video,
> struct v4l2_device *vdev)
> 
>  void omap3isp_video_unregister(struct isp_video *video)
>  {
> +	__isp_video_streamoff(video);
>  	video_unregister_device(&video->video);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] omap3isp: Disable streaming at driver unbind time

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Monday 20 Feb 2017 17:22:20 Sakari Ailus wrote:
>> Once the driver is unbound accessing the hardware is not allowed anymore.
>> Due to this, disable streaming when the device driver is unbound. The
>> states of the associated objects related to Media controller and videobuf2
>> frameworks are updated as well, just like if the application disabled
>> streaming explicitly.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> This looks mostly good to me, although I'm a bit concerned about race
> conditions related to buffer handling. I don't think this patch introduces any
> new one though, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> We'll have to go through buffer management at some point in the near future,
> including from a V4L2 API point of view I think.

Thanks for the review!

Are you happy with me sending a pull request on the set, or would you 
prefer to pick the omap3isp patches? In the latter case I'll send a fix 
for the issue in the first patch.

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

Re: [PATCH 4/6] omap3isp: Disable streaming at driver unbind time

[Laurent Pinchart]

Hi Sakari,

On Tuesday 28 Feb 2017 16:00:01 Sakari Ailus wrote:
> > On Monday 20 Feb 2017 17:22:20 Sakari Ailus wrote:
> >> Once the driver is unbound accessing the hardware is not allowed anymore.
> >> Due to this, disable streaming when the device driver is unbound. The
> >> states of the associated objects related to Media controller and
> >> videobuf2 frameworks are updated as well, just like if the application
> >> disabled streaming explicitly.
> >> 
> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > This looks mostly good to me, although I'm a bit concerned about race
> > conditions related to buffer handling. I don't think this patch introduces
> > any new one though, so
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > We'll have to go through buffer management at some point in the near
> > future, including from a V4L2 API point of view I think.
> 
> Thanks for the review!
> 
> Are you happy with me sending a pull request on the set, or would you
> prefer to pick the omap3isp patches? In the latter case I'll send a fix
> for the issue in the first patch.

Feel free to send a pull request, I don't have anything conflicting queued for 
v4.12.

-- 
Regards,

Laurent Pinchart