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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider:
- [High] The HDMI interrupt handler checks a register offset macro instead of the read value, causing interrupts to be silently ignored.
- [High] Deadlock between the PM runtime suspend callback and the threaded IRQ handler.
- [High] Calling `drm_hdcp_update_content_protection()` without holding `connection_mutex` triggers a WARN_ON and creates a state race.
- [High] The delayed work `hdcp_work` is not canceled during driver removal, leading to a Use-After-Free.
- [High] Double detachment of the MIPI DSI device due to mixing manual and devm-managed teardown.
- [High] Unclamped DDC FIFO read count in `it61620_hdmi_hdcp_ksvlist_rd` can cause a heap buffer overflow or an infinite loop.
- [High] Incorrect SHA-1 hash length computation breaks HDCP authentication.
- [High] edid_read accesses powered-off hardware, causing regmap_read failures and pervasive use of uninitialized stack variables.
- [High] Premature destruction of `ddc_lock` in `it61620_remove()` before the bridge is unregistered.
- [Medium] PM runtime and regulator state leaks in probe errors and remove path.
- [Medium] Incorrect regulator disable order in it61620_poweroff().
- [Medium] `it61620_parse_dt()` is declared to return `unsigned int` but returns negative error codes.
- [Medium] Unprotected DDC hardware access in `it61620_hdmi_ddc_abort()`.
- [Medium] Uninitialized stack variables are written to hardware registers if unsupported audio parameters are supplied.
- [Medium] Unsigned underflow in abs(htotal - 2880) causes incorrect delay calculation.
--

--- Patch [2]: [PATCH v8 2/2] drm/bridge: Add ITE IT61620 MIPI DSI to HDMI bridge driver ---
commit 0da87ff1083d89041b3e6585c68651e55d9e8de2
Author: Pet Weng <pet.weng@ite.com.tw>

drm/bridge: Add ITE IT61620 MIPI DSI to HDMI bridge driver

This adds support for the ITE IT61620 bridge chip which converts
MIPI DSI input to HDMI output. The Driver implements the basic
bridge functions and integrates with the DRM bridge and connector
frameworks.

> diff --git a/drivers/gpu/drm/bridge/ite-it61620.c b/drivers/gpu/drm/bridge/ite-it61620.c
> index 0000000000000..95771e5a39114
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ite-it61620.c

[ ... ]

