Re: [PATCH v1] media: sun8i-a83t-mipi-csi2: Use V4L2 subdev active state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Uninitialized `state` pointer and flawed error handling in `sun8i_a83t_mipi_csi2_s_stream` lead to guaranteed kernel crashes.
--

commit 0ebc430b2a12334ca2b95edeadb9cb2ca2b1588b
Author: Arash Golgol <arash.golgol@gmail.com>

media: sun8i-a83t-mipi-csi2: Use V4L2 subdev active state

This commit transitions the sun8i-a83t-mipi-csi2 driver to use the V4L2
subdev active state API. It stores the active format using this state API
and drops the bridge lock and mbus_format field.

> diff --git a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c
> index dbc51daa4fe37..2383b0cd1a127 100644
> --- a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c
> +++ b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c

[ ... ]

> @@ -205,7 +205,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
>  	struct v4l2_subdev *source_subdev = csi2_dev->bridge.source_subdev;
>  	union phy_configure_opts dphy_opts = { 0 };
>  	struct phy_configure_opts_mipi_dphy *dphy_cfg = &dphy_opts.mipi_dphy;
> -	struct v4l2_mbus_framefmt *mbus_format = &csi2_dev->bridge.mbus_format;
> +	struct v4l2_subdev_state *state;
> +	const struct v4l2_mbus_framefmt *mbus_format;
>  	const struct sun8i_a83t_mipi_csi2_format *format;
>  	struct phy *dphy = csi2_dev->dphy;
>  	struct device *dev = csi2_dev->dev;
> @@ -215,8 +216,10 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
>  	unsigned long pixel_rate;
>  	int ret;
>  
> -	if (!source_subdev)
> -		return -ENODEV;
> +	if (!source_subdev) {
> +		ret = -ENODEV;
> +		goto unlock;
> +	}

Is the state pointer initialized before we jump to the unlock label here?
It looks like state is declared but never assigned before we attempt to
unlock it.

[ ... ]

> @@ -254,6 +257,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
>  		goto error_pm;
>  	}
>  
> +	mbus_format = v4l2_subdev_state_get_format(
> +		state, SUN8I_A83T_MIPI_CSI2_PAD_SINK);

Are we passing an uninitialized state pointer to
v4l2_subdev_state_get_format() here? It doesn't appear that
v4l2_subdev_lock_and_get_active_state() was called anywhere earlier in this
function.

If state were to be locked at the top of the function, there is an error
path during pm_runtime_resume_and_get() that looks like:

	ret = pm_runtime_resume_and_get(dev);
	if (ret < 0)
		return ret;

Would this direct return bypass the unlock label and permanently leak the
subdev state lock?

[ ... ]

> @@ -318,6 +324,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
>  error_pm:
>  	pm_runtime_put(dev);
>  
> +unlock:
> +	v4l2_subdev_unlock_state(state);
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514113804.12596-1-arash.golgol@gmail.com?part=1