Error routes through omap3isp ccdc.

Error routes through omap3isp ccdc.

[Jonathan Cameron]

Hi All,

Came across this when my camera driver failed to load..

Following causes a kernel panic.

Set up link between cdcc and ccdc output to memory.
(at this point intitializing a capture gives an invalid
arguement error which is fine.)

Now set the format on ISP CCDC/0 to SGRBG10 752x480 (doubt what these
settings are actually matters).  Actually come to think of it, this
may only be relevant as otherwise, the format of my attempted stream
setup from userspace doesn't matter.

Blithely attempt to grab frames from userspace off the ccdc output.
(note it has no input.)

Null pointer exception occurs because:

pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
in 
static void ccdc_configure(struct isp_ccdc_device *ccdc)

returns null and this is not checked.

Obvious local fix is to check the value of pad and allow ccdc_configure
to return an error + pass this up out of ccdc_set_stream.

Something like the following does the trick.
(not sure error code is right choice!)

Patch is against todays linux-next + a few unconnected things to make
that tree actually build for the beagle xm.

Grep isn't indicating any exactly matching cases to this problem, so
the rest of the tree may be fine.

 [PATCH] omap3isp: check if there is actually a source for ispccdc when
 setting up the link.

Without this if no source actually exists and the ccdc is configured
to output to memory, a read from userspace will cause a null pointer
exception.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/media/video/omap3isp/ispccdc.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
index 6766247..2703a94 100644
--- a/drivers/media/video/omap3isp/ispccdc.c
+++ b/drivers/media/video/omap3isp/ispccdc.c
@@ -1110,7 +1110,7 @@ static const u32 ccdc_sgbrg_pattern =
 	ISPCCDC_COLPTN_R_Ye  << ISPCCDC_COLPTN_CP3PLC2_SHIFT |
 	ISPCCDC_COLPTN_Gr_Cy << ISPCCDC_COLPTN_CP3PLC3_SHIFT;
 
-static void ccdc_configure(struct isp_ccdc_device *ccdc)
+static int ccdc_configure(struct isp_ccdc_device *ccdc)
 {
 	struct isp_device *isp = to_isp_device(ccdc);
 	struct isp_parallel_platform_data *pdata = NULL;
@@ -1127,6 +1127,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	u32 ccdc_pattern;
 
 	pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
+	if (pad == NULL)
+		return -ENODEV;
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	if (ccdc->input == CCDC_INPUT_PARALLEL)
 		pdata = &((struct isp_v4l2_subdevs_group *)sensor->host_priv)
@@ -1257,6 +1259,7 @@ unlock:
 	spin_unlock_irqrestore(&ccdc->lsc.req_lock, flags);
 
 	ccdc_apply_controls(ccdc);
+	return 0;
 }
 
 static void __ccdc_enable(struct isp_ccdc_device *ccdc, int enable)
@@ -1726,7 +1729,9 @@ static int ccdc_set_stream(struct v4l2_subdev *sd, int enable)
 		isp_reg_set(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_CFG,
 			    ISPCCDC_CFG_VDLC);
 
