Re: [PATCH v7 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 8 potential issue(s) to consider:
- [Critical] The mt9m113_ifp_registered callback triggers a Use-After-Free if media_create_pad_link fails.
- [High] mt9m113_remove() ignores the sensor->chip_off flag, causing double-decrement of clocks and regulators.
- [High] mt9m113_ifp_s_stream initiates hardware sequences without acquiring sensor->lock, allowing race conditions.
- [High] The sensor structure and its associated resources are leaked if the driver is unbound before the async subdev matches a bridge.
- [High] PM state mismatch on stream-start failure leads to a double power-off on subsequent retries.
- [High] Calling __v4l2_ctrl_handler_setup() without holding the control handler mutex triggers lockdep assertions and data races.
- [Medium] The pixel array subdev pad operations omit the .set_fmt callback, breaking V4L2 TRY format verification.
- [Medium] Failing to write the test pattern disable command leaves the software state permanently desynchronized from the running MCU.
--

commit 36cbbea6221881b8d118c066c2b7c1a0947ac0a3
Author: Herman van Hazendonk <github.com@herrie.org>
Subject: 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
camera sensor. It features dual context support and single-lane MIPI CSI-2.

> diff --git a/drivers/media/i2c/mt9m113.c b/drivers/media/i2c/mt9m113.c
> new file mode 100644
> index 0000000000000..453e02e0f5b11
> --- /dev/null
> +++ b/drivers/media/i2c/mt9m113.c

[ ... ]

> +static int mt9m113_stream_on(struct mt9m113 *sensor,
> +			     struct v4l2_subdev_state *state)
> +{
[ ... ]
> +	/* Apply all V4L2 controls (color effects, etc.) before streaming */
> +	ret = __v4l2_ctrl_handler_setup(&sensor->ifp.hdl);

[Severity: High]
Is it safe to call this underscored helper without holding the control
handler's lock first? 

The stream-on path seems to be called from the subdev stream operation which
only holds the state lock, so this might trigger a lockdep splat and allow
data races on the control lists. Should this use v4l2_ctrl_handler_setup()
instead?

[ ... ]

> +static int mt9m113_start_streaming(struct mt9m113 *sensor,
> +				   struct v4l2_subdev_state *state)
> +	__must_hold(state->lock)
> +{
[ ... ]
> +		if (attempt + 1 >= MT9M113_STREAM_START_RETRIES) {
> +			/*
[ ... ]
> +			 */
> +			mt9m113_power_off(sensor);
> +			break;
> +		}

[Severity: High]
If the retry loop is exhausted, the hardware is powered down but the PM
runtime state is left active because of the external bridge reference. 

Could a subsequent stream start attempt instantly succeed the PM resume check
and then blindly call power-off again here upon failing to talk to the chip,
causing a clock and regulator refcount underflow?

[ ... ]

> +static int mt9m113_ifp_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct mt9m113 *sensor = ifp_to_mt9m113(sd);
> +	struct v4l2_subdev_state *state;
> +	int ret;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
> +
> +	if (enable)
> +		ret = mt9m113_start_streaming(sensor, state);

[Severity: High]
Does this operation need to acquire the driver's internal sensor->lock? 

Since driver removal and V4L2 control updates acquire it to serialize
hardware access, skipping it here might allow concurrent hardware sequences or
an unbind during stream initialization to corrupt the MCU state.

[ ... ]

> +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 there a missing set_fmt callback in this structure? 

Without it, any try-format operations on the pixel array pad might fail
unconditionally with an unsupported ioctl error, which breaks standard
compliance tools trying to test format states.

[ ... ]

> +static int mt9m113_ifp_registered(struct v4l2_subdev *sd)
> +{
[ ... ]
> +	if (ret < 0) {
> +		dev_err(&sensor->client->dev,
> +			"Failed to link pixel array to ifp\n");
> +		v4l2_device_unregister_subdev(&sensor->pa.sd);
> +		return ret;
> +	}

[Severity: Critical]
Could this error path cause a use-after-free? 

Unregistering the pixel array subdev here will synchronously trigger its
release callback. Since the release count is still at its initial value of 1
at this point, the callback will free the main sensor structure, leaving the
framework to clean up the partially registered ifp subdev embedded within it.

[ ... ]

> +static int mt9m113_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +				if (!ret && sensor->streaming)
> +					ret = mt9m113_refresh(sensor);
> +				if (!ret) {
> +					sensor->test_pattern_active = false;
> +					sensor->test_pattern_value = 0;
> +				}

[Severity: Medium]
If the test pattern register write fails, the MCU is successfully unhalted but
the test pattern active flag remains true. 

Would this prevent all subsequent control updates from issuing their necessary
refresh commands, leaving the software state disconnected from the running
hardware?

[ ... ]

> +static void mt9m113_remove(struct i2c_client *client)
> +{
[ ... ]
> +	pm_runtime_disable(dev);
> +	pm_runtime_dont_use_autosuspend(dev);
> +	if (!pm_runtime_status_suspended(dev))
> +		mt9m113_power_off(sensor);

[Severity: High]
Since the start-streaming failure path sets the chip-off flag after a manual
power down, shouldn't we check that flag here before powering off? 

Bypassing it based purely on the active PM state might trigger a double
disable of the regulators and clocks.

> +	pm_runtime_set_suspended(dev);
> +
> +	v4l2_async_unregister_subdev(&sensor->ifp.sd);
> +	/* @sensor may already be freed here; do not dereference it. */
> +}

[Severity: High]
What happens if the driver is unbound before the async subdev ever matches a
bridge? 

In that case, the subdev was never fully registered to a v4l2 device, so
removing it from the async list won't trigger the release callback, causing
the memory for the sensor structure to be permanently leaked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607-submit-media-mt9m113-v7-0-5da397a3d2a5@herrie.org?part=2