Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] regulator: qcom-lcdb: LDO pull-down left forced on after disable error - [High] regulator: qcom-lcdb: self-deadlock in short-circuit notifier chain - [High] regulator: qcom-lcdb: state desync on SC recovery failure - [Medium] regulator: qcom-lcdb: internal state corrupted on hardware write failure --
commit 5e78a1b6d6d6c5c217f997a1d7627dc506e8ba37 Author: Dmitry Baryshkov <[email protected]> regulator: add Qualcomm LCDB driver Adds a driver for the LCDB block found on Qualcomm PMICs (PMI632, PM6150L, PM8150L) to provide positive and negative bias rails for LCD panels. The driver supports enabling with voltage ramp-up and disabling with an LDO pull-down. > diff --git a/drivers/regulator/qcom-lcdb-regulator.c > b/drivers/regulator/qcom-lcdb-regulator.c > new file mode 100644 > index 0000000000000..45b3b7118e31d > --- /dev/null > +++ b/drivers/regulator/qcom-lcdb-regulator.c [ ... ] > +static int qcom_lcdb_module_disable(struct qcom_lcdb *lcdb) > +{ > + int ret; > + > + /* Force the LDO pull-down to discharge the output quickly */ > + ret = regmap_write(lcdb->regmap, lcdb->base + LCDB_LDO_FORCE_PD_CTL, > + LCDB_LDO_FORCE_PD_EN | LCDB_LDO_FORCE_PD_MODE); > + if (ret) > + return ret; > + > + ret = regmap_write(lcdb->regmap, lcdb->base + LCDB_ENABLE_CTL1, 0); > + if (ret) > + return ret; [Severity: High] If the regmap_write() to LCDB_ENABLE_CTL1 fails, the function returns early without reverting the prior LCDB_LDO_FORCE_PD_CTL write. Will this leave the physical regulator shorted to ground with the strong pull-down resistor active while the LDO remains enabled? > + > + lcdb->off_time = ktime_get(); > + > + usleep_range(10000, 11000); > + > + return regmap_write(lcdb->regmap, lcdb->base + LCDB_LDO_FORCE_PD_CTL, > 0); > +} [ ... ] > +static int qcom_lcdb_disable(struct regulator_dev *rdev) > +{ > + struct qcom_lcdb *lcdb = rdev_get_drvdata(rdev); > + > + guard(mutex)(&lcdb->lock); > + > + lcdb->enabled_mask &= ~BIT(rdev_get_id(rdev)); > + if (lcdb->enabled_mask) > + return 0; > + > + return qcom_lcdb_module_disable(lcdb); > +} [Severity: Medium] Does clearing lcdb->enabled_mask before qcom_lcdb_module_disable() completes permanently desynchronize the software state if the hardware disablement fails? If the write fails, the mask will remain cleared, causing qcom_lcdb_is_enabled() to return false even though the hardware is still active. This misalignment might cause the next enable call to incorrectly execute the ramp-up sequence on an already active rail. [ ... ] > +static int qcom_lcdb_set_voltage_sel(struct regulator_dev *rdev, unsigned > int sel) > +{ > + struct qcom_lcdb *lcdb = rdev_get_drvdata(rdev); > + > + guard(mutex)(&lcdb->lock); > + > + lcdb->uV[rdev_get_id(rdev)] = qcom_lcdb_sel_to_uV(sel); > + > + return qcom_lcdb_write_voltages(lcdb, lcdb->uV[LCDB_LDO], > + lcdb->uV[LCDB_NCP]); > +} [Severity: Medium] If qcom_lcdb_write_voltages() fails, the cached lcdb->uV array will already have been updated. Will this cause subsequent calls to qcom_lcdb_get_voltage_sel() to return the new but unapplied voltage instead of the actual hardware state? [ ... ] > +static void qcom_lcdb_handle_sc(struct qcom_lcdb *lcdb) > +{ > + int i; > + > + guard(mutex)(&lcdb->lock); > + > + if (!lcdb->enabled_mask || lcdb->sc_disabled) > + return; > + > + qcom_lcdb_module_disable(lcdb); > + > + if (ktime_us_delta(ktime_get(), lcdb->sc_time) > > LCDB_SC_RESET_CNT_DLY_US) { > + lcdb->sc_count = 0; > + } else if (lcdb->sc_count > LCDB_SC_CNT_MAX) { > + dev_err(lcdb->dev, "persistent short circuit, keeping the > outputs off\n"); > + lcdb->sc_disabled = true; > + > + for (i = 0; i < LCDB_NUM_RAILS; i++) > + regulator_notifier_call_chain(lcdb->rdev[i], > + REGULATOR_EVENT_FAIL, > NULL); > + return; > + } [Severity: High] Can calling the synchronous notifier chain with lcdb->lock held result in a self-deadlock? Consumers often react to REGULATOR_EVENT_FAIL by attempting to disable the failing regulator via regulator_disable(). This acquires rdev->mutex and then invokes the driver's disable callback: regulator_disable() qcom_lcdb_disable() guard(mutex)(&lcdb->lock); Since lcdb->lock is already held by qcom_lcdb_handle_sc(), wouldn't this result in a self-deadlock? > + > + lcdb->sc_count++; > + lcdb->sc_time = ktime_get(); > + > + if (qcom_lcdb_module_enable(lcdb)) > + dev_err(lcdb->dev, "failed to recover from a short circuit\n"); > +} [Severity: High] If qcom_lcdb_module_enable() fails here, it returns an error and the hardware is left off, but lcdb->enabled_mask is not cleared and lcdb->sc_disabled is not set. Does this create a state mismatch where future consumers might call qcom_lcdb_enable() and falsely see a success without any actual physical hardware enablement? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
