Re: [PATCH v4 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: qcom-bcl: IRQ storm on teardown due to devres LIFO ordering
- [Medium] hwmon: qcom-bcl: leaked wake source due to devres LIFO race with delayed work
- [Medium] hwmon: qcom-bcl: max_mu limits for SMB2360/SMB2370 exceed L1/L2 threshold register capacity
--

commit 89d1ce4f2b9b2e93fca41d6caf2cfb094e5b9f02
Author: Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>

hwmon: Add Qualcomm PMIC BCL driver

This commit adds a hardware monitoring driver for the Qualcomm SPMI PMIC
Battery Current Limiting (BCL) peripheral. It provides real-time voltage and
current readings, configurable threshold-based alarms, and automatic
threshold management with polling-based recovery.

> diff --git a/drivers/hwmon/qcom-bcl-hwmon.c b/drivers/hwmon/qcom-bcl-hwmon.c
> --- /dev/null
> +++ b/drivers/hwmon/qcom-bcl-hwmon.c
[ ... ]
> +static const struct bcl_desc smb2360_data = {
> +	.reg_fields = bcl_smb2360_reg_fields,
> +	.num_reg_fields = F_CURR_INPUT + 1,
> +	.data_field_bits_size = 8,
> +	.thresh_field_bits_size = 8,
> +	.battery_config_field = REG_FIELD(0x2a50, 0, 1),
> +	.in_mon_enabled = true,
> +	.in_input_enabled = true,
> +	.curr_mon_enabled = true,
> +	.channel_cfg[BCL_BATT_2S][CHANNEL_IN] = {
> +		.base_mu = 4500,
> +		.max_mu = 8400,
> +		.step_mu = 50,

[Severity: Medium]
Does this maximum threshold value exceed the capacity of the L1 and L2
registers?

The L1 and L2 registers use a 6-bit index field, which limits the maximum
value to 63. For BCL_BATT_2S, this gives a maximum voltage of 7650 mV
(4500 base + 63 * 50 step).

Since max_mu is 8400, if userspace tries to set a valid value like 8000 mV,
the computed index will exceed the field mask and be rejected by
bcl_validate_threshold_fits(), resulting in an unexpected -EINVAL error.

[ ... ]
> +static int bcl_alarm_irq_init(struct platform_device *pdev,
> +			      struct bcl_device *bcl)
> +{
[ ... ]
> +		ret = devm_request_threaded_irq(&pdev->dev, irq_num, NULL,
> +						bcl_handle_alarm, IRQF_ONESHOT,
> +						bcl_int_names[i], alarm);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Register the work cleanup action right after the IRQ is
> +		 * requested, before any other fallible step in this loop, so
> +		 * a delayed work scheduled by the IRQ handler is guaranteed to
> +		 * be canceled on teardown even if the wake cleanup registration
> +		 * below fails. Keeping it after the IRQ request also preserves
> +		 * safe devm LIFO teardown order.
> +		 */
> +		ret = devm_add_action_or_reset(&pdev->dev, bcl_alarm_work_cleanup_action,
> +					       alarm);

[Severity: High]
Is it possible for an endless IRQ storm to occur during teardown?

Because devres uses LIFO ordering, the work cleanup action runs before the
IRQ is freed, which sets shutting_down to true. If the hardware asserts the
interrupt in this window, bcl_handle_alarm() will return IRQ_HANDLED without
calling disable_irq_nosync().

Could this leave the level-triggered interrupt unmasked and cause it to
continuously fire in a loop?

> +		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);

[Severity: Medium]
Could this lead to a leaked wake source during driver unbind?

The bcl_alarm_wake_cleanup_action is registered after the work cleanup action,
meaning devres will execute it first and disable the wake source. However, the
delayed polling work hasn't been canceled yet.

If bcl_alarm_enable_poll() executes concurrently in this window, it will see
shutting_down is still false and could call enable_irq_wake() again, leaving
the wake source permanently enabled after the work is finally canceled.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-qcom-bcl-hwmon-v4-0-50b9a8c730b7@oss.qualcomm.com?part=2