Re: [PATCH 5/9] regulator: add Qualcomm LCDB driver

[sashiko-bot@kernel.org]

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

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