> @@ -353,10 +353,10 @@ static void it61620_mipi_set_d2v_video_timing(struct it61620 *it61620,
>  	if (it61620->dev_ver != DEV_VERSION_A0)
>  		regmap_update_bits(it61620->mipirx_regmap, RX_REG_SELDCLK, 0x1c, 0x04);
>  
>  	if (hdew > 1920) {
>  		if (htotal < 4272) {

[Severity: Medium]
Could htotal - 2880 underflow here? Since htotal is defined as a u32 and can
be less than 2880, the result might wrap around to a very large positive
integer. The abs() macro would return this unsigned value unchanged,
resulting in a large value being divided by 24 and cast to u8, which could
configure incorrect video timing delays.

>  			d2vffrd_adr_dly = abs(htotal - 2880) / 24;
>  			regmap_write(it61620->mipirx_regmap, RX_REG_DSC_VFRD,
>  				     d2vffrd_adr_dly);
>  		} else {

[ ... ]

> @@ -482,10 +482,10 @@ static void it61620_hdmi_ddc_abort(struct it61620 *it61620)
>  static unsigned int it61620_hdmi_read_ddc_status(struct it61620 *it61620)
>  {
>  	unsigned int val;
>  
>  	regmap_read(it61620->tx_regmap, TX_REG_DDC_STATUS, &val);

[Severity: High]
Is it safe to return val without checking if regmap_read() succeeded? If the
device is powered off, regmap_read() will fail and leave val uninitialized.
This uninitialized stack variable is then returned and used as a loop
termination condition in it61620_hdmi_ddc_wait(), which could lead to
unpredictable polling loop behavior.

>  	return val;
>  }

[ ... ]

> @@ -628,10 +628,10 @@ static int it61620_hdmi_hdcp_ksvlist_rd(struct it61620 *it61620,
>  		if (it61620_hdmi_ddc_wait(it61620) < 0) {
>  			it61620_hdmi_ddc_abort(it61620);
>  			ret = -EIO;
>  			dev_dbg(it61620->dev, "ddc fail");
>  			break;
>  		}
>  
>  		regmap_read(it61620->tx_regmap, TX_REG_DDC_FIFO_STS, &count);
>  		count &= M_DDC_STAGE_NUM;

[Severity: High]
Can the hardware report a count that exceeds the remaining bytes requested?
The loop blindly accepts the hardware count and writes it to the output
buffer. If count is not clamped to (bytenum - i), it could overflow the
sha1_input buffer. Additionally, if the hardware reports 0 bytes, would this
cause an infinite loop?

>  		it61620_hdmi_get_ddc_fifo(it61620, (out + i), count);
>  		i += count;
>  	}

[ ... ]

> @@ -668,10 +668,10 @@ static int it61620_hdmi_setup_sha1_input(struct it61620 *it61620, u8 *input)
>  	input[count++] = bstatus[0];
>  	input[count++] = bstatus[1];
>  
>  	regmap_update_bits(it61620->tx_regmap, TX_REG_SHA_SEL, 0x70, 0x70);
>  	regmap_update_bits(it61620->tx_regmap, TX_REG_HDCP_CTRL2, B_EN_M0_RD, B_EN_M0_RD);
>  	regmap_bulk_read(it61620->tx_regmap, TX_REG_ANM0V1, &input[count], 8);
>  
>  	regmap_update_bits(it61620->tx_regmap, TX_REG_HDCP_CTRL2, B_EN_M0_RD, 0x00);

[Severity: High]
Should the count variable be incremented by 8 after reading the M0 value into
the input buffer? Because count is returned without accounting for these 8
bytes, the caller might compute the SHA-1 hash with an incomplete length,
which could cause HDCP repeater authentication to consistently fail.

>  	return count;
>  }

[ ... ]

> @@ -754,10 +754,10 @@ static int it61620_hdmi_enable_hdcp(struct it61620 *it61620)
>  	if (auth_cs == CS_AUTH_DONE) {
>  		it61620_hdmi_enable_avmute(it61620, false);
>  		regmap_update_bits(it61620->tx_regmap, TX_REG_INT1, B_INT_AUTH_F,
>  				   B_INT_AUTH_F);
>  		regmap_update_bits(it61620->tx_regmap, TX_REG_INT_MASK1, B_INT_AUTH_F,
>  				   B_INT_AUTH_F);
>  		drm_dbg(drm, "auth done");
>  		it61620->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_ENABLED;

[Severity: High]
Could calling drm_hdcp_update_content_protection() in this context trigger a
warning from the DRM core? The DRM core expects the mode config
connection_mutex to be held when updating this state. Modifying connector
state concurrently with atomic commits might create data races.

>  		drm_hdcp_update_content_protection(it61620->connector,
>  						   it61620->hdcp_cp);
>  		return true;
>  	}
>  
>  	drm_dbg(drm, "auth fail");
>  	it61620_hdmi_enable_avmute(it61620, true);

[Severity: Medium]
Is there a risk of concurrent hardware access here? While standard EDID reads
hold the ddc_lock, this abort routine is called on HDCP error paths without
acquiring the lock. If userspace concurrently attempts an EDID read, it might
cause state corruption on the DDC bus.

>  	it61620_hdmi_ddc_abort(it61620);
>  	return false;
>  }

[ ... ]

> @@ -876,10 +876,10 @@ static void it61620_hdmi_irq_hdcp_auth_fail(struct it61620 *it61620)
>  {
>  	struct drm_device *drm = it61620->drm;
>  
>  	if (it61620->hdcp_state == CP_GOING) {
>  		it61620_hdmi_enable_avmute(it61620, true);
>  		drm_dbg(drm, "auth fail after done, set avmute");
>  		it61620->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_DESIRED;

[Severity: High]
Could calling drm_hdcp_update_content_protection() here from the threaded IRQ
handler cause lockdep splats or state races? Similar to the previous usage,
the connection_mutex is not held here.

>  		drm_hdcp_update_content_protection(it61620->connector,
>  						   it61620->hdcp_cp);
>  		it61620_start_hdcp_work(it61620);
>  	}
>  }

[ ... ]

> @@ -930,10 +930,10 @@ static void it61620_hdmi_interrupt_handler(struct it61620 *it61620)
>  	regmap_read(it61620->tx_regmap, TX_REG_INT3, &int_status3);
>  
>  	regmap_write(it61620->tx_regmap, TX_REG_INT1, int_status1);
>  	regmap_write(it61620->tx_regmap, TX_REG_INT2, int_status2);
>  	regmap_write(it61620->tx_regmap, TX_REG_INT3, int_status3);

[Severity: High]
Does this conditional evaluate the TX_REG_INT1 macro (which expands to the
register address 0x10) instead of the int_status1 variable that was just read?
Since TX_REG_INT1 & B_HPD_CHG always evaluates to 0, this would silently
ignore Hot-Plug Detect and other interrupts.

>  	if (TX_REG_INT1 & B_HPD_CHG)
>  		it61620_hdmi_irq_hpd(it61620);
>  
>  	if (TX_REG_INT1 & B_RXSEN_CHANGE)
>  		it61620_hdmi_irq_rxsen_chg(it61620);
>  
>  	if (TX_REG_INT1 & B_INT_AUTH_F)
>  		it61620_hdmi_irq_hdcp_auth_fail(it61620);

[ ... ]

> @@ -1067,10 +1067,10 @@ static void it61620_audio_update_hw_params(struct it61620 *it61620,
>  	case HDMI_I2S:
>  	case HDMI_SPDIF:
>  		break;
>  	default:
>  		return;
>  	}
>  
>  	is_lpcm = !(hparms->iec.status[0] & IEC958_AES0_NONAUDIO);
>  	it61620_hdmi_audio_set_ncts(it61620, hparms->sample_rate);
>  
>  	drm_dbg(drm, "sample rate %d", sample_rate_val);
>  	drm_dbg(drm, "sample width %d", sample_width);

[Severity: Medium]
Are sample_rate_val and sample_width guaranteed to be initialized? The switch
statements lack default cases, so if the ALSA/DRM core passes an unsupported
audio parameter, these variables remain uninitialized when they are written
to the hardware registers below.

>  	regmap_update_bits(it61620->tx_regmap, TX_REG_AUD_FMT, 0x7f,
>  			   (sample_width << 5) | it61620->i2s_input_format);

[ ... ]

> @@ -1220,10 +1220,10 @@ static unsigned int it61620_parse_dt(struct it61620 *it61620)
>  {
>  	struct device *dev = it61620->dev;
>  	struct device_node *np = it61620->dev->of_node;
>  	int num_lanes;
>  
>  	if (!dev->of_node)

[Severity: Medium]
Should this function be declared to return an int? It is currently defined
to return an unsigned int, but it directly returns negative error codes like
-EINVAL and the result of dev_err_probe().

>  		return -EINVAL;
>  
>  	num_lanes = drm_of_get_data_lanes_count_ep(np, 0, -1, 1, 4);
>  	if (num_lanes < 0)
>  		num_lanes = 4;
>  	it61620->mipirx_config.lane_num = num_lanes;
>  
>  	it61620->next_bridge = devm_drm_of_get_bridge(dev, np, 1, -1);
>  	if (IS_ERR(it61620->next_bridge))
>  		return dev_err_probe(dev, PTR_ERR(it61620->next_bridge),
>  				     "failed to get next bridge\n");
>  
>  	return 0;
>  }

[ ... ]

> @@ -1310,18 +1310,18 @@ static int it61620_poweroff(struct it61620 *it61620)
>  		return 0;
>  	}
>  
>  	if (it61620->it61620_i2c->irq) {

[Severity: High]
The suspend path calls it61620_poweroff() which synchronously blocks on
disable_irq() waiting for the threaded handler to finish. However, the
threaded handler calls pm_runtime_get_sync(dev), which blocks waiting for
suspend to finish. Could this create an AB-BA deadlock where both paths wait
on each other?

>  		disable_irq(it61620->it61620_i2c->irq);
>  		drm_dbg(drm, "disable irq %d\n",
>  			it61620->it61620_i2c->irq);
>  	}
>  
>  	gpiod_set_value_cansleep(it61620->gpiod_reset, 1);

