Re: [PATCH 1/2] media: i2c: ak7375: Add support for the AK7377

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The V4L2 control handler `ak7375_set_ctrl()` performs I2C writes without checking the PM runtime state, predictably failing when the device is idle.
- [High] Potential NULL pointer dereference in `ak7375_probe()` if the device is instantiated without matching firmware data.
--

commit 26c53b3777209184aea3eb12bf84bb8a8e00a5f8
Author: Jan-Michael Brummer <jan.brummer@tabos.org>

media: i2c: ak7375: Add support for the AK7377
    
This commit adds support for the AK7377 focus actuator by defining its
register layout and adding it to the compatible table. The AK7377 drives
the focus of the main camera on the Fairphone 5.

> diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c
> --- a/drivers/media/i2c/ak7375.c
> +++ b/drivers/media/i2c/ak7375.c

[ ... ]

> @@ -65,6 +65,25 @@ static const struct ak73xx_chipdef ak7375_cdef = {
>  	.power_delay_us	= 10000,
>  };
>  
> +/*
> + * The AK7377 is register compatible with the AK7375: 12 bit position in
> + * registers 0x00/0x01, standby via 0x02.  Verified on the Fairphone 5,
> + * where it drives the focus of the IMX800 main camera.
> + */
> +static const struct ak73xx_chipdef ak7377_cdef = {
> +	.reg_position	= 0x0,
> +	.reg_cont	= 0x2,
> +	.shift_pos	= 4,	/* 12 bits position values, need to << 4 */
> +	.mode_active	= 0x0,
> +	.mode_standby	= 0x40,
> +	.has_standby	= true,
> +	.focus_pos_max	= 4095,
> +	.focus_steps	= 1,
> +	.ctrl_steps	= 64,
> +	.ctrl_delay_us	= 1000,
> +	.power_delay_us	= 10000,
> +};
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the control handler
safely handle I2C writes when the device is not actively streaming?

If userspace sets the V4L2 focus control while the camera is idle,
ak7375_set_ctrl() blindly issues an I2C write without checking the PM 
runtime state:

drivers/media/i2c/ak7375.c:ak7375_set_ctrl() {
    ...
    if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
        return ak7375_i2c_write(dev_vcm, cdef->reg_position,
                                ctrl->val << cdef->shift_pos, 2);
    ...
}

Without first calling pm_runtime_get_if_in_use() or similar, won't the
unpowered VCM NACK the I2C transaction? This would return an error like
-EIO to userspace and break focus initialization before streaming begins.

[ ... ]

> @@ -331,6 +350,7 @@ static int __maybe_unused ak7375_vcm_resume(struct device *dev)
>  static const struct of_device_id ak7375_of_table[] = {
>  	{ .compatible = "asahi-kasei,ak7345", .data = &ak7345_cdef, },
>  	{ .compatible = "asahi-kasei,ak7375", .data = &ak7375_cdef, },
> +	{ .compatible = "asahi-kasei,ak7377", .data = &ak7377_cdef, },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, ak7375_of_table);

[Severity: High]
This is a pre-existing issue, but can this cause a NULL pointer dereference
if the device is instantiated without matching firmware data?

If the driver is instantiated via the I2C sysfs new_device interface or
legacy board info, there will be no corresponding OF or ACPI node. Since
the driver lacks an I2C id_table, device_get_match_data() will return NULL:

drivers/media/i2c/ak7375.c:ak7375_probe() {
    ...
    ak7375_dev->cdef = device_get_match_data(&client->dev);
    ...
    ret = ak7375_init_controls(ak7375_dev);
    ...
}

The cdef pointer is then passed into ak7375_init_controls(), which
dereferences it unconditionally:

drivers/media/i2c/ak7375.c:ak7375_init_controls() {
    ...
    dev_vcm->focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE,
            0, cdef->focus_pos_max, cdef->focus_steps, 0);
    ...
}

Should there be a NULL check after device_get_match_data() to prevent
local privileged users from triggering a panic via sysfs hotplugging?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829052514.18178-1-jan.brummer@tabos.org?part=1