Re: [PATCH v4 2/2] drm/bridge: Add ITE IT6162 MIPI DSI to HDMI bridge driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] Unbalanced regulator enable calls cause permanent resource leaks during probe error paths, duplicate enables on successful probe, and missing disables on driver removal.
- [High] Missing mutex lock in `it6162_write_infoframe()` and `it6162_clear_infoframe()` causes concurrent I2C register corruption.
- [High] The HDCP worker thread calls `drm_hdcp_update_content_protection()` without holding the required DRM `connection_mutex`.
- [High] The `video_setting` struct is missing zero-initialization, causing uninitialized stack memory to be written to hardware registers.
- [High] Ignored return values from I2C regmap reads cause hardware logic to execute based on uninitialized stack memory.
- [High] Premature destruction of mutex and workqueue before the DRM bridge is unregistered, creating a Use-After-Free/Use-After-Destroy window.
- [High] Incorrect bitwise assembly of `chip_id` and `version` due to reversed endianness, causing probe to fail on real hardware.
- [Medium] Adjusted display timings validated during `atomic_check` are completely ignored in `atomic_enable`.
- [Medium] Device tree node reference leak in `it6162_load_mipi_pars_from_port()` on early error return.
- [Medium] Potential NULL pointer dereference of `chip_info` in `it6162_detect_devices()`.
- [Medium] `it6162_get_edid_block()` ignores the provided `len` parameter, potentially writing past the bounds of the destination buffer.
- [Medium] Lockless concurrent access to `it6162->connector_status` can lead to missed HPD notifications.
--

--- Patch [2]: [PATCH v4 2/2] drm/bridge: Add ITE IT6162 MIPI DSI to HDMI bridge driver ---
commit 53d21a12a42eb37de64cf50aa087df4e67188c9d
Author: Hermes Wu <Hermes.wu@ite.com.tw>
Subject: drm/bridge: Add ITE IT6162 MIPI DSI to HDMI bridge driver

This commit adds support for the ITE IT6162 MIPI DSI to HDMI 2.0 bridge chip.
It implements the DRM bridge and connector frameworks, supporting mode setting,
EDID retrieval, and HPD.

> diff --git a/drivers/gpu/drm/bridge/ite-it6162.c b/drivers/gpu/drm/bridge/ite-it6162.c
> new file mode 100644
> index 0000000000000..09274468dc73f
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ite-it6162.c

[ ... ]

> +static void it6162_tx_hdcp_enable(struct it6162 *it6162,
> +				  struct it6162_hdcp_cfg *tx_out)
> +{
> +	int val;
> +	struct regmap *regmap = it6162->regmap;
> +
> +	guard(mutex)(&it6162->lock);
> +	it6162->hdcp_sts = HDCP_STATE_IDLE;
> +
> +	regmap_read(regmap, OFFSET_TX_SETTING, &val);
> +	val &= 0x0F;

[Severity: High]
Does this code process uninitialized memory if the regmap_read() fails?
If the I2C read encounters an error, regmap_read() will leave val 
uninitialized, but its return value is ignored here. The driver then applies 
a bitmask to stack garbage and writes it back to the hardware.

[ ... ]

> +static inline void
> +it6162_display_mode_to_settings(struct drm_display_mode *mode,
> +				struct it6162_video_settings *settings)
> +{
> +	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> +		settings->hpol = 1;
> +
> +	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> +		settings->vpol = 1;
> +
> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		settings->prog = 0;

[Severity: High]
Can this leave hardware settings uninitialized? In
it6162_bridge_atomic_enable(), video_setting is declared without zero
initialization. Here in it6162_display_mode_to_settings(), hpol, vpol,
and prog are conditionally assigned. If the corresponding flags are missing
from mode->flags, these fields will contain uninitialized stack memory,
which is later packed into a byte and sent to the hardware in
it6162_pack_video_setting().

[ ... ]

> +static void it6162_hdcp_handler(struct it6162 *it6162)
> +{
[ ... ]
> +			case HDCP_STATE_AUTH_FAIL:
> +				if (hdcp_ver == HDCP_23)
> +					it6162_tx_hdcp_setup(it6162, HDCP_14,
> +							     true);
> +				else
> +					it6162_tx_hdcp_disable(it6162);
> +
> +				break;
> +			default:
> +				it6162_tx_hdcp_setup(it6162,
> +						     it6162->hdcp_version,
> +						     true);
> +				break;
> +			}
> +			drm_hdcp_update_content_protection(it6162->connector,
> +							   cp_status);
> +		}
> +	}
> +}

[Severity: High]
Is it safe to call drm_hdcp_update_content_protection() from this context?
This is executed asynchronously from a delayed workqueue thread
(it6162_hdcp_work). The DRM helper requires the caller to hold the
dev->mode_config.connection_mutex lock, which is not acquired here. This
could lead to a kernel warning splat and data races.

[ ... ]

