Re: [PATCH v8 2/2] drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] DSI transfer callback returns success for reads without populating the receive buffer, leaking uninitialized memory.
- [High] DSI transfer callback polls for eDP video readiness before the upstream encoder is enabled, guaranteeing a timeout.
- [High] The driver attempts I2C transfers while the hardware is held in physical reset.
- [High] Missing DRM_MIPI_DSI Kconfig dependency causes linker errors.
- [High] request_firmware is called while holding the hardware lock and halting the MCU, risking a system pipeline stall.
- [High] Sleeping functions are called from atomic context in the DRM bridge callbacks.
- [Medium] lt7911exc_dsi_host_transfer bypasses the required MCU hardware halt sequence.
--

commit ac42b7d6e1a6501b8416d964b8bbfde4d4b376bc
Author: Sunyun Yang <syyang@lontium.com>
Subject: drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge

This commit adds support for the Lontium LT7911EXC bridge chip, converting eDP
input to MIPI DSI output. It includes I2C firmware upgrade mechanisms, GPIO
management, and DRM bridge integration.

> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index c3209b0f46786..cb74730c6ef44 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -132,6 +132,21 @@ config DRM_ITE_IT6505
>  	help
>  	  ITE IT6505 DisplayPort bridge chip driver.
>  
> +config DRM_LONTIUM_LT7911EXC
> +	tristate "Lontium eDP/MIPI DSI bridge"
> +	depends on OF
> +	depends on I2C
> +	select CRC32
> +	select FW_LOADER
> +	select DRM_PANEL
> +	select DRM_KMS_HELPER
> +	select REGMAP_I2C

Is a select DRM_MIPI_DSI missing here? 

The driver unconditionally uses MIPI DSI core APIs such as
mipi_dsi_host_register() and mipi_dsi_host_unregister(), so building without
the MIPI DSI core enabled might result in undefined reference build failures.

[ ... ]
> diff --git a/drivers/gpu/drm/bridge/lontium-lt7911exc.c b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
> new file mode 100644
> index 0000000000000..e7b8a22b2e29b
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
[ ... ]
> +static int lt7911exc_firmware_upgrade(struct lt7911exc *lt7911exc)
> +{
> +	struct device *dev = lt7911exc->dev;
> +	const struct firmware *fw;
> +	u8 *buffer;
> +	size_t total_size = FW_SIZE - 4;
> +	u32 crc32;
> +	int ret;
> +
> +	/* 1. load firmware */
> +	ret = request_firmware(&fw, FW_FILE, dev);

Could calling request_firmware() here block concurrent DRM atomic commits and 
freeze the display pipeline? 

This function is called by lt7911exc_firmware_store(), which acquires 
ocm_lock and halts the MCU via lt7911exc_lock() before invoking it. If the 
firmware file is missing or takes time to load, request_firmware() can 
block waiting for the userspace fallback mechanism while the mutex is held.

[ ... ]
> +static void lt7911exc_atomic_pre_enable(struct drm_bridge *bridge, struct drm_atomic_state *state)
> +{
> +	struct lt7911exc *lt7911exc = bridge_to_lt7911exc(bridge);
> +
> +	guard(mutex)(&lt7911exc->ocm_lock);
> +
> +	lt7911exc_reset(lt7911exc);
> +}

Is it safe to use sleeping functions like guard(mutex) and msleep() inside 
lt7911exc_reset(), as well as gpiod_set_value_cansleep() here? 

The DRM subsystem guide specifies that encoder atomic enable/disable 
callbacks execute in an atomic context where sleeping is forbidden, which 
could lead to kernel warnings, system instability, or deadlocks.

> +
> +static void lt7911exc_atomic_disable(struct drm_bridge *bridge, struct drm_atomic_state *state)
> +{
> +	/* Delay after panel is disabled */
> +	msleep(20);
> +}

Similar to the above, does calling msleep() here violate atomic context 
constraints?

> +
> +static void lt7911exc_atomic_post_disable(struct drm_bridge *bridge, struct drm_atomic_state *state)
> +{
> +	struct lt7911exc *lt7911exc = bridge_to_lt7911exc(bridge);
> +
> +	guard(mutex)(&lt7911exc->ocm_lock);
> +
> +	gpiod_set_value_cansleep(lt7911exc->reset_gpio, 1);
> +}

If the chip is placed in hardware reset here via gpiod_set_value_cansleep(), 
will subsequent I2C transfers in lt7911exc_dsi_host_transfer() fail?

During the next display enable sequence, the panel's pre_enable will 
likely send DCS commands before the bridge's atomic_pre_enable deasserts 
the reset, leaving the chip physically in reset during I2C communication.

[ ... ]
> +static ssize_t lt7911exc_dsi_host_transfer(struct mipi_dsi_host *host,
> +					   const struct mipi_dsi_msg *msg)
> +{
> +	struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host);
> +	unsigned int val;
> +	int ret;
> +
> +	guard(mutex)(&lt7911exc->ocm_lock);

Should this use lt7911exc_lock() instead of guarding ocm_lock directly?

lt7911exc_lock() is explicitly defined to protect register accesses by 
halting the on-chip MCU. Bypassing this sequence might lead to I2C 
collisions with the running firmware.

> +
> +	/* ensure eDP input video ready */
> +	ret = regmap_read_poll_timeout(lt7911exc->regmap, 0xe084, val, (val & 0x01), 5000, 50000);
> +	if (ret) {
> +		dev_warn(lt7911exc->dev, "Video source not ready within 50ms\n");
> +		return -EBUSY;
> +	}

Will this polling loop always time out during panel initialization?

The downstream panel typically sends initialization DCS commands during its 
pre_enable phase, which runs before the upstream eDP encoder's enable 
callback. Since the eDP video source is inactive at this stage, this 
might permanently break the display initialization pipeline.

> +
> +	/* enable mipi dsi output stream and firmware will send dcs */
> +	ret = regmap_write(lt7911exc->regmap, 0xe0b0, 0x01);
> +	if (ret)
> +		return ret;
> +
> +	return msg->tx_len;
> +}

Could returning msg->tx_len for DCS read operations (where msg->rx_len > 0) 
lead to uninitialized memory being used by the caller?

Because the function returns a positive success value without actually 
populating msg->rx_buf, the calling panel driver might assume the read was 
successful and consume uninitialized stack or heap memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515080934.9870-1-syyang@lontium.com?part=2