| Message ID | 1520499719-23955-1-git-send-email-hugues.fruchet@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 >
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; }
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(+)