> +static void it6162_interrupt_handler(struct it6162 *it6162)
> +{
[ ... ]
> +	connector_status = GET_TX_HPD_STATUS(tx_status) ?
> +			   connector_status_connected :
> +			   connector_status_disconnected;
> +
> +	if (it6162->connector_status != connector_status) {
> +		it6162->connector_status = connector_status;
> +		drm_bridge_hpd_notify(&it6162->bridge,
> +				      it6162->connector_status);
> +	}

[Severity: Medium]
Could this miss HPD notifications due to lockless state tracking?
The it6162_detect() function reads hardware status and updates
it6162->connector_status without locks. If a concurrent ioctl triggers
it6162_detect() right before this check in the IRQ handler, the IRQ might
observe no change and fail to call drm_bridge_hpd_notify().

[ ... ]

> +static int it6162_platform_set_power(struct it6162 *it6162)
> +{
> +	int err;
> +
> +	err = regulator_enable(it6162->ivdd);
> +	if (err)
> +		return err;
> +
> +	err = regulator_enable(it6162->pwr1833);
> +	if (err)
> +		return err;
> +
> +	err = regulator_enable(it6162->ovdd);
> +	if (err)
> +		return err;

[Severity: High]
Does this code leak enabled regulators on failure? If enabling pwr1833
or ovdd fails, the function returns early without calling
regulator_disable() for the already-enabled ivdd and pwr1833.

[ ... ]

> +static int it6162_detect_devices(struct it6162 *it6162)
> +{
> +	struct device *dev = it6162->dev;
> +	const struct it6162_chip_info *chip_info;
> +	u32 chip_id, version;
> +	u8 buf[6];
> +
> +	if (it6162_platform_set_power(it6162) < 0)
> +		return -ENODEV;
> +
> +	/* Firmware must finish booting before register access is valid */
> +	if (!it6162_wait_mcu_ready(it6162))
> +		return -ENODEV;
> +
> +	chip_info = of_device_get_match_data(dev);
> +	regmap_bulk_read(it6162->regmap, OFFSET_CHIP_ID_L, &buf[0], 6);

[Severity: Medium]
Will this crash if probed without a device tree? If the driver is bound
via sysfs or an ACPI fallback using it6162_i2c_ids, of_device_get_match_data()
will return NULL, which is unconditionally dereferenced below.

[Severity: High]
Does this code process uninitialized memory if the regmap_bulk_read() fails?
The return value is ignored. If the read fails, the stack buffer buf
remains uninitialized, and the driver will attempt to match a chip ID
built from garbage memory.

> +	chip_id = (buf[0] << 16) | (buf[1] << 8) | (buf[2]);
> +	version = (buf[3] << 16) | (buf[4] << 8) | (buf[5]);

[Severity: High]
Is the bitwise assembly of chip_id correct for little-endian hardware?
If buf[0] holds the LSB from OFFSET_CHIP_ID_L, shifting it by 16 places it
in the MSB position. This reverses the endianness, causing the chip ID and
version comparison to fail on real hardware.

> +	if (chip_id == chip_info->chip_id && version >= chip_info->version)
> +		return 0;

[ ... ]

> +static enum drm_connector_status it6162_detect(struct it6162 *it6162)
> +{
> +	struct regmap *regmap = it6162->regmap;
> +	unsigned int tx_status;
> +
> +	regmap_read(regmap, OFFSET_TX_STATUS, &tx_status);
> +	it6162->connector_status = GET_TX_HPD_STATUS(tx_status) ?
> +				   connector_status_connected :
> +				   connector_status_disconnected;
> +	return it6162->connector_status;
> +}

[Severity: High]
Does this ignore the return value of regmap_read()? If the I2C read fails,
tx_status contains uninitialized stack memory, which will be parsed as a
hot-plug status, causing erratic or false connector state tracking.

[ ... ]

> +static int it6162_get_edid_block(void *data, u8 *buf, unsigned int block,
> +				 size_t len)
> +{
> +	struct it6162 *it6162 = data;
> +	unsigned int i, cnt;
> +	int ret;
> +	u8 config;
> +
> +	if (len > EDID_LENGTH)
> +		return -EINVAL;
> +
> +	guard(mutex)(&it6162->lock);
> +	for (i = 0, cnt = 0; i < EDID_LENGTH; i += DATA_BUFFER_DEPTH, cnt++) {
> +		config = (block << 2) | (cnt);
> +		ret = it6162_infoblock_request_data(it6162, HOST_SET_EDID_R,
> +						    config, buf + i);

[Severity: Medium]
Will this overwrite memory if the requested len is smaller than EDID_LENGTH?
While it ensures len is not too large, the loop always unconditionally
reads exactly EDID_LENGTH (128) bytes from hardware into buf.

[ ... ]

> +static int it6162_load_mipi_pars_from_port(struct it6162 *it6162, int port)
> +{
> +	struct device_node *of = it6162->dev->of_node;
> +	struct device_node *endpoint;
> +	struct it6162_mipi_cfg *mipicfg = &it6162->mipi_cfg;
> +	u32 data_lanes[4] = {1};
> +	u32 lane_polarities[5] = {0};
> +	int dsi_lanes;
> +
> +	endpoint = of_graph_get_endpoint_by_regs(of, port, -1);
> +	if (!endpoint)
> +		return 0;
> +
> +	dsi_lanes = drm_of_get_data_lanes_count(endpoint, 1, 4);
> +	if (dsi_lanes < 0)
> +		return dsi_lanes;

[Severity: Medium]
Does this code leak the endpoint device node? If drm_of_get_data_lanes_count()
returns a negative error, the function returns without calling of_node_put()
to release the reference acquired by of_graph_get_endpoint_by_regs().

[ ... ]

> +static int it6162_attach_dsi(struct it6162 *it6162)
> +{
[ ... ]
> +		dsi->lanes = mipi_cfg->lane_num;
> +		dsi->format = mipi_cfg->format;
> +		dsi->mode_flags = mipi_cfg->mode_flags;
> +		ret = devm_mipi_dsi_attach(dev, dsi);
> +		if (ret) {
> +			dev_err(dev, "failed to attach dsi device %d", port);
> +			return ret;
> +		}
> +	}
> +
> +	it6162_poweron(it6162);
> +	return 0;
> +}

[Severity: High]
Does this cause duplicate regulator enables? The driver's probe function
calls it6162_detect_devices(), which successfully calls
it6162_platform_set_power(). Later, it6162_attach_dsi() calls it6162_poweron(),
which calls it6162_platform_set_power() a second time, leaving the regulator
enable counts permanently unbalanced.

[ ... ]

> +static void it6162_bridge_atomic_enable(struct drm_bridge *bridge,
> +					struct drm_atomic_state *state)
> +{
> +	struct it6162 *it6162 = bridge_to_it6162(bridge);
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_connector_state *conn_state;
> +	struct drm_connector *connector;
> +	struct hdmi_avi_infoframe *avi;
> +	struct it6162_video_settings video_setting;
[ ... ]
> +	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
> +	avi = &conn_state->hdmi.infoframes.avi.data.avi;
> +	video_setting.colorspace = conn_state->hdmi.output_format;
> +	it6162_avi_to_video_setting(avi, &video_setting);
> +	it6162_display_mode_to_settings(&crtc_state->mode, &video_setting);
> +
> +	it6162_mipi_set_video_timing(it6162, &video_setting);
> +	it6162_tx_enable(it6162);
> +	it6162_mipi_enable(it6162);
> +}

[Severity: Medium]
Should this use crtc_state->adjusted_mode instead of crtc_state->mode?
The it6162_bridge_atomic_check() function enforces hardware constraints by
modifying the horizontal front porch in crtc_state->adjusted_mode. However,
this function discards those adjustments and passes the original unadjusted
mode directly to it6162_display_mode_to_settings().

[ ... ]

> +static inline int
> +it6162_write_infoframe(struct it6162 *it6162, const u8 *buffer, size_t len)
> +{
> +	if (len > DATA_BUFFER_DEPTH)
> +		return -EINVAL;
> +
> +	regmap_bulk_write(it6162->regmap, OFFSET_DATA_BUFFER, buffer, len);
> +	regmap_write(it6162->regmap, OFFSET_DATA_TYPE_IDX, len);
> +	it6162_infoblock_trigger(it6162, HOST_SET_CEA_INFOFRAME);
> +	return 0;
> +}
> +
> +static inline int it6162_clear_infoframe(struct it6162 *it6162, u8 type)
> +{
> +	regmap_write(it6162->regmap, OFFSET_DATA_TYPE_IDX, 3);
> +	regmap_write(it6162->regmap, OFFSET_DATA_BUFFER, type);
> +	regmap_write(it6162->regmap, OFFSET_DATA_BUFFER + 1, 0x00);
> +	regmap_write(it6162->regmap, OFFSET_DATA_BUFFER + 2, 0x00);
> +	it6162_infoblock_trigger(it6162, HOST_SET_CEA_INFOFRAME);
> +	return 0;
> +}

[Severity: High]
Do these functions lack the necessary mutex lock? They write to the shared
mailbox registers (OFFSET_DATA_BUFFER and OFFSET_DATA_TYPE_IDX) and trigger
firmware commands without acquiring it6162->lock. Since they are called during
atomic commits, they could corrupt concurrent I2C register accesses from
it6162_get_edid_block() or it6162_hdcp_read_information().

[ ... ]

> +static void it6162_remove(struct i2c_client *client)
> +{
> +	struct it6162 *it6162 = i2c_get_clientdata(client);
> +
> +	disable_irq(client->irq);
> +	cancel_delayed_work_sync(&it6162->hdcp_work);
> +	mutex_destroy(&it6162->lock);
> +}

[Severity: High]
Does this code leak the enabled regulators during driver removal? There is no
call to regulator_disable() or the it6162_poweroff() helper during teardown.

[Severity: High]
Is this prone to a use-after-destroy window? The DRM bridge is registered
with devm_drm_bridge_add(), so it remains globally active until after this
remove function returns. If userspace issues an ioctl (e.g., an EDID read or
modeset) concurrently during unbind, the bridge callbacks will try to lock the
already-destroyed mutex.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260319-upstream-6162-v4-0-7268483ef03d@ite.com.tw?part=2