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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Ignoring regulator_disable() failure causes a permanent regulator reference leak and hardware state corruption.

Pre-existing issues:
- [High] I2C over AUX reads and writes silently ignore timeout and NACK errors, returning success with garbage data.
- [High] Device power state functions lack synchronization, leading to race conditions between extcon events and system suspend.
- [High] Background works are not cancelled during system suspend, risking I/O operations on powered-down hardware.
- [High] Regulators are disabled in the same order they are enabled, violating power sequencing constraints.
--

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

drm/bridge: it6505: complete poweroff even if disabling regulators fails

This patch modifies the driver to continue the power-off sequence and set
the powered state to false even if regulator_disable() fails. This aims
to prevent the bridge from being left deaf to hotplug interrupts by ensuring
the state transition completes.

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

[Severity: High]
This is a pre-existing issue, but does disabling the regulators in the same
order they were enabled violate power sequencing constraints?

In it6505_poweron(), pwr18 is enabled before ovdd. Here in
it6505_poweroff(), pwr18 is disabled before ovdd.

Could removing core power before I/O power cause back-powering and latch-up
in CMOS logic, potentially damaging the hardware?

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

[Severity: High]
If regulator_disable() fails (for example, due to an I2C timeout on a PMIC),
does ignoring the error here cause a permanent regulator reference leak?

When the regulator core returns an error during disable, it does not
decrement the consumer's enable_count.

Since this patch ignores the error and forces it6505->powered to false, the
next power-on will increment the enable_count from 1 to 2.

Because the count never reaches 0 again, will the regulator core permanently
leave the hardware powered on?

[ ... ]

While reviewing this driver, I also noticed a few other pre-existing issues:

it6505_aux_i2c_readb() {
    for (retry = 0; retry < AUX_I2C_DEFER_RETRY; retry++) {
        // ...
        if (ret >= 0)
            break;
    }

    for (i = 0; i < size; i++)
        buf[i] = it6505_read(it6505, REG_AUX_USER_RXB(0 + i));

    return size;
}

[Severity: High]
This is a pre-existing issue, but does this I2C read silently ignore timeout
and NACK errors?

When an AUX I2C transfer encounters a NACK or timeout, it6505_aux_i2c_wait()
returns a negative error code.

If the retry loop exhausts all retries without breaking, the function
unconditionally executes the register read loop and returns the requested
size as if the transfer succeeded.

Can this result in the DRM core receiving uninitialized memory or garbage
data instead of an error, leading to corrupted EDID parsing?


it6505_extcon_work() {
    // ...
    if (ret < 0)
        it6505_poweron(it6505);
}

[Severity: High]
This is a pre-existing issue, but do the device power state functions lack
synchronization?

During system suspend, pm_runtime_force_suspend() calls
it6505_bridge_suspend(), which invokes it6505_poweroff() without acquiring
extcon_lock.

Concurrently, if an extcon event triggers it6505_extcon_work() on the
non-freezable system_wq, it attempts pm_runtime_get_sync().

If runtime PM is disabled during suspend, this fails and the fallback path
directly calls it6505_poweron().

Could this race condition lead to unbalanced IRQ enable depths or corrupted
regulator states?


it6505_bridge_suspend() {
    // ...
    it6505_remove_edid(it6505);
    return it6505_poweroff(it6505);
}

[Severity: High]
This is a pre-existing issue, but does the suspend handler fail to cancel
background works before powering off the hardware?

it6505_bridge_suspend() calls it6505_poweroff() but does not call
cancel_work_sync() for link_works or extcon_wq.

Because these works are scheduled on the non-freezable system_wq, could
they execute concurrently during the suspend process and attempt I/O
operations on powered-down hardware?

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