Re: [PATCH 6/9] arm64: dts: qcom: pm8150l: add LCDB regulators

[sashiko-bot@kernel.org]

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 <dmitry.baryshkov@oss.qualcomm.com>

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/20261015-sm8150-display-card-v1-0-5c14928cef29@oss.qualcomm.com?part=6