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 <[email protected]> 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/[email protected]?part=1
