Re: [PATCH v2 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 4 potential issue(s) to consider:
- [High] PM Runtime concurrency defeats the MCU wedge recovery power-cycle in `mt9m113_start_streaming()`.
- [High] Context A silently ignores the `SEQ_CMD_RUN` timeout and returns 0, defeating the retry logic and leaving the camera wedged.
- [Medium] Test pattern control fails to hold the MCU in boot mode if applied before streaming, and misses a refresh when disabled.
- [Medium] The pixel array subdevice (`mt9m113_pa_pad_ops`) lacks a `.set_fmt` pad operation, which breaks userspace pipeline configuration.
--

commit 72b95e84525e87e9da1a8e3e471cb42137c9f583
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 (1280x1024 active pixel array) with on-chip ISP. The
sensor exposes dual context operation, single-lane MIPI CSI-2 output,
and various V4L2 controls.

> diff --git a/drivers/media/i2c/mt9m113.c b/drivers/media/i2c/mt9m113.c
> new file mode 100644
> index 0000000000000..7a4a8a50822d3
> --- /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 silently ignore the timeout error?

If mt9m113_poll_mcu_var() returns -ETIMEDOUT, the function logs a warning
but proceeds to return 0.

This tricks mt9m113_stream_on() into succeeding and bypasses the retry logic
in mt9m113_start_streaming(), leaving the camera wedged.

[ ... ]
> +static int mt9m113_start_streaming(struct mt9m113 *sensor,
> +				   struct v4l2_subdev_state *state)
> +{
[ ... ]
> +	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]
Is this retry path vulnerable to PM runtime concurrency?

If another context holds a PM runtime reference, the usage counter will not
drop to zero.

The call to pm_runtime_put_sync_suspend() will return -EAGAIN without
suspending, meaning the sensor is never physically power-cycled and the
wedge recovery fails.

[ ... ]
> +static const struct v4l2_subdev_pad_ops mt9m113_pa_pad_ops = {
> +	.enum_mbus_code = mt9m113_pa_enum_mbus_code,
> +	.enum_frame_size = mt9m113_pa_enum_framesizes,
> +	.get_fmt = v4l2_subdev_get_fmt,
> +	.get_selection = mt9m113_pa_get_selection,
> +};

[Severity: Medium]
Is the set_fmt callback missing here?

V4L2 requires a set_fmt callback even for fixed-format pads so that it can
force the state to the fixed format.

Without it, userspace tools configuring the pipeline will receive
-ENOIOCTLCMD and fail to negotiate the format link.

[ ... ]
> +static int mt9m113_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	case V4L2_CID_TEST_PATTERN:
> +		if (ctrl->val == 0) {
[ ... ]
> +			if (sensor->test_pattern_active) {
> +				dev_dbg(&sensor->client->dev,
> +					 "MT9M113: Disabling test pattern, restarting MCU\n");
> +				cci_write(sensor->regmap, MT9M113_MCU_BOOT_MODE,
> +					  0x0000, &ret);
> +				if (!ret) {
> +					usleep_range(10000, 15000);
> +					ret = mt9m113_write_mcu_var(sensor,
> +								    MT9M113_CAM_MODE_SELECT,
> +								    MT9M113_CAM_MODE_SELECT_NORMAL);
> +				}
> +				if (!ret)
> +					sensor->test_pattern_active = false;
> +			}

[Severity: Medium]
Does disabling the test pattern miss a refresh call?

The driver writes MT9M113_CAM_MODE_SELECT_NORMAL but does not call
mt9m113_refresh(), so the MCU might ignore the request to switch back to
normal mode.

> +		} else {
[ ... ]
> +			ret = mt9m113_write_mcu_var(sensor,
> +						    MT9M113_CAM_MODE_TEST_PATTERN_SELECT,
> +						    mt9m113_test_pattern_value[ctrl->val - 1]);
> +			if (!ret)
> +				ret = mt9m113_write_mcu_var(sensor,
> +							    MT9M113_CAM_MODE_SELECT,
> +							    MT9M113_CAM_MODE_SELECT_TEST_PATTERN);
> +			if (!ret && sensor->streaming) {
> +				mt9m113_refresh(sensor);
> +				dev_dbg(&sensor->client->dev,
> +					 "MT9M113: Stopping MCU for test pattern\n");
> +				cci_write(sensor->regmap, MT9M113_MCU_BOOT_MODE,
> +					  0x0001, &ret);
> +			}
> +		}
> +		break;

[Severity: Medium]
If V4L2_CID_TEST_PATTERN is set before streaming starts,
__v4l2_ctrl_handler_setup() applies it while sensor->streaming is still
false.

Will this cause the driver to skip putting the MCU in boot mode
(MT9M113_MCU_BOOT_MODE = 0x0001)? The free-running MCU would then override
the test pattern.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-submit-media-mt9m113-v2-0-de1cc807f4cf@herrie.org?part=2