Re: [PATCH v5 2/2] media: i2c: add driver for ITE IT6625/IT6626

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] IT6626 C-PHY support is broken because the endpoint parsing logic forces D-PHY, causing C-PHY device tree configurations to fail probing.
- [High] Concurrent ioctls racing with remove() can schedule delayed work on freed memory.
- [Medium] Ignored return value from v4l2_phys_addr_validate() allows configuration of malformed CEC topologies.
--

--- Patch [2]: [PATCH v5 2/2] media: i2c: add driver for ITE IT6625/IT6626 ---
commit bfb7e6cd73b65d97c7af09ea2f5bbbecc5bfd67b
Author: Hermes Wu <Hermes.wu@ite.com.tw>
Subject: media: i2c: add driver for ITE IT6625/IT6626

This patch adds a V4L2 subdevice driver for the ITE IT6625/IT6626 HDMI-to-MIPI
CSI-2 bridge chips. It supports EDID read/write, DV timings detection, and
exposes an HDMI CEC adapter. The bridge is configured over I2C with HPD
control via standard V4L2 operations.

> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
> --- /dev/null
> +++ b/drivers/media/i2c/it6625.c
> [ ... ]
> @@ -1167,7 +1167,7 @@ static int it6625_s_edid(struct v4l2_subdev *sd,
>  		pa = cec_get_edid_phys_addr(edid->edid, edid->blocks * 128, NULL);
>  		if (pa == CEC_PHYS_ADDR_INVALID) {
>  			v4l2_warn(sd, "invalid CEC physical address in EDID");
>  			cec_phys_addr_invalidate(it6625->cec_adap);
>  		} else {
>  			v4l2_phys_addr_validate(pa, NULL, NULL);
[Severity: Medium]
Does this code ignore the return value of v4l2_phys_addr_validate()?

If a malformed EDID provides an invalid CEC physical address, the validation
might fail, but the code appears to proceed and pass the address to
cec_s_phys_addr() unconditionally. Could this allow the configuration of
malformed CEC topologies?

>  			cec_s_phys_addr(it6625->cec_adap, pa, false);
>  		}
>  	}
> [ ... ]
> @@ -1627,7 +1627,7 @@ static int it6625_parse_endpoint(struct it6625 *it6625)
>  {
>  	struct device *dev = it6625->dev;
>  	struct v4l2_fwnode_endpoint endpoint = { .bus_type = V4L2_MBUS_CSI2_DPHY };
[Severity: High]
Is IT6626 C-PHY support disabled because of this initialization?

By explicitly setting bus_type to V4L2_MBUS_CSI2_DPHY, the V4L2 fwnode
parser might strictly reject C-PHY endpoints during probe.

>  	struct device_node *ep = NULL;
>  	unsigned int max_lanes;
>  	unsigned int port;
> [ ... ]
> @@ -1659,7 +1659,7 @@ static int it6625_parse_endpoint(struct it6625 *it6625)
>  	ret = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(ep), &endpoint);
>  	of_node_put(ep);
>  	if (ret) {
>  		dev_err(dev, "failed to parse endpoint: %d", ret);
>  		return ret;
>  	}
>  
>  	if (endpoint.bus_type == V4L2_MBUS_CSI2_CPHY &&
>  	    it6625->chip_type != IT6626_CHIP) {
[Severity: High]
Will this C-PHY check ever be reached?

If a device tree specifies a C-PHY endpoint, v4l2_fwnode_endpoint_alloc_parse()
would likely have already returned an error because the driver pre-initialized
the endpoint bus type to D-PHY above.

>  		dev_err(dev, "IT6625 does not support C-PHY, only IT6626 does");
>  		v4l2_fwnode_endpoint_free(&endpoint);
>  		return -EINVAL;
>  	}
> [ ... ]
> @@ -2169,7 +2169,7 @@ static void it6625_remove(struct i2c_client *client)
>  	if (client->irq)
>  		disable_irq(client->irq);
>  	else
>  		timer_shutdown_sync(&it6625->timer);
>  
>  	cancel_work_sync(&it6625->polling_work);
>  	cancel_delayed_work_sync(&it6625->hpd_delayed_work);
>  	debugfs_remove_recursive(it6625->debugfs_dir);
>  	cec_unregister_adapter(it6625->cec_adap);
>  	mutex_destroy(&it6625->it6625_lock);
[Severity: High]
Can a concurrent ioctl race with this teardown and cause a use-after-free?

If VIDIOC_SUBDEV_S_EDID is in flight and reaches the end of it6625_s_edid():

drivers/media/i2c/it6625.c:it6625_s_edid() {
    ...
    if (hdmi_5v_power_present(it6625)) {
        it6625_enable_hpd(it6625);
    ...
}

Could it unconditionally schedule the delayed work after it6625_remove() has
already completed cancel_delayed_work_sync() and the devm-allocated driver
state has been freed?

I noted the commit message mentions this residual risk is similar to other
peer drivers and was discussed in previous versions, but could it still result
in enqueuing a timer on freed memory?

>  	media_entity_cleanup(&sd->entity);
>  	v4l2_ctrl_handler_free(&it6625->hdl);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-upstream-it6625-it6626-v5-0-387e7584f2c9@ite.com.tw?part=2