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