Re: [PATCH 2/2] thermal: qcom: add PM8901 PMIC temperature-alarm driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware auto-shutdown override is left enabled if probe fails, permanently disabling PMIC thermal protection.
- [Low] The calculation for the initial uninitialized temperature computes the lower bound of the stage, contradicting the comment that claims it picks the midpoint.
- [Low] Lockless read of shared state variables in `dev_info` during probe.
--

commit 4441da30420e5466a8731b177283d08a1edfc099
Author: Herman van Hazendonk <github.com@herrie.org>

thermal: qcom: add PM8901 PMIC temperature-alarm driver

This commit adds a thermal-of sensor driver for the temperature-alarm block
inside the Qualcomm PM8901 PMIC. It registers interrupts and decodes stages
into temperatures for board thermal zones.

> diff --git a/drivers/thermal/qcom/qcom-pm8901-tm.c b/drivers/thermal/qcom/qcom-pm8901-tm.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/thermal/qcom/qcom-pm8901-tm.c
[ ... ]
> +/*
> + * Decode the (stage, threshold) pair into a single millicelsius value.
> + * Logic matches the legacy pmic8901-tm.c hysteresis selection:
> + *  - on a rising stage transition, use the lower bound of the new stage
> + *    plus +HYSTERESIS so we don't bounce
> + *  - on a falling stage transition, use the upper bound of the new stage
> + *    minus -HYSTERESIS
> + *  - on the first read after probe (initialised == false), pick a
> + *    representative point: midpoint of the stage range, or
> + *    PM8901_TEMP_NO_ALARM when stage == 0.
> + */
> +static int pm8901_tm_update_temp_locked(struct pm8901_tm_chip *chip)
> +{
[ ... ]
> +	if (!chip->initialised) {
> +		if (new_stage)
> +			chip->temp = PM8901_TEMP_THRESH_MIN +
> +				     chip->thresh * PM8901_TEMP_THRESH_STEP +
> +				     (new_stage - 1) * PM8901_TEMP_STAGE_STEP;

[Severity: Low]
This isn't a bug, but the comment above states this block picks the "midpoint
of the stage range". Does this formula actually compute the exact lower bound
of the stage instead?

> +		else
> +			chip->temp = PM8901_TEMP_NO_ALARM;
> +		chip->initialised = true;
[ ... ]
> +static int pm8901_tm_init_hw(struct pm8901_tm_chip *chip)
> +{
[ ... ]
> +	/*
> +	 * Enable software override so PMIC does NOT auto-shut-down on stage 3.
> +	 * Critical-trip orderly_poweroff is delivered by the kernel thermal
> +	 * core via the DT thermal-zone trip with type = "critical".
> +	 */
> +	reg = (reg & ~(CTRL_OVRD_MASK | CTRL_STATUS_MASK | CTRL_THRESH_MASK)) |
> +	      CTRL_OVRD_ST3 | CTRL_OVRD_ST2;
> +	ret = pm8901_tm_write_ctrl(chip, reg);
> +	if (ret)
> +		goto out;
[ ... ]
> +static int pm8901_tm_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = pm8901_tm_init_hw(chip);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "hw init failed\n");
> +
> +	chip->tz_dev = devm_thermal_of_zone_register(&pdev->dev, 0, chip,
> +						     &pm8901_tm_zone_ops);
> +	if (IS_ERR(chip->tz_dev))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(chip->tz_dev),
> +				     "thermal zone register failed\n");

[Severity: High]
Because pm8901_tm_init_hw() disables the PMIC's hardware auto-shutdown, if a
subsequent initialization step like devm_thermal_of_zone_register() or IRQ
registration fails, probe() exits with an error.

Since there is no devm action or cleanup path to restore the hardware state
on failure, does this leave the system running with no hardware or software
thermal protection?

> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq_alarm, NULL,
> +					pm8901_tm_isr, IRQF_ONESHOT,
> +					"pm8901-tm-alarm", chip);
[ ... ]
> +	platform_set_drvdata(pdev, chip);
> +	thermal_zone_device_update(chip->tz_dev, THERMAL_EVENT_UNSPECIFIED);
> +
> +	dev_info(&pdev->dev,
> +		 "PM8901 thermal alarm: base=0x%x stage=%u thresh=%u temp=%d\n",
> +		 chip->base, chip->stage, chip->thresh, chip->temp);

[Severity: Low]
Since IRQs and the thermal core polling are fully enabled at this point,
could an interrupt or thermal zone update execute concurrently and modify
chip->stage, chip->thresh, or chip->temp?

Does reading these variables here without holding chip->lock risk printing
torn or inconsistent values?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780148149.git.github.com@herrie.org?part=2