Re: [PATCH] backlight: qcom-wled: Remove redundant dev_err()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Low] Incomplete cleanup of redundant dev_err() after devm_request_threaded_irq().

Pre-existing issues:
- [High] Unconditional calls to disable_irq() during driver removal can use negative or 0 IRQ values, leading to crashes or disabling critical system IRQs.
- [High] Teardown race condition in wled_remove() allows sysfs and IRQ access to a destroyed mutex.
- [Medium] The driver swallows -EPROBE_DEFER from platform_get_irq_byname(), leading to permanent feature loss.
--

--- Patch [1]: [PATCH] backlight: qcom-wled: Remove redundant dev_err() ---
commit d0dd0e4fe4ebc4391de7914a6014d4894f4acea9
Author: Pan Chuang <panchuang@vivo.com>

backlight: qcom-wled: Remove redundant dev_err()

Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_threaded_irq() automatically logs
detailed error messages on failure. Remove the now-redundant
driver-specific dev_err() calls.

> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 8054e4787725e..650dd95f06ef5 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1639,8 +1639,6 @@ static int wled_configure_ovp_irq(struct wled *wled,
>  				       wled_ovp_irq_handler, IRQF_ONESHOT,
>  				       "wled_ovp_irq", wled);
>  	if (rc < 0) {
> -		dev_err(wled->dev, "Unable to request ovp_irq (err:%d)\n",
> -			rc);
>  		wled->ovp_irq = 0;
>  		return 0;
>  	}

[Severity: Low]
Is this cleanup incomplete? Looking at wled_configure_short_irq(), it appears
there is another call to dev_err() after devm_request_threaded_irq() that was
left behind:

drivers/video/backlight/qcom-wled.c:wled_configure_short_irq()
    ...
	rc = devm_request_threaded_irq(wled->dev, wled->short_irq,
				       NULL, wled_short_irq_handler,
				       IRQF_ONESHOT,
				       "wled_short_irq", wled);
	if (rc < 0)
		dev_err(wled->dev, "Unable to request short_irq (err:%d)\n",
			rc);
    ...

[Severity: High]
This isn't a bug introduced by this patch, but could wled_remove() pass
negative or zero IRQ values to disable_irq()?

Looking at wled_remove(), it disables both IRQs unconditionally:

drivers/video/backlight/qcom-wled.c:wled_remove()
    ...
	disable_irq(wled->short_irq);
	disable_irq(wled->ovp_irq);
}

If platform_get_irq_byname() fails, wled->short_irq retains the negative
error code because probe continues. If devm_request_threaded_irq() fails
for ovp_irq, wled->ovp_irq is explicitly set to 0. Would passing these
values to disable_irq() lead to out-of-bounds access or disable system IRQs?

[Severity: Medium]
This is a pre-existing issue, but might the driver permanently lose features
by swallowing -EPROBE_DEFER?

drivers/video/backlight/qcom-wled.c:wled_configure_ovp_irq()
    ...
	wled->ovp_irq = platform_get_irq_byname(pdev, "ovp");
	if (wled->ovp_irq < 0) {
		dev_dbg(&pdev->dev, "OVP IRQ not found ...\n");
		return 0;
	}
    ...

If the interrupt controller driver probes after the backlight driver,
platform_get_irq_byname() returns -EPROBE_DEFER. Instead of deferring,
the driver treats any negative value as missing and proceeds without the IRQ.
The same pattern exists in wled_configure_short_irq().

[Severity: High]
This is a pre-existing issue, but is there a use-after-destroy race condition
during driver unbind?

drivers/video/backlight/qcom-wled.c:wled_remove()
{
	struct wled *wled = platform_get_drvdata(pdev);

	mutex_destroy(&wled->lock);
	cancel_delayed_work_sync(&wled->ovp_work);
	disable_irq(wled->short_irq);
	disable_irq(wled->ovp_irq);
}

Since devm_backlight_device_register() is used, the sysfs interface remains
active until devres cleanup, which happens after wled_remove() returns.
A concurrent write to the sysfs brightness attribute can call
wled_update_status(), locking the already destroyed mutex. 

Additionally, hardware IRQs are disabled after mutex_destroy(), so an
in-flight OVP interrupt could trigger wled_ovp_irq_handler() and also lock
the destroyed mutex.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722065709.346072-1-panchuang@vivo.com?part=1