Re: [PATCH v3 02/12] hwmon: Add Qualcomm PMIC BCL driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] hwmon: use-after-free in delayed work due to missing cleanup on probe error
- [Medium] hwmon: potential torn read of 16-bit volatile ADC values
- [Low] hwmon: incorrect rounding of negative ADC current readings
--

commit 582b6b399b20e883c83628558dc27f1dcb1c1d4d
Author: Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>
Subject: hwmon: Add Qualcomm PMIC BCL driver

Add driver for Qualcomm SPMI PMIC Battery Current Limiting (BCL)
hardware monitor. The driver exposes battery voltage and current
monitoring through hwmon interface.

> diff --git a/drivers/hwmon/qcom-bcl-hwmon.c b/drivers/hwmon/qcom-bcl-hwmon.c
> new file mode 100644
> index 0000000000000..eb6bfa7ff18f5
> --- /dev/null
> +++ b/drivers/hwmon/qcom-bcl-hwmon.c
[ ... ]
> +static int bcl_convert_raw_to_milliunit(const struct bcl_device *bcl,
> +					s32 raw_val,
> +					enum bcl_channel type,
> +					u8 field_width)
> +{
> +	const struct bcl_desc *desc = bcl->desc;
> +	u32 def_scale = desc->channel_cfg[bcl->batt_config][type].default_scale_nu;
> +	u32 scaling_factor = (field_width > 8) ? def_scale : (def_scale << field_width);
> +
> +	return div_s64((s64)raw_val * scaling_factor + 500000, 1000000);

[Severity: Low]
Does this math correctly handle negative current values?

When the hardware provides a negative ADC value for current (e.g., during
battery discharging), it is sign-extended in bcl_curr_input_read() and
passed here. The arithmetic unconditionally adds 500000 before performing
integer division (div_s64). For negative dividends, adding a positive offset
shifts the value towards positive infinity, and C integer division truncates
towards zero, causing the hwmon statistics to be off by 1 unit.

Could this conditionally subtract 500000 for negative dividends to ensure
mathematically symmetric rounding?

[ ... ]
> +static int bcl_curr_input_read(struct bcl_device *bcl, long *out)
> +{
> +	int ret;
> +	u32 raw_val = 0, msb = 0;
> +	s32 signed_val;
> +	const struct bcl_desc *desc = bcl->desc;
> +
> +	/* Return cached value if read too soon after last update */
> +	if (bcl->last_curr_updated &&
> +	    time_before(jiffies, bcl->last_curr_updated + HZ)) {
> +		*out = bcl->last_curr_input;
> +		return 0;
> +	}
> +
> +	ret = regmap_field_read(bcl->fields[F_CURR_INPUT], &raw_val);
> +	if (ret)
> +		return ret;
> +
> +	/* For 16-bit data, read MSB and combine with LSB */
> +	if (desc->data_field_bits_size == 16) {
> +		ret = regmap_field_read(bcl->fields[F_CURR_INPUT1], &msb);

[Severity: Medium]
Is there a risk of a torn read when fetching this 16-bit volatile ADC value?

These sequential reads can capture hardware registers in an inconsistent
state if the PMIC hardware updates the ADC registers exactly between the two
independent bus transactions. Since the mutex lock only serializes software
threads, a hardware update right between these calls would result in reading a
mix of old and new bytes (e.g., an LSB of 0xFF and an MSB of 0x01).

Should this use an atomic regmap_bulk_read() for 16-bit data fields to
prevent wildly incorrect sensor readings?

> +		if (ret)
> +			return ret;
> +		raw_val |= FIELD_PREP(GENMASK(15, 8), msb);
> +	}
[ ... ]
> +static int bcl_alarm_irq_init(struct platform_device *pdev,
> +			      struct bcl_device *bcl)
> +{
> +	int ret, irq_num, i;
> +	struct bcl_alarm_data *alarm;
> +
> +	for (i = 0; i < ARRAY_SIZE(bcl->bcl_alarms); i++) {
> +		alarm = &bcl->bcl_alarms[i];
[ ... ]
> +		ret = devm_request_threaded_irq(&pdev->dev, irq_num, NULL,
> +						bcl_handle_alarm, IRQF_ONESHOT,
> +						bcl_int_names[i], alarm);
> +		if (ret)
> +			return ret;
> +
> +		if (!enable_irq_wake(irq_num))
> +			alarm->irq_wake_enabled = true;
> +
> +		ret = devm_add_action_or_reset(&pdev->dev, bcl_alarm_wake_cleanup_action,
> +					       alarm);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(&pdev->dev, bcl_alarm_work_cleanup_action,
> +					       alarm);

[Severity: High]
Could this lead to a use-after-free if device probe fails under memory
pressure?

The IRQ is activated by devm_request_threaded_irq() and can immediately
schedule the delayed work. If the very next devm_add_action_or_reset()
fails, the driver returns early without registering
bcl_alarm_work_cleanup_action.

If the IRQ fires in this brief window and schedules the delayed work, the
work will not be safely canceled during the devm teardown sequence and will
execute after the bcl_device memory is freed.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-qcom-bcl-hwmon-v3-0-f5ca00889cf7@oss.qualcomm.com?part=2