Re: [PATCH 1/9] backlight: qcom-wled: fix unbalanced OVP interrupt enable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] backlight: ABBA deadlock between wled->lock and ovp_irq handler
- [High] backlight: missing cancel_delayed_work_sync in wled_probe() error path
--

commit f07dcfad88603020539ad65ef60f36337093bf41
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

    backlight: qcom-wled: fix unbalanced OVP interrupt enable
    
    If the bootloader leaves the WLED module enabled, the OVP interrupt is
    left enabled at probe time. The first brightness update then schedules
    wled_ovp_work(), which enables the interrupt for a second time and
    triggers the "Unbalanced enable for IRQ" warning.
    
    Track whether the driver has disabled the interrupt and only enable it
    back in that case.
    
    Fixes: 8663c188beea ("backlight: qcom-wled: Add auto string detection logic")
    Assisted-by: LLM
    Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 650dd95f06ef5..f2e0430cccbca 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
[ ... ]
> @@ -321,8 +326,11 @@ static int wled_module_enable(struct wled *wled, int val)
>  			 */
>  			schedule_delayed_work(&wled->ovp_work, HZ / 100);

[Severity: High]
Could this delayed work execution lead to a use-after-free if probe fails?

In wled_probe(), INIT_DELAYED_WORK(&wled->ovp_work, wled_ovp_work) initializes
the delayed work without registering a devm action for cancellation.
wled_probe() then requests the short IRQ, making it active.

If a short IRQ triggers during probe, its handler calls wled_module_enable()
which reaches here and globally schedules ovp_work.

If wled_probe() subsequently returns an error (e.g., if
devm_backlight_device_register() fails), devres automatically frees the wled
structure without cancelling the scheduled work. When ovp_work executes, won't
it dereference the freed wled structure?

>  		} else {
> -			if (!cancel_delayed_work_sync(&wled->ovp_work))
> +			cancel_delayed_work_sync(&wled->ovp_work);
> +			if (!wled->ovp_irq_disabled) {
>  				disable_irq(wled->ovp_irq);

[Severity: High]
Does this code introduce an ABBA deadlock with the OVP IRQ handler?

If wled_update_status() or wled_short_irq_handler() disables the module, it
holds wled->lock and calls wled_module_enable(wled, false), which reaches here
and blocks in disable_irq() waiting for any currently executing OVP IRQ handler
to complete.

Meanwhile, the threaded OVP IRQ handler tries to acquire wled->lock to perform
auto string detection:

drivers/video/backlight/qcom-wled.c:wled_ovp_irq_handler() {
    ...
	if (fault_sts & WLED3_CTRL_REG_OVP_FAULT_BIT) {
		if (wled->wled_auto_detection_required(wled)) {
			mutex_lock(&wled->lock);
			wled_auto_string_detection(wled);
			mutex_unlock(&wled->lock);
		}
	}
    ...
}

If the OVP IRQ fires concurrently, will disable_irq() wait forever while the
handler waits for wled->lock?

> +				wled->ovp_irq_disabled = true;
> +			}
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-sm8150-display-card-v1-0-5c14928cef29@oss.qualcomm.com?part=1