Re: [PATCH] drm: bridge: adv7511: Stop power-on after cache sync failure

Re: [PATCH] drm: bridge: adv7511: Stop power-on after cache sync failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The error path in adv7511_power_on() performs an incomplete rollback of the hardware state compared to the standard power-off sequence.
--

commit 978a99aaf2683acfac949339d63b25eabfcf6074
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

drm: bridge: adv7511: Stop power-on after cache sync failure

This commit updates adv7511_power_on() to handle register cache sync failures.
Instead of ignoring the failure, it attempts to roll back the power state,
marks the cache as dirty, and returns the error to prevent hotplug and
atomic-enable paths from continuing with incomplete hardware state.

> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 038b852405b54..c57c566cf0ad6 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
[ ... ]
> @@ -341,18 +341,31 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>  				   ADV7511_REG_POWER2_HPD_SRC_NONE);
>  }
>  
> -static void adv7511_power_on(struct adv7511 *adv7511)
> +static int 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) {
> +		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> +				   ADV7511_POWER_POWER_DOWN,
> +				   ADV7511_POWER_POWER_DOWN);
> +		regcache_mark_dirty(adv7511->regmap);

[Severity: Medium]
Does this error path perform an incomplete rollback compared to the standard
power-off sequence?

When __adv7511_power_off() is called, it explicitly disables the HPD override
and DDC error interrupts in addition to setting the power down bit:

static void __adv7511_power_off(struct adv7511 *adv7511)
{
	if (adv7511->info->hpd_override_enable)
		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
				   ADV7535_REG_POWER2_HPD_OVERRIDE, 0);

	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
			   ADV7511_POWER_POWER_DOWN,
			   ADV7511_POWER_POWER_DOWN);
	regmap_update_bits(adv7511->regmap,
			   ADV7511_REG_INT_ENABLE(1),
			   ADV7511_INT1_DDC_ERROR, 0);
	regcache_mark_dirty(adv7511->regmap);
}

Since __adv7511_power_on() enables these bits, open-coding the rollback here
without disabling ADV7511_INT1_DDC_ERROR and ADV7535_REG_POWER2_HPD_OVERRIDE
might leave the device in an inconsistent hardware state and lead to spurious
interrupts or incorrect HPD state while purportedly powered down.

Could this error path call __adv7511_power_off() directly, or include the
same bit clearing logic?

> +		adv7511->powered = false;
> +		dev_err(&adv7511->i2c_main->dev,
> +			"failed to sync register cache: %d\n", ret);
> +		return ret;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828082935.82266-1-pengpeng@iscas.ac.cn?part=1