Re: [PATCH v3 2/2] thermal: qcom: Add support for Qualcomm MBG thermal monitoring

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent sysfs temperature reads consume the faked thermal trip state, preventing the thermal core from mitigating threshold violations.
- [Medium] The driver logs a dev_err and returns -ERANGE when the thermal framework attempts to disable a trip point by passing INT_MAX.
--

--- Patch [2]: [PATCH v3 2/2] thermal: qcom: Add support for Qualcomm MBG thermal monitoring ---
commit 9dc7e944c7be51e8b177c47f01252fb99daf9ee2
Author: Satya Priya Kakitapalli <[email protected]>

thermal: qcom: Add support for Qualcomm MBG thermal monitoring

Add a driver for the Qualcomm MBG (Master Bandgap) thermal monitoring
device. It monitors PMIC die temperature in hardware and raises an
interrupt when a programmed threshold is crossed.

> diff --git a/drivers/thermal/qcom/qcom-spmi-mbg-tm.c b/drivers/thermal/qcom/qcom-spmi-mbg-tm.c
> new file mode 100644
> index 0000000000000..fa2f10002253f
> --- /dev/null
> +++ b/drivers/thermal/qcom/qcom-spmi-mbg-tm.c

[ ... ]

> +static int mbg_tm_set_trip_temp(struct thermal_zone_device *tz, int low_temp,
> +				int temp)
> +{
> +	struct mbg_tm_chip *chip = thermal_zone_device_priv(tz);
> +	int ret = 0;
> +
> +	guard(mutex)(&chip->lock);
> +
> +	/* The HW has a limitation that the trip set must be above 25C */
> +	if (temp > MBG_MIN_TRIP_TEMP && temp < MBG_MAX_SUPPORTED_TEMP) {
> +		ret = regmap_write(chip->map, chip->base + MON2_LVL1_UP_THRESH,
> +				   temp_to_vtemp_mv(temp));
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_set_bits(chip->map, chip->base + MBG_TEMP_MON2_MISC_CFG,
> +				      MON2_UP_THRESH_EN);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		dev_err(chip->dev, "Set trip b/w 25C and 160C\n");
> +		ret = regmap_clear_bits(chip->map, chip->base + MBG_TEMP_MON2_MISC_CFG,
> +					MON2_UP_THRESH_EN);
> +		return -ERANGE;
> +	}

[Severity: Medium]
Will this log a spurious error when the thermal framework attempts to disable
a trip point?

The thermal framework uses INT_MAX to disable the high trip point (e.g., when
the temperature drops below all trips). When mbg_tm_set_trip_temp() receives
INT_MAX, it correctly clears the MON2_UP_THRESH_EN bit, but also logs an
error and returns -ERANGE.

Because thermal zones can be polled frequently, could this result in severe
kernel log spam and cause the thermal core to log failure messages during
normal operation?

[ ... ]

> +static irqreturn_t mbg_tm_isr(int irq, void *data)
> +{
> +	struct mbg_tm_chip *chip = data;
> +	int ret, val;
> +
> +	scoped_guard(mutex, &chip->lock) {
> +		ret = regmap_read(chip->map, chip->base + MBG_TEMP_MON2_FAULT_STATUS, &val);
> +		if (ret < 0)
> +			return IRQ_HANDLED;
> +		if (FIELD_GET(MON_FAULT_STATUS_MASK, val) == MON_FAULT_LVL1_UPR)
> +			chip->last_thres_crossed = true;
> +	}
> +
> +	if (FIELD_GET(MON_FAULT_STATUS_MASK, val) == MON_FAULT_LVL1_UPR) {
> +		dev_dbg(chip->dev, "Notifying Thermal, fault status=%d\n", val);
> +		thermal_zone_device_update(chip->tz_dev, THERMAL_TRIP_VIOLATED);

[Severity: High]
Could a concurrent sysfs read consume the faked thermal trip state and
prevent thermal mitigation?

In mbg_tm_isr(), the lock is dropped before calling
thermal_zone_device_update():

CPU1 (mbg_tm_isr):
	scoped_guard(mutex, &chip->lock) {
		...
		if (FIELD_GET(MON_FAULT_STATUS_MASK, val) == MON_FAULT_LVL1_UPR)
			chip->last_thres_crossed = true;
	} // lock dropped

CPU2 (userspace sysfs read of temp file):
	mbg_tm_get_temp()
		scoped_guard(mutex, &chip->lock) {
			if (chip->last_thres_crossed) {
				chip->last_thres_crossed = false; // consumed
				*temp = chip->last_temp;
				return 0;
			}
		}

CPU1:
	thermal_zone_device_update(chip->tz_dev, THERMAL_TRIP_VIOLATED);

When thermal_zone_device_update() subsequently reads the temperature, it will
see last_thres_crossed = false, read the raw ADC value (which might be below
the threshold due to conversion inaccuracies), and potentially skip thermal
mitigation.

> +	} else {
> +		dev_dbg(chip->dev, "Lvl1 upper threshold not violated, ignoring interrupt\n");
> +	}
> +
> +	return IRQ_HANDLED;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2