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

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

[Pengpeng Hou]

adv7511_power_on() replays the register cache after taking the bridge
out of power-down.  If that replay fails, the function still powers on
the DSI side and records the bridge as powered, so hotplug and
atomic-enable paths can continue with incomplete hardware state.

Return the replay error, put the bridge back into power-down, leave its
cache dirty and keep powered clear.  Make the three callers stop their
follow-on work when restoration fails.

The issue was identified via static analysis and manually reviewed.

Fixes: 9c8af882bf12 ("drm: Add adv7511 encoder driver")

Assisted-by: LLM
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 26 ++++++++++++++++----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 038b852405b5..c57c566cf0ad 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);
+		adv7511->powered = false;
+		dev_err(&adv7511->i2c_main->dev,
+			"failed to sync register cache: %d\n", ret);
+		return ret;
+	}
 
 	if (adv7511->info->has_dsi)
 		adv7533_dsi_power_on(adv7511);
 	adv7511->powered = true;
+
+	return 0;
 }
 
 static void __adv7511_power_off(struct adv7511 *adv7511)
@@ -425,7 +438,8 @@ static void adv7511_hpd_work(struct work_struct *work)
 	    adv7511->status == connector_status_disconnected &&
 	    adv7511->powered) {
 		regcache_mark_dirty(adv7511->regmap);
-		adv7511_power_on(adv7511);
+		if (adv7511_power_on(adv7511))
+			return;
 	}
 
 	if (adv7511->status != status) {
@@ -634,7 +648,8 @@ adv7511_detect(struct adv7511 *adv7511)
 	 * has to be reinitialized. */
 	if (status == connector_status_connected && hpd && adv7511->powered) {
 		regcache_mark_dirty(adv7511->regmap);
-		adv7511_power_on(adv7511);
+		if (adv7511_power_on(adv7511))
+			status = connector_status_disconnected;
 		if (adv7511->status == connector_status_connected)
 			status = connector_status_disconnected;
 	} else {
@@ -788,7 +803,8 @@ static void adv7511_bridge_atomic_enable(struct drm_bridge *bridge,
 	struct drm_connector_state *conn_state;
 	struct drm_crtc_state *crtc_state;
 
-	adv7511_power_on(adv);
+	if (adv7511_power_on(adv))
+		return;
 
 	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
 	if (WARN_ON(!connector))
-- 
2.43.0

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