[PATCH 1/2] drm/bridge: adv7511: Reorder power_on() and power_off()

[PATCH 1/2] drm/bridge: adv7511: Reorder power_on() and power_off()

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Move the power_on function block below the power_off function block
to prepare for the following patch.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 48 ++++++++++----------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 02f8f7e78a16..d98a8f7afc7f 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -302,6 +302,30 @@ static void adv7511_set_link_config(struct adv7511 *adv7511,
 	adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
 }
 
+static void __adv7511_power_off(struct adv7511 *adv7511)
+{
+	/* TODO: setup additional power down modes */
+	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);
+}
+
+static void adv7511_power_off(struct adv7511 *adv7511)
+{
+	__adv7511_power_off(adv7511);
+	if (adv7511->info->has_dsi)
+		adv7533_dsi_power_off(adv7511);
+	adv7511->powered = false;
+}
+
 static void __adv7511_power_on(struct adv7511 *adv7511)
 {
 	adv7511->current_edid_segment = -1;
@@ -355,30 +379,6 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 	adv7511->powered = true;
 }
 
-static void __adv7511_power_off(struct adv7511 *adv7511)
-{
-	/* TODO: setup additional power down modes */
-	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);
-}
-
-static void adv7511_power_off(struct adv7511 *adv7511)
-{
-	__adv7511_power_off(adv7511);
-	if (adv7511->info->has_dsi)
-		adv7533_dsi_power_off(adv7511);
-	adv7511->powered = false;
-}
-
 /* -----------------------------------------------------------------------------
  * Interrupt and hotplug detection
  */
-- 
2.43.0

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

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

regcache_sync() failure in adv7511_power_on() is silently ignored,
leaving the chip marked as powered on even though register
synchronization may not have completed successfully. Log the error
and attempt to power the chip back off on failure, to avoid the
driver believing the chip is fully configured when it may not be.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index d98a8f7afc7f..3e470366bce0 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;
+	}
 
 	if (adv7511->info->has_dsi)
 		adv7533_dsi_power_on(adv7511);
-- 
2.43.0

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

[sashiko-bot]

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