Re: [PATCH v7 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:
- [Critical] Firmware upgrade via sysfs races with DRM atomic modeset callbacks, risking hardware bricking.
- [High] Hardware reset in `remove` callback races with DRM bridge teardown.
- [Medium] Undefined behavior in CRC validation due to integer promotion.
- [Medium] Incomplete display configuration and missing MIPI DSI host registration.
- [Medium] Unbalanced reset pin state on probe failure creates backpowering risk.
- [Medium] Ignored return values for critical I2C writes during firmware upgrades.
- [Low] `dev_err_probe()` used outside of `probe` context.
--

commit 0bae053b4ef87003ffe3f7c844f0ddb1092f16bf
Author: Sunyun Yang <syyang@lontium.com>
Date:   Tue May 12 06:40:13 2026 +0000

    drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge
    
    This commit adds support for the Lontium LT7911EXC eDP to MIPI DSI
    bridge. It implements I2C-based setup, display configuration, and
    provides a firmware update mechanism.

The commit message states:
> It implements I2C-based setup, display configuration, and
> provides a firmware update mechanism.

Is this description completely accurate? Looking at the drm_bridge_funcs
implementation, there are no atomic_enable/disable functions configuring
the display. Additionally, as an eDP to MIPI DSI bridge, should it also
register a mipi_dsi_host so downstream panels can attach?

> diff --git a/drivers/gpu/drm/bridge/lontium-lt7911exc.c b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lontium-lt7911exc.c
[ ... ]
> +static void lt7911exc_block_erase(struct lt7911exc *lt7911exc)
> +{
[ ... ]
> +	regmap_multi_reg_write(lt7911exc->regmap, seq_write, ARRAY_SIZE(seq_write));
> +
> +	msleep(200);
> +	dev_dbg(dev, "erase flash done.\n");
> +}

Could this silently fail to erase the flash? If regmap_multi_reg_write()
fails due to hardware communication errors, the driver will continue and
attempt to write new firmware data to an unerased flash segment. Should
the return value be checked here and in lt7911exc_prog_init()?

[ ... ]
> +static int lt7911exc_upgrade_result(struct lt7911exc *lt7911exc, u32 crc32)
> +{
[ ... ]
> +	read_hw_crc = crc_tmp[0] << 24 | crc_tmp[1] << 16 |
> +				crc_tmp[2] << 8 | crc_tmp[3];

Can this cause a signed integer overflow? In C, u8 values are implicitly
promoted to signed 32-bit integers before shifting. If crc_tmp[0] is 128
or greater, shifting left by 24 bits will shift into the sign bit. Would
explicitly casting to u32 prevent this?

[ ... ]
> +static int lt7911exc_firmware_upgrade(struct lt7911exc *lt7911exc)
> +{
[ ... ]
> +	/*1. load firmware*/
> +	ret = request_firmware(&fw, FW_FILE, dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to load '%s'\n", FW_FILE);

Is it safe to use dev_err_probe() outside of a probe context? This function
is called exclusively from the sysfs store callback. Calling dev_err_probe()
here might interfere with deferred probe tracking.

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

[ ... ]

> +static ssize_t lt7911exc_firmware_store(struct device *dev, struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
[ ... ]
> +	lt7911exc_lock(lt7911exc);
> +
> +	ret = lt7911exc_firmware_upgrade(lt7911exc);

Can a DRM atomic modeset race with a firmware upgrade here? 

The sysfs attribute protects internal I2C sequences with ocm_lock, but the
DRM bridge callbacks toggle the reset_gpio without acquiring this lock.

If a display modeset occurs concurrently with a firmware flash, could the
hardware reset pin be toggled mid-flash and corrupt the firmware?

[ ... ]
> +static int lt7911exc_probe(struct i2c_client *client)
> +{
[ ... ]
> +	lt7911exc_reset(lt7911exc);
> +
> +	lt7911exc_lock(lt7911exc);
> +
> +	lt7911exc->fw_version = lt7911exc_read_version(lt7911exc);
> +
> +	lt7911exc_unlock(lt7911exc);
> +	if (lt7911exc->fw_version < 0)
> +		return dev_err_probe(dev, lt7911exc->fw_version, "failed read version of chip\n");

If lt7911exc_read_version() fails, does this leave the reset pin in an
unbalanced state? 

The lt7911exc_reset() call leaves the reset_gpio low, and as noted in the
remove function, it should be pulled high to prevent backpowering. Should
the error path assert the reset pin high?

[ ... ]
> +	return devm_drm_bridge_add(dev, &lt7911exc->bridge);
> +}
> +
> +static void lt7911exc_remove(struct i2c_client *client)
> +{
> +	struct lt7911exc *lt7911exc = i2c_get_clientdata(client);
> +
> +	/* pull reset low to prevent backpowering*/
> +	gpiod_set_value_cansleep(lt7911exc->reset_gpio, 1);
> +}

Does this teardown sequence race with DRM operations? 

Because the bridge is registered with devm_drm_bridge_add(), it will be
unregistered during the devres cleanup phase, which happens after the
driver's remove callback has finished.

Since lt7911exc_remove() asserts the reset pin to shut down the hardware,
could concurrent DRM operations attempt to access the disabled hardware
while it is still registered?

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