media: platform: nxp: fix return value check in mipi_csis_s_stream()

Commit Message

Andy Chen Feb. 7, 2024, 3:36 p.m. UTC

Hi,

find_csis_format() may return NULL, and we should determine its return value
to prevent a fatal error when later functions use it.


This is my first patch, any suggestions are welcome, thanks!

Regards,

Andy Chen

Comments

Hans Verkuil Feb. 12, 2024, 3:17 p.m. UTC | #1

Hi Andy,

On 07/02/2024 16:36, Andy Chen wrote:
> Hi,
> 
> find_csis_format() may return NULL, and we should determine its return value
> to prevent a fatal error when later functions use it.
> 
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index db8ff5f5c4d3..ac867620e2ba 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -956,6 +956,8 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  
>  	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
>  	csis_fmt = find_csis_format(format->code);
> +	if (!csis_fmt)
> +		return -EPIPE;
>  
>  	ret = mipi_csis_calculate_params(csis, csis_fmt);
>  	if (ret < 0)
> 
> This is my first patch, any suggestions are welcome, thanks!

This is missing your Signed-off-by line. Much more information on how
to submit patches is given here:

https://docs.kernel.org/process/submitting-patches.html

But typically patches are formatted like this:

https://patchwork.linuxtv.org/project/linux-media/patch/[e-mail]/

One question for this patch: have you observed that find_csis_format() can
return NULL, or do you just think that can happen?

Regards,

	Hans

Jacopo Mondi Feb. 12, 2024, 3:32 p.m. UTC | #2

Hi Andy,

  thanks for the patch

On Wed, Feb 07, 2024 at 11:36:57PM +0800, Andy Chen wrote:
> Hi,
>
> find_csis_format() may return NULL, and we should determine its return value
> to prevent a fatal error when later functions use it.
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index db8ff5f5c4d3..ac867620e2ba 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -956,6 +956,8 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>
>  	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
>  	csis_fmt = find_csis_format(format->code);
> +	if (!csis_fmt)
> +		return -EPIPE;
>

This shouldn't be required afaict

'format' comes from the subdev state on the CSIS_PAD_SINK:

 	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);

If you check the mipi_csis_set_fmt() implementation, the driver first
fix-ups any non-valid format supplied by the user:

	csis_fmt = find_csis_format(sdformat->format.code);
	if (!csis_fmt)
		csis_fmt = &mipi_csis_formats[0];

And then stores it in the subdev state (on the same CSIS_PAD_SINK pad)

	fmt = v4l2_subdev_state_get_format(sd_state, sdformat->pad);

	fmt->code = csis_fmt->code;

from where it gets retrieved at s_stream() time.

This guarantees that at s_stream time, the format is valid if I'm not
mistaken.

As Hans has already asked: did you see this failing or was this patch
"just in case" ?

Thanks
  j

>  	ret = mipi_csis_calculate_params(csis, csis_fmt);
>  	if (ret < 0)
>
> This is my first patch, any suggestions are welcome, thanks!
>
> Regards,
>
> Andy Chen
>

Patch

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index db8ff5f5c4d3..ac867620e2ba 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -956,6 +956,8 @@  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 
 	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
 	csis_fmt = find_csis_format(format->code);
+	if (!csis_fmt)
+		return -EPIPE;
 
 	ret = mipi_csis_calculate_params(csis, csis_fmt);
 	if (ret < 0)