Re: [PATCH v4 2/2] media: i2c: add Aptina MT9M113 1.3 Mpx SoC sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Stream-start retry loop fails to power-cycle the sensor due to PM device links.
- [High] Use-after-free and locking freed memory during concurrent ioctls due to missing devnode synchronization in `remove()`
- [High] mt9m113_stream_context_a ignores the SEQ_CMD_RUN timeout, defeating the stream-start retry mechanism.
- [Medium] State Desynchronization on V4L2_CID_TEST_PATTERN failure
- [Low] Missing Kconfig Dependency for V4L2_FWNODE
--

commit 2bce75457f74983c7027490425c51795edae1c92
Author: Herman van Hazendonk <github.com@herrie.org>

media: i2c: add Aptina MT9M113 1.3 Mpx SoC sensor driver

Add a V4L2 subdev driver for the Aptina MT9M113 1.3 Megapixel SoC
image sensor with on-chip ISP. The sensor exposes dual context operation,
single-lane MIPI CSI-2 output, and implements a stream-start retry
mechanism to recover the MCU.

> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
[ ... ]
> @@ -302,6 +302,18 @@ config VIDEO_MT9M111
>  	  This driver supports MT9M111, MT9M112 and MT9M131 cameras from
>  	  Micron/Aptina
>  
> +config VIDEO_MT9M113
> +	tristate "Aptina MT9M113 sensor support"
> +	select V4L2_CCI_I2C

[Severity: Low]
Is this missing a select for V4L2_FWNODE? The driver calls
v4l2_fwnode_endpoint_alloc_parse() and v4l2_fwnode_endpoint_free() during
probe and remove. If the driver is enabled but V4L2_FWNODE is disabled,
can this cause a build failure with undefined references?

> diff --git a/drivers/media/i2c/mt9m113.c b/drivers/media/i2c/mt9m113.c
> --- /dev/null
> +++ b/drivers/media/i2c/mt9m113.c
[ ... ]
> +static int mt9m113_stream_context_a(struct mt9m113 *sensor, u16 output_ctrl_val,
> +				    u32 code)
> +{
[ ... ]
> +	dev_dbg(dev, "MT9M113: Writing SEQ_CMD_RUN\n");
> +	ret = mt9m113_write_mcu_var(sensor, MT9M113_SEQ_CMD, MT9M113_SEQ_CMD_RUN);
> +	if (ret)
> +		return ret;
> +
> +	ret = mt9m113_poll_mcu_var(sensor, MT9M113_SEQ_CMD, 0x0000, 500);
> +	if (ret < 0)
> +		dev_warn(dev, "MT9M113: SEQ_CMD_RUN did not complete\n");
> +
> +	mt9m113_reassert_output(sensor, output_ctrl_val,
> +				MT9M113_MODE_OUTPUT_FORMAT_A, code);
> +	msleep(20);
> +	return 0;
> +}

[Severity: High]
Does this code silently ignore the failure if mt9m113_poll_mcu_var() times
out? By unconditionally returning 0, the error code in ret is discarded.
Could this falsely report success back to mt9m113_start_streaming() and
completely bypass the retry mechanism for Context A, leaving the MCU
wedged?

[ ... ]
> +static int mt9m113_start_streaming(struct mt9m113 *sensor,
> +				   struct v4l2_subdev_state *state)
> +{
> +	struct device *dev = &sensor->client->dev;
> +	unsigned int attempt;
> +	int ret = 0;
[ ... ]
> +	for (attempt = 0; attempt < MT9M113_STREAM_START_RETRIES; attempt++) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = mt9m113_stream_on(sensor, state);
> +		if (!ret) {
> +			sensor->streaming = true;
> +			return 0;
> +		}
> +
> +		pm_runtime_put_sync_suspend(dev);
> +		if (attempt + 1 < MT9M113_STREAM_START_RETRIES)
> +			dev_warn(dev,
> +				 "MT9M113: stream start failed (%d), power-cycle + retry %u/%u\n",
> +				 ret, attempt + 1, MT9M113_STREAM_START_RETRIES - 1);
> +	}

[Severity: High]
Does pm_runtime_put_sync_suspend() successfully suspend the sensor here?

Because V4L2 subdevices have managed device links (DL_FLAG_PM_RUNTIME)
connecting them to the host bridge, the bridge is active while calling
s_stream.

Will the PM core reject the suspend attempt with -EBUSY, preventing the
sensor from actually powering down? It seems the subsequent
pm_runtime_resume_and_get() would immediately return without invoking the
runtime_resume callback, leaving the hardware wedged and defeating the
retry loop.

[ ... ]
> +static int mt9m113_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	case V4L2_CID_TEST_PATTERN:
> +		if (ctrl->val == 0) {
[ ... ]
> +		} else {
> +			/*
> +			 * Enable test pattern mode.
[ ... ]
> +			 */
> +			sensor->test_pattern_active = true;
> +			sensor->test_pattern_value = ctrl->val;
> +			dev_dbg(&sensor->client->dev,
> +				 "MT9M113: Enabling test pattern %d\n", ctrl->val);
> +			if (sensor->streaming)
> +				ret = mt9m113_apply_test_pattern(sensor,
> +								 ctrl->val);
> +		}

[Severity: Medium]
What happens if mt9m113_apply_test_pattern() fails and returns an error?
The internal state (test_pattern_active and test_pattern_value) is already
updated before verifying the hardware write succeeded.

If the I2C write fails, V4L2 will reject the control change, but wouldn't
the driver's internal state remain permanently stuck with test_pattern_active
set to true? It looks like this would cause all subsequent V4L2 controls
to skip the required mt9m113_refresh() call because they mistakenly think
the MCU is halted for a test pattern.

[ ... ]
> +static void mt9m113_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct mt9m113 *sensor = ifp_to_mt9m113(sd);
> +	struct device *dev = &client->dev;
> +
> +	v4l2_async_unregister_subdev(&sensor->ifp.sd);
> +	v4l2_subdev_cleanup(&sensor->ifp.sd);
> +	v4l2_ctrl_handler_free(&sensor->ifp.hdl);
> +	media_entity_cleanup(&sensor->ifp.sd.entity);

[Severity: High]
Is it safe to free the control handlers and rely on devres to free the
sensor structure here without waiting for active ioctls to finish?

The driver registers a devnode for userspace access via
V4L2_SUBDEV_FL_HAS_DEVNODE. While v4l2_async_unregister_subdev() calls
video_unregister_device() to mark the device as unregistered, it does not
block waiting for active ioctls to conclude.

If a userspace process has the devnode open and is executing an ioctl like
VIDIOC_S_CTRL, it holds the control handler's mutex. When the ioctl
completes, could it access freed memory and attempt to unlock a destroyed
mutex when devres frees the sensor structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-submit-media-mt9m113-v4-0-046b4cbc7f94@herrie.org?part=2