Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [High] If default-brightness is 0, the OVP IRQ is incorrectly disabled while the hardware module remains active, and the module cannot be subsequently disabled. Pre-existing issues: - [High] Missing platform_set_drvdata() in probe causes a guaranteed NULL pointer dereference in wled_remove(). - [High] wled_remove() calls disable_irq() with negative or uninitialized IRQ values if the IRQs failed to map during probe. - [High] Tearing down synchronization primitives in wled_remove() before the devm-managed backlight is unregistered creates a use-after-free window. - [High] WLED3 devices lack the wled_auto_detection_required function handler, leading to a NULL pointer dereference if the OVP IRQ fires. - [High] The short IRQ handler unconditionally re-enables the hardware module, overriding software intentions. -- commit 5bacd29d54de18ff6f2680d6c09d873e1a8a3c3a Author: David Heidelberg <[email protected]> backlight: qcom-wled: Fix unbalanced OVP IRQ enable at probe This commit fixes an issue where the OVP IRQ was unbalanced at probe. It reads the hardware module enable state to initialize the software brightness level, and keys the OVP IRQ enable off this software state rather than the hardware state to avoid mismatches. > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index 650dd95f06ef5..344b8cad90105 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -1627,7 +1627,6 @@ static int wled_configure_ovp_irq(struct wled *wled, > struct platform_device *pdev) > { > int rc; > - u32 val; > > wled->ovp_irq = platform_get_irq_byname(pdev, "ovp"); > if (wled->ovp_irq < 0) { [Severity: High] This is a pre-existing issue, but if platform_get_irq_byname() fails and returns a negative error code, it is left in wled->ovp_irq without being reset to 0. Could this cause a regression during module removal? drivers/video/backlight/qcom-wled.c:wled_remove() { ... disable_irq(wled->ovp_irq); ... } It appears wled_remove() calls disable_irq() unconditionally, which would result in an out-of-bounds IRQ descriptor lookup or inadvertently disabling a system IRQ if wled->ovp_irq is negative. [ ... ] > @@ -1643,13 +1642,8 @@ static int wled_configure_ovp_irq(struct wled *wled, > return 0; > } > > - rc = regmap_read(wled->regmap, wled->ctrl_addr + > - WLED3_CTRL_REG_MOD_EN, &val); > - if (rc < 0) > - return rc; > - > - /* Keep OVP irq disabled until module is enabled */ > - if (!(val & WLED3_CTRL_REG_MOD_EN_MASK)) > + /* Keep the OVP irq disabled until the module is enabled */ > + if (!wled->brightness) > disable_irq(wled->ovp_irq); [Severity: High] If default-brightness is 0, wled->brightness will be seeded with 0 later in wled_probe(). Will this incorrectly disable the OVP IRQ while the hardware module remains active? If the bootloader leaves the backlight ON, but default-brightness in the device tree is 0, checking !wled->brightness instead of the hardware MOD_EN state disables the OVP IRQ while hardware is still ON. Additionally, a user space request to set brightness to 0 evaluates !!0 != !!0 as false, silently ignoring the request and leaving the hardware permanently ON without OVP protection. [ ... ] > @@ -1734,6 +1729,24 @@ static int wled_probe(struct platform_device *pdev) > > INIT_DELAYED_WORK(&wled->ovp_work, wled_ovp_work); > > + val = WLED_DEFAULT_BRIGHTNESS; > + of_property_read_u32(pdev->dev.of_node, "default-brightness", &val); > + > + /* > + * The module may already be enabled, either by a bootloader that left > + * the backlight lit or by the setup above. Record that, so that the > + * first brightness update does not enable an already enabled module, > + * and so that the OVP irq is armed from probe rather than from that > + * first update. > + */ > + rc = regmap_read(wled->regmap, wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN, > + &mod_en); > + if (rc < 0) > + return rc; > + > + if (mod_en & WLED3_CTRL_REG_MOD_EN_MASK) > + wled->brightness = val; > + [Severity: High] This isn't a bug introduced by this patch, but WLED3 devices lack the wled_auto_detection_required function handler: drivers/video/backlight/qcom-wled.c:wled_configure() { ... case 3: u32_opts = wled3_opts; size = ARRAY_SIZE(wled3_opts); *cfg = wled3_config_defaults; wled->wled_set_brightness = wled3_set_brightness; wled->wled_sync_toggle = wled3_sync_toggle; ... } Does this lead to a NULL pointer dereference if the OVP IRQ fires? 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); ... } [Severity: High] This is a pre-existing issue, but the short IRQ handler unconditionally re-enables the hardware module: drivers/video/backlight/qcom-wled.c:wled_short_irq_handler() { ... rc = wled_module_enable(wled, true); ... } Does this override the software intentions if the backlight is logically OFF (or being turned OFF) in wled->brightness when the short-circuit IRQ triggers? This could cause the hardware backlight to become stuck ON, leading to permanent battery drain and incorrect state. [ ... ] > @@ -1742,9 +1755,6 @@ static int wled_probe(struct platform_device *pdev) > if (rc < 0) > return rc; > > - val = WLED_DEFAULT_BRIGHTNESS; > - of_property_read_u32(pdev->dev.of_node, "default-brightness", &val); > - > memset(&props, 0, sizeof(struct backlight_properties)); > props.type = BACKLIGHT_RAW; > props.brightness = val; [Severity: High] This is a pre-existing issue, but wled_probe() never calls platform_set_drvdata() before returning successfully: drivers/video/backlight/qcom-wled.c:wled_probe() { ... bl = devm_backlight_device_register(&pdev->dev, wled->name, &pdev->dev, wled, &wled_ops, &props); return PTR_ERR_OR_ZERO(bl); } Will this cause a guaranteed NULL pointer dereference in wled_remove() when unbinding the driver? drivers/video/backlight/qcom-wled.c:wled_remove() { struct wled *wled = platform_get_drvdata(pdev); mutex_destroy(&wled->lock); ... } [Severity: High] This is another pre-existing issue, but tearing down synchronization primitives in wled_remove() before the devm-managed backlight is unregistered creates a use-after-free window. drivers/video/backlight/qcom-wled.c:wled_remove() { ... mutex_destroy(&wled->lock); cancel_delayed_work_sync(&wled->ovp_work); ... } The backlight device is registered via devm_backlight_device_register(), meaning it remains active until devres cleanup after wled_remove() returns. Could this lead to lock corruption or use-after-free if userspace accesses the sysfs interface concurrently while the driver is being removed? drivers/video/backlight/qcom-wled.c:wled_update_status() { ... mutex_lock(&wled->lock); ... } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
