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

[Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>]

Hi

On 9/29/2026 6:51 PM, sashiko-bot@kernel.org wrote:
> 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?

I will use DIV_S64_ROUND_CLOSEST instead of the biased 
add-500000-then-truncate pattern in next revision.

> 
> [ ... ]
>> +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?

I will update regmap_bulk_read() for the 16-bit LSB/MSB pair.

> 
>> +		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.

ACK, will update

Thanks,
Manaf

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