Hi Alexander and Dorota,
Thank you for the patch.
On Fri, Feb 04, 2022 at 01:15:10PM +0100, Alexander Stein wrote:
> From: Dorota Czaplejewicz <[email protected]>
>
> imx_media_find_mbus_format has NULL as a valid return value,
> therefore the caller should take it into account.
>
> Signed-off-by: Dorota Czaplejewicz <[email protected]>
> Signed-off-by: Alexander Stein <[email protected]>
> ---
> drivers/staging/media/imx/imx-media-utils.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 32aaa2e81bea..e0a256a08c3b 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -544,6 +544,9 @@ static int imx56_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> cc = imx_media_find_mbus_format(code, PIXFMT_SEL_YUV);
The code passed to the function comes from the previous line:
imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
As far as I can tell, this is guaranteed to return a code that will
result in imx_media_find_mbus_format() returning a non-NULL pointer.
> }
>
> + if (!cc)
> + return -EINVAL;
> +
> /* Round up width for minimum burst size */
> width = round_up(mbus->width, 8);
>
--
Regards,
Laurent Pinchart
Hi Laurent,
On Sat, 5 Feb 2022 06:07:37 +0200
Laurent Pinchart <[email protected]> wrote:
> Hi Alexander and Dorota,
>
> Thank you for the patch.
>
> On Fri, Feb 04, 2022 at 01:15:10PM +0100, Alexander Stein wrote:
> > From: Dorota Czaplejewicz <[email protected]>
> >
> > imx_media_find_mbus_format has NULL as a valid return value,
> > therefore the caller should take it into account.
> >
> > Signed-off-by: Dorota Czaplejewicz <[email protected]>
> > Signed-off-by: Alexander Stein <[email protected]>
> > ---
> > drivers/staging/media/imx/imx-media-utils.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> > index 32aaa2e81bea..e0a256a08c3b 100644
> > --- a/drivers/staging/media/imx/imx-media-utils.c
> > +++ b/drivers/staging/media/imx/imx-media-utils.c
> > @@ -544,6 +544,9 @@ static int imx56_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> > cc = imx_media_find_mbus_format(code, PIXFMT_SEL_YUV);
>
> The code passed to the function comes from the previous line:
>
> imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
>
> As far as I can tell, this is guaranteed to return a code that will
> result in imx_media_find_mbus_format() returning a non-NULL pointer.
>
While I am not well-versed in the implicit code style of the kernel, I decided to leave it in because it makes the code more locally legible. With a check here, even a non-functional one, there's no need to understand the internals of `imx_media_find_mbus_format` that are only implicit. That makes the code less surprising when interested only in the outer function.
The other advantage of a check is becoming robust against future changes to `imx_media_find_mbus_format` itself.
I don't have a strong preference about keeping or leaving this patch, but if this check was there in the first place, I wouldn't have spent time trying to figure out whether there's a bug here.
Cheers,
Dorota
> > }
> >
> > + if (!cc)
> > + return -EINVAL;
> > +
> > /* Round up width for minimum burst size */
> > width = round_up(mbus->width, 8);
> >
>
Hi Laurent and Dorota,
Am Samstag, 5. Februar 2022, 08:51:51 CET schrieb Dorota Czaplejewicz:
> * PGP Signed by an unknown key
>
> Hi Laurent,
>
> On Sat, 5 Feb 2022 06:07:37 +0200
>
> Laurent Pinchart <[email protected]> wrote:
> > Hi Alexander and Dorota,
> >
> > Thank you for the patch.
> >
> > On Fri, Feb 04, 2022 at 01:15:10PM +0100, Alexander Stein wrote:
> > > From: Dorota Czaplejewicz <[email protected]>
> > >
> > > imx_media_find_mbus_format has NULL as a valid return value,
> > > therefore the caller should take it into account.
> > >
> > > Signed-off-by: Dorota Czaplejewicz <[email protected]>
> > > Signed-off-by: Alexander Stein <[email protected]>
> > > ---
> > >
> > > drivers/staging/media/imx/imx-media-utils.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/staging/media/imx/imx-media-utils.c
> > > b/drivers/staging/media/imx/imx-media-utils.c index
> > > 32aaa2e81bea..e0a256a08c3b 100644
> > > --- a/drivers/staging/media/imx/imx-media-utils.c
> > > +++ b/drivers/staging/media/imx/imx-media-utils.c
> > > @@ -544,6 +544,9 @@ static int imx56_media_mbus_fmt_to_pix_fmt(struct
> > > v4l2_pix_format *pix,> >
> > > cc = imx_media_find_mbus_format(code, PIXFMT_SEL_YUV);
> >
> > The code passed to the function comes from the previous line:
> > imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
> >
> > As far as I can tell, this is guaranteed to return a code that will
> > result in imx_media_find_mbus_format() returning a non-NULL pointer.
>
> While I am not well-versed in the implicit code style of the kernel, I
> decided to leave it in because it makes the code more locally legible. With
> a check here, even a non-functional one, there's no need to understand the
> internals of `imx_media_find_mbus_format` that are only implicit. That
> makes the code less surprising when interested only in the outer function.
>
> The other advantage of a check is becoming robust against future changes to
> `imx_media_find_mbus_format` itself.
>
> I don't have a strong preference about keeping or leaving this patch, but if
> this check was there in the first place, I wouldn't have spent time trying
> to figure out whether there's a bug here.
Laurent, thanks for your feedback.
I'm on Dorota's side here. While you are right that the code from
imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
is guaranteed to return something. I don't lnek this implicit assumption over
2 function calls. Better be safe than sorry. For that reason code should be
initialized as well. Will create a patch for that.
Regards,
Alexander
Hello,
On Mon, Feb 07, 2022 at 10:52:00AM +0100, Alexander Stein wrote:
> Am Samstag, 5. Februar 2022, 08:51:51 CET schrieb Dorota Czaplejewicz:
> > On Sat, 5 Feb 2022 06:07:37 +0200 Laurent Pinchart wrote:
> > > On Fri, Feb 04, 2022 at 01:15:10PM +0100, Alexander Stein wrote:
> > > > From: Dorota Czaplejewicz <[email protected]>
> > > >
> > > > imx_media_find_mbus_format has NULL as a valid return value,
> > > > therefore the caller should take it into account.
> > > >
> > > > Signed-off-by: Dorota Czaplejewicz <[email protected]>
> > > > Signed-off-by: Alexander Stein <[email protected]>
> > > > ---
> > > >
> > > > drivers/staging/media/imx/imx-media-utils.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/staging/media/imx/imx-media-utils.c
> > > > b/drivers/staging/media/imx/imx-media-utils.c index
> > > > 32aaa2e81bea..e0a256a08c3b 100644
> > > > --- a/drivers/staging/media/imx/imx-media-utils.c
> > > > +++ b/drivers/staging/media/imx/imx-media-utils.c
> > > > @@ -544,6 +544,9 @@ static int imx56_media_mbus_fmt_to_pix_fmt(struct
> > > > v4l2_pix_format *pix,> >
> > > > cc = imx_media_find_mbus_format(code, PIXFMT_SEL_YUV);
> > >
> > > The code passed to the function comes from the previous line:
> > > imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
> > >
> > > As far as I can tell, this is guaranteed to return a code that will
> > > result in imx_media_find_mbus_format() returning a non-NULL pointer.
> >
> > While I am not well-versed in the implicit code style of the kernel, I
> > decided to leave it in because it makes the code more locally legible. With
> > a check here, even a non-functional one, there's no need to understand the
> > internals of `imx_media_find_mbus_format` that are only implicit. That
> > makes the code less surprising when interested only in the outer function.
> >
> > The other advantage of a check is becoming robust against future changes to
> > `imx_media_find_mbus_format` itself.
> >
> > I don't have a strong preference about keeping or leaving this patch, but if
> > this check was there in the first place, I wouldn't have spent time trying
> > to figure out whether there's a bug here.
>
> Laurent, thanks for your feedback.
> I'm on Dorota's side here. While you are right that the code from
> imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
> is guaranteed to return something. I don't lnek this implicit assumption over
> 2 function calls. Better be safe than sorry. For that reason code should be
> initialized as well. Will create a patch for that.
I'm fine with hardening the code even if the issue can't occur at the
moment.
--
Regards,
Laurent Pinchart