-		ccdc_configure(ccdc);
+		ret = ccdc_configure(ccdc);
+		if (ret < 0)
+			return ret;
 
 		/* TODO: Don't configure the video port if all of its output
 		 * links are inactive.
-- 
1.7.3.4

Re: Error routes through omap3isp ccdc.

[Laurent Pinchart]

Hi Jonathan,

On Monday 11 July 2011 12:41:49 Jonathan Cameron wrote:
> Hi All,
> 
> Came across this when my camera driver failed to load..
> 
> Following causes a kernel panic.
> 
> Set up link between cdcc and ccdc output to memory.
> (at this point intitializing a capture gives an invalid
> arguement error which is fine.)
> 
> Now set the format on ISP CCDC/0 to SGRBG10 752x480 (doubt what these
> settings are actually matters).  Actually come to think of it, this
> may only be relevant as otherwise, the format of my attempted stream
> setup from userspace doesn't matter.
> 
> Blithely attempt to grab frames from userspace off the ccdc output.
> (note it has no input.)
> 
> Null pointer exception occurs because:
> 
> pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
> in
> static void ccdc_configure(struct isp_ccdc_device *ccdc)
> 
> returns null and this is not checked.

Good catch, thanks.

> Obvious local fix is to check the value of pad and allow ccdc_configure
> to return an error + pass this up out of ccdc_set_stream.
> 
> Something like the following does the trick.
> (not sure error code is right choice!)
> 
> Patch is against todays linux-next + a few unconnected things to make
> that tree actually build for the beagle xm.
> 
> Grep isn't indicating any exactly matching cases to this problem, so
> the rest of the tree may be fine.
> 
>  [PATCH] omap3isp: check if there is actually a source for ispccdc when
>  setting up the link.
> 
> Without this if no source actually exists and the ccdc is configured
> to output to memory, a read from userspace will cause a null pointer
> exception.

Thanks for the patch.

I think we should try to fix it in ispvideo.c instead. You could add a check 
to isp_video_validate_pipeline() to make sure that the pipeline has a video 
source.

> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/media/video/omap3isp/ispccdc.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/ispccdc.c
> b/drivers/media/video/omap3isp/ispccdc.c index 6766247..2703a94 100644
> --- a/drivers/media/video/omap3isp/ispccdc.c
> +++ b/drivers/media/video/omap3isp/ispccdc.c
> @@ -1110,7 +1110,7 @@ static const u32 ccdc_sgbrg_pattern =
>  	ISPCCDC_COLPTN_R_Ye  << ISPCCDC_COLPTN_CP3PLC2_SHIFT |
>  	ISPCCDC_COLPTN_Gr_Cy << ISPCCDC_COLPTN_CP3PLC3_SHIFT;
> 
> -static void ccdc_configure(struct isp_ccdc_device *ccdc)
> +static int ccdc_configure(struct isp_ccdc_device *ccdc)
>  {
>  	struct isp_device *isp = to_isp_device(ccdc);
>  	struct isp_parallel_platform_data *pdata = NULL;
> @@ -1127,6 +1127,8 @@ static void ccdc_configure(struct isp_ccdc_device
> *ccdc) u32 ccdc_pattern;
> 
>  	pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
> +	if (pad == NULL)
> +		return -ENODEV;
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
>  	if (ccdc->input == CCDC_INPUT_PARALLEL)
>  		pdata = &((struct isp_v4l2_subdevs_group *)sensor->host_priv)
> @@ -1257,6 +1259,7 @@ unlock:
>  	spin_unlock_irqrestore(&ccdc->lsc.req_lock, flags);
> 
>  	ccdc_apply_controls(ccdc);
> +	return 0;
>  }
> 
>  static void __ccdc_enable(struct isp_ccdc_device *ccdc, int enable)
> @@ -1726,7 +1729,9 @@ static int ccdc_set_stream(struct v4l2_subdev *sd,
> int enable) isp_reg_set(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_CFG,
>  			    ISPCCDC_CFG_VDLC);
> 
> -		ccdc_configure(ccdc);
> +		ret = ccdc_configure(ccdc);
> +		if (ret < 0)
> +			return ret;
> 
>  		/* TODO: Don't configure the video port if all of its output
>  		 * links are inactive.

-- 
Regards,

Laurent Pinchart

Re: Error routes through omap3isp ccdc.

[Laurent Pinchart]

On Monday 11 July 2011 12:54:42 Laurent Pinchart wrote:
> On Monday 11 July 2011 12:41:49 Jonathan Cameron wrote:

[snip]

> I think we should try to fix it in ispvideo.c instead. You could add a
> check to isp_video_validate_pipeline() to make sure that the pipeline has
> a video source.

And I forgot to mention, I can send a patch if you don't want to write it.

-- 
Regards,

Laurent Pinchart

Re: Error routes through omap3isp ccdc.

[Jonathan Cameron]

On 07/11/11 11:57, Laurent Pinchart wrote:
> On Monday 11 July 2011 12:54:42 Laurent Pinchart wrote:
>> On Monday 11 July 2011 12:41:49 Jonathan Cameron wrote:
> 
> [snip]
> 
>> I think we should try to fix it in ispvideo.c instead. You could add a
>> check to isp_video_validate_pipeline() to make sure that the pipeline has
>> a video source.
> 
> And I forgot to mention, I can send a patch if you don't want to write it.
> 
Given I can't quite see why the validate_pipeline code would ever want to break
on source pad being null (which I think is what it is currently doing),
I'll leave it to you.  Really don't know this code well enough!

Thanks.

Jonathan

Re: Error routes through omap3isp ccdc.

[Laurent Pinchart]

Hi Jonathan,

On Monday 11 July 2011 13:16:32 Jonathan Cameron wrote:
> On 07/11/11 11:57, Laurent Pinchart wrote:
> > On Monday 11 July 2011 12:54:42 Laurent Pinchart wrote:
> >> On Monday 11 July 2011 12:41:49 Jonathan Cameron wrote:
> > [snip]
> > 
> >> I think we should try to fix it in ispvideo.c instead. You could add a
> >> check to isp_video_validate_pipeline() to make sure that the pipeline
> >> has a video source.
> > 
> > And I forgot to mention, I can send a patch if you don't want to write
> > it.
> 
> Given I can't quite see why the validate_pipeline code would ever want to
> break on source pad being null (which I think is what it is currently
> doing), I'll leave it to you.  Really don't know this code well enough!

Could you please test the following patch ?

>From 578d0b64a25177290815f974fb7898e32587b450 Mon Sep 17 00:00:00 2001
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date: Sun, 31 Jul 2011 17:12:02 +0200
Subject: [PATCH] omap3isp: Don't accept pipelines with no video source as valid

Make sure the pipeline has a valid video source (either a subdev with no
sink pad, or a non-subdev entity) at stream-on time and return -EPIPE if
no video source can be found.

Reported-by: Jonathan Cameron <jic23@cam.ac.uk>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/ispvideo.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispvideo.c b/drivers/media/video/omap3isp/ispvideo.c
index fd965ad..d3ddfc0 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -278,7 +278,8 @@ isp_video_far_end(struct isp_video *video)
  * limits reported by every block in the pipeline.
  *
  * Return 0 if all formats match, or -EPIPE if at least one link is found with
- * different formats on its two ends.
+ * different formats on its two ends or if the pipeline doesn't start with a
+ * video source (either a subdev with no input pad, or a non-subdev entity).
  */
 static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 {
@@ -329,10 +330,15 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 		 * in the middle of it. */
 		shifter_link = subdev == &isp->isp_ccdc.subdev;
 
-		/* Retrieve the source format */
+		/* Retrieve the source format. Return an error if no source
+		 * entity can be found, and stop checking the pipeline if the
+		 * source entity isn't a subdev.
+		 */
 		pad = media_entity_remote_source(pad);
-		if (pad == NULL ||
-		    media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
+		if (pad == NULL
+			return -EPIPE;
+
+		if (media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
 			break;
 
 		subdev = media_entity_to_v4l2_subdev(pad->entity);
-- 
Regards,

Laurent Pinchart