Re: [PATCH v3 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 3 potential issue(s) to consider:
- [High] The test pattern implementation has critical state machine flaws that prevent it from activating on stream start and break the video stream when disabled.
- [High] V4L2 controls silently fail to apply when CONFIG_PM is disabled.
- [Medium] Modifying V4L2 controls while the test pattern is active causes 1-second stalls and false-positive timeout warnings.
--

commit 089f461b38f980bbf1bb0f31d1f94bb7bd1d0721
Author: Herman van Hazendonk <github.com@herrie.org>

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

This commit adds a V4L2 subdev driver for the Aptina MT9M113 1.3 Megapixel
SoC image sensor. It supports dual contexts (preview and capture), various
V4L2 controls, and test patterns. The driver also includes a workaround for
intermittent MCU wedges during stream start by using runtime-PM retries.

> 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_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct mt9m113 *sensor = container_of(ctrl->handler,
> +					       struct mt9m113, ifp.hdl);
> +	int ret = 0;
> +
> +	/*
> +	 * pm_runtime_get_if_in_use() returns >0 on success, 0 if not active,
> +	 * and -EINVAL if runtime PM is disabled.  Both 0 and -EINVAL mean we
> +	 * must not touch the hardware (and must not pair with a put), so the
> +	 * defensive comparison is "<= 0" rather than the naive "!ret".
> +	 */
> +	if (pm_runtime_get_if_in_use(&sensor->client->dev) <= 0)
> +		return 0;

[Severity: High]
Does this code silently ignore V4L2 control updates when CONFIG_PM is
disabled?

When CONFIG_PM is not enabled, pm_runtime_get_if_in_use() will return
-EINVAL. By treating -EINVAL the same as an inactive state (0), all hardware
control updates such as exposure, gain, saturation, and flip will be
skipped, while still returning success to userspace. Should the driver treat
-EINVAL as an active state to ensure controls are applied?

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_HFLIP: {
[ ... ]
> +		/* Only refresh if streaming - otherwise MCU may not be ready */
> +		if (!ret && sensor->streaming)
> +			mt9m113_refresh(sensor);
> +		break;

[Severity: Medium]
Will this result in 1-second stalls and false-positive timeout warnings if a
control is modified while the test pattern is active?

When the test pattern is active, the MCU is halted (BOOT_MODE = 1). If a
standard control like V4L2_CID_HFLIP is changed, mt9m113_s_ctrl() attempts
to call mt9m113_refresh(), which tries to send SEQ_CMD_REFRESH. Because the
MCU is halted, it cannot process the command, leading to two 500ms timeouts
in mt9m113_poll_mcu_var(). Should the driver check if the MCU is halted by
the test pattern before attempting a refresh?

[ ... ]
> +	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: High]
Can disabling the test pattern permanently freeze the camera stream?

When userspace disables the test pattern while streaming, writing 0 to
MT9M113_MCU_BOOT_MODE releases the halt and reboots the MCU. However, the
driver does not appear to reapply the sensor configuration (such as the init
table, output dimensions, and AE) or issue SEQ_CMD_RUN afterward. Will the
stream remain frozen without these initialization steps?

> +		} else {
> +			/*
> +			 * Enable test pattern mode.
> +			 * Per datasheet: "Disabling the MCU is recommended
> +			 * before enabling test patterns."
[ ... ]
> +			sensor->test_pattern_active = true;
> +			dev_dbg(&sensor->client->dev,
> +				 "MT9M113: Enabling test pattern %d\n", ctrl->val);
> +			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: High]
Does the test pattern fail to activate if it is set before streaming starts?

If the test pattern is configured before streaming begins,
__v4l2_ctrl_handler_setup() (called from mt9m113_stream_on()) applies this
control while sensor->streaming is still false. This skips halting the MCU
with BOOT_MODE = 1. When mt9m113_stream_on() then starts the normal
sequencer, the test pattern settings will be overwritten by the normal
streaming configuration. How should the test pattern state be maintained
across stream starts?

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