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 <[email protected]> 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 <[email protected]> > 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/[email protected]?part=1
