Re: [PATCH 2/2] drm/bridge: adv7511: handle regcache_sync() failure on power on

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Incomplete error handling in `adv7511_power_on()` leaves state out of sync

Pre-existing issues:
- [High] Missing `cancel_work_sync()` in removal path causes Use-After-Free
- [Low] I2C device ID array uses positional initializers
--

--- Patch [2]: [PATCH 2/2] drm/bridge: adv7511: handle regcache_sync() failure on power on ---
commit 5725e03d2519d4fb324dba03e70e2f0824a5bd67
Author: bui duc phuc <phucduc.bui@gmail.com>

drm/bridge: adv7511: handle regcache_sync() failure on power on

The commit adds error handling for regcache_sync() in adv7511_power_on().
If it fails, it logs an error and attempts to power off the chip.

> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index db55423781ea9..452180a95c384 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -367,12 +367,20 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>  
>  static void adv7511_power_on(struct adv7511 *adv7511)
>  {
> +	int ret;
> +
>  	__adv7511_power_on(adv7511);
>  
>  	/*
>  	 * Most of the registers are reset during power down or when HPD is low.
>  	 */
> -	regcache_sync(adv7511->regmap);
> +	ret = regcache_sync(adv7511->regmap);
> +	if (ret) {
> +		dev_err(&adv7511->i2c_main->dev,
> +			"Failed to sync register cache: %d\n", ret);
> +		__adv7511_power_off(adv7511);
> +		return;
> +	}

[Severity: High]
Does this early return leave the hardware state out of sync?

If adv7511_power_on() is called when the device was already logically
powered, this early return bypasses setting adv7511->powered = false and
skips calling adv7533_dsi_power_off().

Could this leave the DSI hardware powered on while the software state
remains powered == true, causing subsequent EDID reads to fail because
they see powered == true and attempt to read from a chip where the
rest of the components are powered off?

>  
>  	if (adv7511->info->has_dsi)
>  		adv7533_dsi_power_on(adv7511);

While reviewing this, I noticed a pre-existing issue in the removal path:

> static void adv7511_remove(struct i2c_client *i2c)
> {
> 	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);

[Severity: High]
This isn't a bug introduced by this patch, but does this function need to
cancel the hotplug work item?

If a hotplug interrupt fires and schedules adv7511->hpd_work just before or
during adv7511_remove(), the work could run after the memory is freed by
devres, resulting in a use-after-free.

Would adding cancel_work_sync(&adv7511->hpd_work) here prevent this?

I also noticed another pre-existing issue in the I2C device ID array:

> static const struct i2c_device_id adv7511_i2c_ids[] = {
> 	{ "adv7511", (kernel_ulong_t)&adv7511_chip_info },
> 	{ "adv7511w", (kernel_ulong_t)&adv7511_chip_info },

[Severity: Low]
This is a pre-existing issue, but the I2C subsystem guidelines state that
initialized arrays of type struct i2c_device_id must use named initializers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724100803.50169-1-phucduc.bui@gmail.com?part=2