[PATCH] media: ov5640: fix frame interval enumeration

[PATCH] media: ov5640: fix frame interval enumeration

[Hugues Fruchet]

Driver must reject frame interval enumeration of unsupported resolution.
This was detected by v4l2-compliance format ioctl test:
v4l2-compliance Format ioctls:
  info: found 2 frameintervals for pixel format 4745504a and size 176x144
  fail: v4l2-test-formats.cpp(123):
                           found frame intervals for invalid size 177x144
  test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: FAIL

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov5640.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 676f635..28dc687 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1400,6 +1400,9 @@ static int ov5640_set_virtual_channel(struct ov5640_dev *sensor)
 	if (nearest && i < 0)
 		mode = &ov5640_mode_data[fr][0];
 
+	if (!nearest && i < 0)
+		return NULL;
+
 	return mode;
 }
 
-- 
1.9.1

Re: [PATCH] media: ov5640: fix frame interval enumeration

[Mauro Carvalho Chehab]

Em Thu, 8 Mar 2018 10:01:59 +0100
Hugues Fruchet <hugues.fruchet@st.com> escreveu:

> Driver must reject frame interval enumeration of unsupported resolution.
> This was detected by v4l2-compliance format ioctl test:
> v4l2-compliance Format ioctls:
>   info: found 2 frameintervals for pixel format 4745504a and size 176x144
>   fail: v4l2-test-formats.cpp(123):
>                            found frame intervals for invalid size 177x144
>   test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: FAIL
> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  drivers/media/i2c/ov5640.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 676f635..28dc687 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1400,6 +1400,9 @@ static int ov5640_set_virtual_channel(struct ov5640_dev *sensor)
>  	if (nearest && i < 0)
>  		mode = &ov5640_mode_data[fr][0];
>  
> +	if (!nearest && i < 0)
> +		return NULL;
> +
>  	return mode;
>  }


I didn't check the full logic here, nor if this patch makes sense, but
just looking at the above code, it looks ugly to fill "mode" var just
to discard it. I would code the above as:

	if (i < 0) {
		if (!nearest)
			return NULL;
		mode = &ov5640_mode_data[fr][0];
	}


Also, if this function starts returning NULL, I suspect that you also
need to change ov5640_s_frame_interval(), as currently it is called
at ov5640_s_frame_interval() as:

	sensor->current_mode = ov5640_find_mode(sensor, frame_rate, mode->width,
						mode->height, true);

without checking if the returned value is NULL. Setting
current_mode to NULL can cause oopses at ov5640_set_mode().

Regards,
Mauro

Re: [PATCH] media: ov5640: fix frame interval enumeration

[Mauro Carvalho Chehab]

Em Thu, 8 Mar 2018 07:39:09 -0300
Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:

> Also, if this function starts returning NULL, I suspect that you also
> need to change ov5640_s_frame_interval(), as currently it is called
> at ov5640_s_frame_interval() as:
> 
> 	sensor->current_mode = ov5640_find_mode(sensor, frame_rate, mode->width,
> 						mode->height, true);
> 
> without checking if the returned value is NULL. Setting
> current_mode to NULL can cause oopses at ov5640_set_mode().

Actually, as ov5640_s_frame_interval() calls ov5640_try_fmt_internal()
first. So, this should never happen.


Thanks,
Mauro

Re: [PATCH] media: ov5640: fix frame interval enumeration

[Hugues FRUCHET]

Hi Mauro,

Thanks for review, I've just sent a v2 to rearrange code as per your 
suggestion and also add a NULL test case for mode even if this should 
not happen.

Best regards,
Hugues.

On 03/08/2018 11:46 AM, Mauro Carvalho Chehab wrote:
> Em Thu, 8 Mar 2018 07:39:09 -0300
> Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:
> 
>> Also, if this function starts returning NULL, I suspect that you also
>> need to change ov5640_s_frame_interval(), as currently it is called
>> at ov5640_s_frame_interval() as:
>>
>> 	sensor->current_mode = ov5640_find_mode(sensor, frame_rate, mode->width,
>> 						mode->height, true);
>>
>> without checking if the returned value is NULL. Setting
>> current_mode to NULL can cause oopses at ov5640_set_mode().
> 
> Actually, as ov5640_s_frame_interval() calls ov5640_try_fmt_internal()
> first. So, this should never happen.
> 
> 
> Thanks,
> Mauro
>