Re: [PATCH v6 04/13] drm/bridge: it6505: complete poweroff even if disabling regulators fails

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Missing newline (`\n`) in the newly added `dev_err` format strings.

Pre-existing issues:
- [High] The interrupt handler does not check the return value of `it6505_read()` for error codes, causing spurious interrupt triggers.
- [High] The power-off sequence disables the IVDD (`pwr18`) regulator before the OVDD (`ovdd`) regulator, violating the reverse-order power-down requirement.
--

--- Patch [4]: [PATCH v6 04/13] drm/bridge: it6505: complete poweroff even if disabling regulators fails ---
commit 1009be5a50c31876cc2fed6f2166f7843b4776a3
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: complete poweroff even if disabling regulators fails
    
it6505_poweroff() returns early when regulator_disable() fails,
leaving it6505->powered set with the IRQ already disabled. The next
it6505_poweron() then takes its early return and never re-enables the
IRQ, leaving the bridge deaf to hotplug and link training interrupts.
The regulator core keeps the consumer's enable count on a failed
disable either way, so bailing out only adds a wedged bridge on top of
the leaked reference. Log the error and complete the power-off state
transition instead.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index c10cc6a786b5d..fb6030a2c18cb 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2811,13 +2811,15 @@ static int it6505_poweroff(struct it6505 *it6505)
>  	if (pdata->pwr18) {
>  		err = regulator_disable(pdata->pwr18);

[Severity: High]
This isn't a bug introduced by this patch, but does this power-off sequence
violate the reverse-order power-down requirement?

The it6505_poweron() function explicitly enforces pwr18 -> ovdd ordering,
but this disables the core voltage (IVDD/pwr18) before the IO voltage
(OVDD/ovdd).

Could this forward order create reverse current paths through ESD diodes,
leading to hardware latch-up or degradation?

Should we disable ovdd before pwr18 here?

>  		if (err)
> -			return err;
> +			dev_err(dev, "cannot disable pwr18 regulator: %d",
> +				err);

[Severity: Low]
Is this missing a trailing newline?

Without a \n in the format string, subsequent kernel log messages from other
subsystems might be concatenated on the same line, resulting in mangled
dmesg output.

>  	}
>  
>  	if (pdata->ovdd) {
>  		err = regulator_disable(pdata->ovdd);
>  		if (err)
> -			return err;
> +			dev_err(dev, "cannot disable ovdd regulator: %d",
> +				err);

[Severity: Low]
Does this dev_err() also need a trailing newline?

>  	}
>  
>  	it6505->powered = false;

[Severity: High]
This is a pre-existing issue, but does it6505_int_threaded_handler() fail
to check it6505_read() for error codes?

If an I2C read fails due to a transient bus error or occurring during
power-down, it6505_read() returns a negative error code (e.g., -ENODEV).

This negative value is directly assigned to the int_status array:

drivers/gpu/drm/bridge/ite-it6505.c:it6505_int_threaded_handler() {
	int_status[0] = it6505_read(it6505, INT_STATUS_01);
	int_status[1] = it6505_read(it6505, INT_STATUS_02);
	int_status[2] = it6505_read(it6505, INT_STATUS_03);
...
}

In two's complement, negative values have many high bits set. This causes
it6505_test_bit() to incorrectly evaluate to true for multiple random
interrupts.

Could this lead to the spurious execution of handlers and writing back garbage
to the interrupt clear register?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784947240.git.daniel@makrotopia.org?part=4