Re: [PATCH v4 06/12] arm64: dts: qcom: pm7550ba-eliza: Enable BCL sensor node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hwmon: qcom-bcl: leaked IRQ wake depth due to cleanup race on unbind
--

commit 5ea386ff99b613c8ed6f6b198a306fbc43b08172
Author: Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>

arm64: dts: qcom: pm7550ba-eliza: Enable BCL sensor node
    
This commit adds the Battery Current Limiting (BCL) hardware monitor node
for pm7550ba-eliza. The BCL node monitors battery voltage and current,
providing hardware interrupts when configurable thresholds are violated.

> diff --git a/arch/arm64/boot/dts/qcom/pm7550ba-eliza.dtsi b/arch/arm64/boot/dts/qcom/pm7550ba-eliza.dtsi
> index 18692893ca418..f0c252a386ee8 100644
> --- a/arch/arm64/boot/dts/qcom/pm7550ba-eliza.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pm7550ba-eliza.dtsi
> @@ -50,6 +50,17 @@ pm7550ba_temp_alarm: temp-alarm@a00 {
>  			#thermal-sensor-cells = <0>;
>  		};
>  
> +		sensor@4700 {
> +			compatible = "qcom,pm7550ba-bcl";

[Severity: High]
Does enabling this sensor node expose an unbind race in the driver?

In drivers/hwmon/qcom-bcl-hwmon.c:bcl_alarm_irq_init(), the cleanup actions
are registered:

	ret = devm_add_action_or_reset(&pdev->dev, bcl_alarm_work_cleanup_action,
				       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 the driver is unbound before an active alarm clears, due to devm LIFO
ordering, bcl_alarm_wake_cleanup_action() executes first. If the IRQ handler
disabled the IRQ wake source and scheduled the poll work,
alarm->irq_wake_enabled is false, so the wake cleanup does nothing.

If the scheduled alarm_poll_work() executes next and sees the alarm cleared,
it successfully re-enables the IRQ wake source and sets
irq_wake_enabled = true.

Then bcl_alarm_work_cleanup_action() executes, cancelling the work but
failing to disable the newly re-enabled IRQ wake source. This leaves the
IRQ wake depth permanently unbalanced, preventing the system from entering
system suspend or deep idle states.

Could the actions be registered in a safe order, perhaps by registering the
wake cleanup first so it runs last, or by combining them into a single
ordered teardown action?

> +			reg = <0x4700>;
> +			interrupts = <0x7 0x47 0x0 IRQ_TYPE_EDGE_RISING>,
> +				     <0x7 0x47 0x1 IRQ_TYPE_EDGE_RISING>,
> +				     <0x7 0x47 0x2 IRQ_TYPE_EDGE_RISING>;
> +			interrupt-names = "max-min",
> +					  "critical",
> +					  "emergency";
> +		};

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