[Severity: Medium]
Are the regulators being disabled in the correct order? They are enabled in
the order ivdd, ovdd1833, then ovdd33, but they are disabled here in the
order ovdd33, ivdd, then ovdd1833. Disabling ivdd before ovdd1833 violates
the reverse sequence and might lead to hardware malfunction.

>  	err = regulator_disable(it61620->ovdd33);
>  	if (err)
>  		dev_err(dev, "Failed to disable ovdd33: %d", err);
>  
>  	err = regulator_disable(it61620->ivdd);
>  	if (err)
>  		dev_err(dev, "Failed to disable IVDD: %d", err);
>  
>  	usleep_range(2000, 3000);
>  
>  	err = regulator_disable(it61620->ovdd1833);
>  	if (err)
>  		dev_err(dev, "Failed to disable ovdd1833: %d", err);

[ ... ]

> @@ -1486,10 +1486,10 @@ static const struct drm_edid *it61620_bridge_edid_read(struct drm_bridge *bridge
>  						       struct drm_connector *connector)
>  {
>  	struct it61620 *it61620 = bridge_to_it61620(bridge);
>  	struct device *dev = it61620->dev;
>  	const struct drm_edid *edid;

[Severity: High]
Does this path ensure the device is powered on before accessing the hardware?
Unprivileged userspace can trigger EDID reads, and if the bridge is currently
runtime-suspended and not awakened via pm_runtime_get_sync(), the subsequent
I2C operations would fail.

>  	edid = drm_edid_read_custom(connector, it61620_get_edid_block, it61620);
>  
>  	if (!edid)
>  		dev_dbg(dev, "failed to get edid!");
>  
>  	return edid;
>  }

