Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] regulator: qcom-lcdb: deadlock in qcom_lcdb_handle_sc() during notifier call - [Medium] regulator: qcom-lcdb: hardware state desync on write failure in set_voltage_sel() - [Medium] regulator: qcom-lcdb: state desync breaking subsequent enables after short-circuit --
commit 4cf3328d99c41fa55695e4085137c0f502bbc641 Author: Dmitry Baryshkov <[email protected]> arm64: dts: qcom: pm8150l: add LCDB regulators This commit describes the LCDB block for the LCD bias rails in the pm8150l device tree, enabling the qcom-lcdb-regulator driver on this hardware. > diff --git a/arch/arm64/boot/dts/qcom/pm8150l.dtsi > b/arch/arm64/boot/dts/qcom/pm8150l.dtsi > index 3911d6d0d2e28..f54ddb6f7946f 100644 > --- a/arch/arm64/boot/dts/qcom/pm8150l.dtsi > +++ b/arch/arm64/boot/dts/qcom/pm8150l.dtsi > @@ -141,5 +141,23 @@ pm8150l_wled: leds@d800 { > > status = "disabled"; > }; > + > + pm8150l_lcdb: lcdb@ec00 { > + compatible = "qcom,pm8150l-lcdb"; [Severity: High] Can this result in a deadlock during a failure notifier call? When the LCD bias short-circuits, qcom_lcdb_handle_sc() takes lcdb->lock and makes a blocking notifier call: drivers/regulator/qcom-lcdb-regulator.c:qcom_lcdb_handle_sc() { ... guard(mutex)(&lcdb->lock); ... for (i = 0; i < LCDB_NUM_RAILS; i++) regulator_notifier_call_chain(lcdb->rdev[i], REGULATOR_EVENT_FAIL, NULL); ... } Consumers commonly react to REGULATOR_EVENT_FAIL by invoking regulator_disable(), which invokes qcom_lcdb_disable(). That also attempts to acquire lcdb->lock: drivers/regulator/qcom-lcdb-regulator.c:qcom_lcdb_disable() { ... guard(mutex)(&lcdb->lock); ... } Will this cause a self-deadlock since the lock is already held by the notifier caller? [Severity: Medium] Can a hardware update failure result in a desync between the driver's cached voltage state and the actual hardware state? When changing the voltage, the internal state lcdb->uV is eagerly updated before writing to the hardware: drivers/regulator/qcom-lcdb-regulator.c:qcom_lcdb_set_voltage_sel() { ... 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]); } If qcom_lcdb_write_voltages() fails and returns an error, the cached state in lcdb->uV isn't rolled back. Since future operations rely on both cached voltages to compute the boost voltage (bst_uV), could an unapplied lower voltage cache cause the driver to calculate a boost voltage lower than physically required, leading to a regulator collapse? [Severity: Medium] Does this code silently break subsequent regulator enables after a short circuit recovery fails? In qcom_lcdb_handle_sc(), the hardware is forced off by qcom_lcdb_module_disable(), and then it attempts to recover: drivers/regulator/qcom-lcdb-regulator.c:qcom_lcdb_handle_sc() { ... if (qcom_lcdb_module_enable(lcdb)) dev_err(lcdb->dev, "failed to recover from a short circuit\n"); } If qcom_lcdb_module_enable(lcdb) fails, it emits an error message and returns, but leaves lcdb->enabled_mask untouched and non-zero. If a consumer later tries to re-enable the regulator: drivers/regulator/qcom-lcdb-regulator.c:qcom_lcdb_enable() { ... if (lcdb->enabled_mask) return 0; lcdb->enabled_mask |= BIT(rdev_get_id(rdev)); return qcom_lcdb_module_enable(lcdb); } Since lcdb->enabled_mask is still non-zero, will qcom_lcdb_enable() silently return success without actually enabling the hardware? > + reg = <0xec00>; > + interrupts = <0x5 0xec 0x1 IRQ_TYPE_EDGE_RISING>; > + > + status = "disabled"; > + > + pm8150l_lcdb_ldo: ldo { > + regulator-min-microvolt = <4000000>; > + regulator-max-microvolt = <6000000>; > + }; > + > + pm8150l_lcdb_ncp: ncp { > + regulator-min-microvolt = <4000000>; > + regulator-max-microvolt = <6000000>; > + }; > + }; > }; > }; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