[ ... ]

> @@ -1560,10 +1560,10 @@ static irqreturn_t it61620_int_threaded_handler(int unused, void *data)
>  {
>  	struct it61620 *it61620 = data;
>  	struct device *dev = it61620->dev;

[Severity: High]
Can this cause an AB-BA deadlock with the suspend path? The threaded handler
calls pm_runtime_get_sync(dev) below, which blocks waiting for any ongoing
suspend to finish, while suspend waits for this handler via disable_irq().

>  	pm_runtime_get_sync(dev);
>  
>  	it61620_mipi_irq_handler(it61620);
>  	it61620_hdmi_irq(it61620);

[ ... ]

> @@ -1763,16 +1763,16 @@ static void it61620_remove(struct i2c_client *client)
>  {
>  	struct it61620 *it61620 = i2c_get_clientdata(client);
>  	struct device *dev = it61620->dev;
>  
>  	disable_irq(client->irq);

[Severity: Medium]
Does this function ensure the device is synchronously powered off before
disabling PM runtime? If the device was active when unbound, calling
pm_runtime_disable() without a prior pm_runtime_put_sync() could leak the
regulator and PM states.

>  	pm_runtime_disable(dev);

[Severity: High]
Is it necessary to call it61620_detach_dsi() here? The DSI device is attached
using devm_mipi_dsi_attach() during probe, which means the devres framework
will automatically detach it. Doing it manually here might cause a double
detach.

[Severity: High]
Should cancel_delayed_work_sync(&it61620->hdcp_work) be called here? If an
HDCP authentication work item is queued when the driver is unbound, the
pending work might execute after the devres-managed structure is freed,
leading to a use-after-free.

>  	it61620_detach_dsi(it61620);

[Severity: High]
Is it safe to destroy the ddc_lock mutex before the bridge is unregistered?
The bridge is registered via devm_drm_bridge_add(), so it remains active until
the devres cleanup phase, which runs after remove() completes. If userspace
initiates an EDID read during this window, it would attempt to lock a
destroyed mutex.

>  	mutex_destroy(&it61620->ddc_lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260320-it61620-0714-v8-0-0e70271cf5a9@ite.com.tw?part=2