[PATCH 0/7] video: fbdev: Remove redundant dev_err()

[PATCH 0/7] video: fbdev: Remove redundant dev_err()

[Pan Chuang]

devm_request_irq() automatically logs detailed error messages on
failure via the devm_request_result() helper, which prints device
name, IRQ number, handler functions, and error code.

Remove the now-redundant driver-specific dev_err() calls in fbdev
drivers, as the core now provides more detailed diagnostic
information on failure

Pan Chuang (7):
  fbdev: mmp: Remove redundant dev_err()
  fbdev: omapfb/dsi-cm: Remove redundant dev_err()
  fbdev: pxa168fb: Remove redundant dev_err()
  fbdev: pxa3xx-gcu: Remove redundant dev_err()
  fbdev: pxafb: Remove redundant dev_err()
  fbdev: s3c-fb: Remove redundant dev_err()
  fbdev: sa1100fb: Remove redundant dev_err()

 drivers/video/fbdev/mmp/hw/mmp_ctrl.c                    | 2 --
 drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c | 4 +---
 drivers/video/fbdev/pxa168fb.c                           | 1 -
 drivers/video/fbdev/pxa3xx-gcu.c                         | 4 +---
 drivers/video/fbdev/pxafb.c                              | 1 -
 drivers/video/fbdev/s3c-fb.c                             | 4 +---
 drivers/video/fbdev/sa1100fb.c                           | 4 +---
 7 files changed, 4 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH 1/7] fbdev: mmp: Remove redundant dev_err()

[Pan Chuang]

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

Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 drivers/video/fbdev/mmp/hw/mmp_ctrl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/video/fbdev/mmp/hw/mmp_ctrl.c b/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
index 75bbdc0b4aa6..2c457da67a9f 100644
--- a/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
+++ b/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
@@ -505,8 +505,6 @@ static int mmphw_probe(struct platform_device *pdev)
 	ret = devm_request_irq(ctrl->dev, ctrl->irq, ctrl_handle_irq,
 		IRQF_SHARED, "lcd_controller", ctrl);
 	if (ret < 0) {
-		dev_err(ctrl->dev, "%s unable to request IRQ %d\n",
-				__func__, ctrl->irq);
 		ret = -ENXIO;
 		goto failed;
 	}
-- 
2.34.1

Re: [PATCH 1/7] fbdev: mmp: Remove redundant dev_err()

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver unconditionally overrides the error codes returned by platform_get_irq() and devm_request_irq() to fixed values (-ENOENT and -ENXIO), breaking probe deferral.
- [High] The shared interrupt handler unconditionally returns IRQ_HANDLED without checking if it actually serviced an interrupt.
- [High] The driver lacks a .remove callback but does not suppress sysfs unbind, leading to Use-After-Free and resource leaks.
--

--- Patch [1]: [PATCH 1/7] fbdev: mmp: Remove redundant dev_err() ---
commit 6ce9511a0259fe7a61c4cdd52d067da8431b2740
Author: Pan Chuang <panchuang@vivo.com>

fbdev: mmp: Remove redundant dev_err()

This commit removes a redundant dev_err() call from mmphw_probe().
The devm_request_irq() function already automatically logs detailed error
messages on failure.

> diff --git a/drivers/video/fbdev/mmp/hw/mmp_ctrl.c b/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
> index 75bbdc0b4aa65..2c457da67a9f1 100644
> --- a/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
> +++ b/drivers/video/fbdev/mmp/hw/mmp_ctrl.c
> @@ -505,8 +505,6 @@ static int mmphw_probe(struct platform_device *pdev)
>  	ret = devm_request_irq(ctrl->dev, ctrl->irq, ctrl_handle_irq,
>  		IRQF_SHARED, "lcd_controller", ctrl);

[Severity: High]
This is a pre-existing issue, but it looks like ctrl_handle_irq()
unconditionally returns IRQ_HANDLED.

If it doesn't check whether it actually serviced an interrupt, could it mask
unhandled interrupts on a shared line?

drivers/video/fbdev/mmp/hw/mmp_ctrl.c:ctrl_handle_irq() {
    ...
    return IRQ_HANDLED;
}

Can this break the kernel's spurious interrupt detection and lead to an
interrupt storm if a sharing device misbehaves?

[Severity: High]
This is a pre-existing issue, but since mmphw_driver lacks a .remove
callback and does not suppress sysfs unbind, what happens if the device is
unbound via sysfs?

When unbound, the driver core frees devm-managed resources, but the display
paths and SPI controller appear to be left registered:

static struct platform_driver mmphw_driver = {
        .driver         = {
                .name   = "mmp-disp",
        },
        .probe          = mmphw_probe,
};

Could this leave dangling pointers and unmapped memory active, leading to
use-after-free conditions?

>  	if (ret < 0) {
> -		dev_err(ctrl->dev, "%s unable to request IRQ %d\n",
> -				__func__, ctrl->irq);
>  		ret = -ENXIO;

[Severity: High]
This is a pre-existing issue, but overriding the return value of
devm_request_irq() to a fixed -ENXIO seems to swallow important error codes
like -EPROBE_DEFER.

Earlier in mmphw_probe(), the return value of platform_get_irq() is also
overridden to a fixed -ENOENT:

    irq = platform_get_irq(pdev, 0);
    if (irq < 0) {
            ret = -ENOENT;
            goto failed;
    }

Can this prevent proper probe deferral if dependencies like the IRQ
controller are not yet ready?

>  		goto failed;
>  	}

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

[PATCH 2/7] fbdev: omapfb/dsi-cm: Remove redundant dev_err()

[Pan Chuang]

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

Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
index 5e7963b4aa93..7c3463ee02ef 100644
--- a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
+++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
@@ -1185,10 +1185,8 @@ static int dsicm_probe(struct platform_device *pdev)
 				IRQF_TRIGGER_RISING,
 				"taal vsync", ddata);
 
-		if (r) {
-			dev_err(dev, "IRQ request failed\n");
+		if (r)
 			return r;
-		}
 
 		INIT_DEFERRABLE_WORK(&ddata->te_timeout_work,
 					dsicm_te_timeout_work_callback);
-- 
2.34.1

Re: [PATCH 2/7] fbdev: omapfb/dsi-cm: Remove redundant dev_err()

[sashiko-bot]

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

Pre-existing issues:
- [High] The error paths in `dsicm_probe()` fail to unregister the DSS device, leaving it linked in the global `panel_list` while its memory is freed, leading to a critical Use-After-Free.
--

--- Patch [2]: [PATCH 2/7] fbdev: omapfb/dsi-cm: Remove redundant dev_err() ---
commit 02fb3b045764b81b12936790330528bf51db067e
Author: Pan Chuang <panchuang@vivo.com>

fbdev: omapfb/dsi-cm: Remove redundant dev_err()

This commit removes a redundant dev_err() call after devm_request_irq()
since the devm API automatically logs detailed error messages on failure.

> diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
> index 5e7963b4aa93c..7c3463ee02ef1 100644
> --- a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
> +++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
[ ... ]
> @@ -1185,10 +1185,8 @@ static int dsicm_probe(struct platform_device *pdev)
>  				IRQF_TRIGGER_RISING,
>  				"taal vsync", ddata);
>  
> -		if (r) {
> -			dev_err(dev, "IRQ request failed\n");
> +		if (r)
>  			return r;

[Severity: High]
This is a pre-existing issue, but does returning directly here leave a
dangling pointer in the global panel_list?

Earlier in dsicm_probe(), the display is registered and added to the
global list:

	r = omapdss_register_display(dssdev);
	if (r) {
		dev_err(dev, "Failed to register panel\n");
		goto err_reg;
	}

If devm_request_irq() fails here, the function returns directly without
calling omapdss_unregister_display().

Since ddata was allocated using devm_kzalloc(), it will be automatically
freed on probe failure. Could this cause memory corruption or a
use-after-free when other functions like omap_dss_get_next_device() iterate
over the panel_list?

> -		}
>  
>  		INIT_DEFERRABLE_WORK(&ddata->te_timeout_work,
>  					dsicm_te_timeout_work_callback);

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

[PATCH 3/7] fbdev: pxa168fb: Remove redundant dev_err()

[Pan Chuang]

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

Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 drivers/video/fbdev/pxa168fb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/video/fbdev/pxa168fb.c b/drivers/video/fbdev/pxa168fb.c
index 6784888d93c9..ce8a823d7128 100644
--- a/drivers/video/fbdev/pxa168fb.c
+++ b/drivers/video/fbdev/pxa168fb.c
@@ -725,7 +725,6 @@ static int pxa168fb_probe(struct platform_device *pdev)
 	ret = devm_request_irq(&pdev->dev, irq, pxa168fb_handle_irq,
 			       IRQF_SHARED, info->fix.id, fbi);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "unable to request IRQ\n");
 		ret = -ENXIO;
 		goto failed_free_cmap;
 	}
-- 
2.34.1

Re: [PATCH 3/7] fbdev: pxa168fb: Remove redundant dev_err()

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver unconditionally returns `-ENOENT` when `platform_get_irq()` fails, breaking probe deferral.
- [High] The error path and remove function trigger a use-after-free and unclocked register access due to improper devm_request_irq() lifecycle.
- [High] Missing return value check for clk_prepare_enable() leads to possible system hang.
--

--- Patch [3]: [PATCH 3/7] fbdev: pxa168fb: Remove redundant dev_err() ---
commit 8117b479bf3e32b7a9c22b5f0311cebdedb684e8
Author: Pan Chuang <panchuang@vivo.com>

fbdev: pxa168fb: Remove redundant dev_err()

Since devm_request_irq() automatically logs detailed error messages on
failure, this commit removes the now-redundant driver-specific dev_err()
call in pxa168fb_probe().

> diff --git a/drivers/video/fbdev/pxa168fb.c b/drivers/video/fbdev/pxa168fb.c
> index 6784888d93c9f..ce8a823d7128e 100644
> --- a/drivers/video/fbdev/pxa168fb.c
> +++ b/drivers/video/fbdev/pxa168fb.c
> @@ -725,7 +725,6 @@ static int pxa168fb_probe(struct platform_device *pdev)

[Severity: High]
This is a pre-existing issue, but does this function mishandle probe
deferral?

Looking at the earlier part of pxa168fb_probe():

pxa168fb_probe()
    ...
    irq = platform_get_irq(pdev, 0);
    if (irq < 0)
        return -ENOENT;

If platform_get_irq() returns -EPROBE_DEFER, this overwrites it to -ENOENT,
which will prevent the kernel from retrying the probe later. Could this
return the actual error code instead?

[Severity: High]
This is a pre-existing issue, but is there a missing check for the return
value of clk_prepare_enable()?

Just before the IRQ request in pxa168fb_probe():

pxa168fb_probe()
    ...
    /*
     * enable controller clock
     */
    clk_prepare_enable(fbi->clk);

    pxa168fb_set_par(info);

If clk_prepare_enable() fails, the clock remains disabled. The driver then
calls pxa168fb_set_par() which writes to memory-mapped registers. Could
accessing these registers without an active clock cause a synchronous
external bus abort?

>  	ret = devm_request_irq(&pdev->dev, irq, pxa168fb_handle_irq,
>  			       IRQF_SHARED, info->fix.id, fbi);
>  	if (ret < 0) {
> -		dev_err(&pdev->dev, "unable to request IRQ\n");
>  		ret = -ENXIO;
>  		goto failed_free_cmap;
>  	}

[Severity: High]
This is a pre-existing issue, but does using a devm-managed IRQ here
introduce a use-after-free or unclocked register access during cleanup?

Because devm_request_irq() is used with IRQF_SHARED, the IRQ handler remains
registered until the devres cleanup runs, which happens after
pxa168fb_probe() or pxa168fb_remove() returns.

However, the error path and remove function free the resources early:

failed_free_clk:
    clk_disable_unprepare(fbi->clk);
failed_free_fbmem:
    ...
failed_free_info:
    framebuffer_release(info);

If another shared device triggers an interrupt during this window,
pxa168fb_handle_irq() will dereference the freed fbi pointer and read
hardware registers without an active clock:

pxa168fb_handle_irq()
    struct pxa168fb_info *fbi = dev_id;
    u32 isr = readl(fbi->reg_base + SPU_IRQ_ISR);

Should the driver explicitly free the IRQ or use a non-devm request to avoid
this race?

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

[PATCH 4/7] fbdev: pxa3xx-gcu: Remove redundant dev_err()

[Pan Chuang]

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

Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 drivers/video/fbdev/pxa3xx-gcu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
index a2320e2fb8f2..fc5bd7e7aae6 100644
--- a/drivers/video/fbdev/pxa3xx-gcu.c
+++ b/drivers/video/fbdev/pxa3xx-gcu.c
@@ -615,10 +615,8 @@ static int pxa3xx_gcu_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(dev, irq, pxa3xx_gcu_handle_irq,
 			       0, DRV_NAME, priv);
-	if (ret < 0) {
-		dev_err(dev, "request_irq failed\n");
+	if (ret < 0)
 		return ret;
-	}
 
 	/* allocate dma memory */
 	priv->shared = dma_alloc_coherent(dev, SHARED_SIZE,
-- 
2.34.1

Re: [PATCH 4/7] fbdev: pxa3xx-gcu: Remove redundant dev_err()

[sashiko-bot]

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

Pre-existing issues:
- [High] Premature IRQ registration before resource allocation and hardware initialization.
- [High] Use-after-free and hardware access violation due to devres teardown asymmetry in remove and error paths.
- [High] Data race leading to NULL pointer dereference in `pxa3xx_gcu_write()`.
- [High] Premature misc device registration exposes uninitialized device to userspace.
- [Medium] Missing timer cancellation in device removal leads to use-after-free.
--

--- Patch [4]: [PATCH 4/7] fbdev: pxa3xx-gcu: Remove redundant dev_err() ---
commit 07f66ca8434e8ab1bbe80f8c87905a7ed13a3c22
Author: Pan Chuang <panchuang@vivo.com>

fbdev: pxa3xx-gcu: Remove redundant dev_err()

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

> diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
> --- a/drivers/video/fbdev/pxa3xx-gcu.c
> +++ b/drivers/video/fbdev/pxa3xx-gcu.c
> @@ -615,10 +615,8 @@ static int pxa3xx_gcu_probe(struct platform_device *pdev)
>  
>  	ret = devm_request_irq(dev, irq, pxa3xx_gcu_handle_irq,
>  			       0, DRV_NAME, priv);

[Severity: High]
This is a pre-existing issue, but is it safe to request the IRQ here before
priv->shared is allocated and the hardware clock is enabled?

If a pending or spurious interrupt fires immediately after this
devm_request_irq() call, the interrupt handler pxa3xx_gcu_handle_irq() will
execute. It attempts to access priv->shared and reads hardware registers
before the clock is enabled:

pxa3xx_gcu_handle_irq()
    struct pxa3xx_gcu_shared *shared = priv->shared;
    u32 status = gc_readl(priv, REG_GCISCR) & IE_ALL;

Could this lead to a NULL pointer dereference or bus fault during probe?

> -	if (ret < 0) {
> -		dev_err(dev, "request_irq failed\n");
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	/* allocate dma memory */
>  	priv->shared = dma_alloc_coherent(dev, SHARED_SIZE,

[Severity: High]
This isn't a bug introduced by this patch, but does the error handling and
device removal order have a use-after-free risk?

Since devm_request_irq() is used, the interrupt remains active until after
pxa3xx_gcu_remove() returns. However, in pxa3xx_gcu_remove(), priv->shared
is manually freed and the hardware clock is disabled:

pxa3xx_gcu_remove() {
    ...
    dma_free_coherent(dev, SHARED_SIZE, priv->shared, priv->shared_phys);
    clk_disable_unprepare(priv->clk);
    ...
}

If the hardware interrupt is not masked and fires between these manual 
teardowns and the devres cleanup, pxa3xx_gcu_handle_irq() would run and
access freed memory and disabled hardware.

[Severity: High]
This is a pre-existing issue, but is there a potential data race leading to a
NULL pointer dereference in pxa3xx_gcu_write()?

The list emptiness check appears to be decoupled from the lock. If multiple
threads concurrently write when only a single buffer is free, they can pass
the lockless check:

pxa3xx_gcu_write() {
    if (!priv->free) {
        ...
    }
    
    spin_lock_irqsave(&priv->spinlock, flags);
    buffer = priv->free;
    priv->free = buffer->next;

If the first thread removes the buffer, making priv->free NULL, would the
second thread acquire the lock, blindly assign buffer = priv->free, and then
cause a NULL dereference when evaluating buffer->next?

[Severity: High]
This isn't a bug introduced by this patch, but is the misc device registered
prematurely during probe?

In pxa3xx_gcu_probe(), misc_register() is called before the hardware clocks
are enabled:

pxa3xx_gcu_probe() {
    ...
    ret = misc_register(&priv->misc_dev);
    ...
    ret = clk_prepare_enable(priv->clk);
    ...
}

If userspace opens the device immediately after it appears and invokes
ioctl(PXA3XX_GCU_IOCTL_RESET), it will attempt to write to hardware registers
via gc_writel() before clk_prepare_enable() completes. Could this lead to a
system hang or external abort?

[Severity: Medium]
This is a pre-existing issue, but is there a missing timer cancellation in
the device removal path?

When optional debugging is enabled, a self-rearming debug timer is
initialized during probe. However, pxa3xx_gcu_remove() does not appear to
call del_timer_sync() to clean it up.

After the driver is unloaded and devres frees priv, will the timer fire and
access the freed priv pointer?

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

[PATCH 5/7] fbdev: pxafb: Remove redundant dev_err()

[Pan Chuang]

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

Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 drivers/video/fbdev/pxafb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index e418eee825fb..1682345fb9b0 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -2312,7 +2312,6 @@ static int pxafb_probe(struct platform_device *dev)
 
 	ret = devm_request_irq(&dev->dev, irq, pxafb_handle_irq, 0, "LCD", fbi);
 	if (ret) {
-		dev_err(&dev->dev, "request_irq failed: %d\n", ret);
 		ret = -EBUSY;
 		goto failed_free_mem;
 	}
-- 
2.34.1

Re: [PATCH 5/7] fbdev: pxafb: Remove redundant dev_err()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The background thread `fbi->smart_thread` is never stopped, leading to a leaked thread that causes Use-After-Free (UAF) upon device removal or probe failure.
- [Critical] The CPU frequency notifier is never unregistered, leading to a UAF upon device unbind.
- [High] The driver masks the original error codes returned by `platform_get_irq()` and `devm_request_irq()`, overriding them with hardcoded values (`-ENODEV` and `-EBUSY`).
- [High] The asynchronous work `fbi->task` may be scheduled during probe but is not canceled in the probe error paths.
- [High] The device IRQ is requested before the completion objects used by the IRQ handler are initialized, risking a crash if an interrupt fires early.
--

--- Patch [5]: [PATCH 5/7] fbdev: pxafb: Remove redundant dev_err() ---
commit 689be132f4af510aaf94189aee84d65e9d9a1907
Author: Pan Chuang <panchuang@vivo.com>

fbdev: pxafb: Remove redundant dev_err()

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

> diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
> index e418eee825fb2..1682345fb9b0a 100644
> --- a/drivers/video/fbdev/pxafb.c
> +++ b/drivers/video/fbdev/pxafb.c
> @@ -2312,7 +2312,6 @@ static int pxafb_probe(struct platform_device *dev)
>  
>  	ret = devm_request_irq(&dev->dev, irq, pxafb_handle_irq, 0, "LCD", fbi);

[Severity: High]
This is a pre-existing issue, but is it safe to request the IRQ here before
the completion objects are initialized?

If the bootloader leaves the hardware active, an early interrupt could cause
complete() to operate on a zero-initialized wait queue in pxafb_handle_irq(),
leading to a NULL pointer dereference. The IRQ is requested before
pxafb_smart_init() and pxafb_overlay_init() have had a chance to run.

>  	if (ret) {
> -		dev_err(&dev->dev, "request_irq failed: %d\n", ret);
>  		ret = -EBUSY;

[Severity: High]
This is a pre-existing issue, but does this improperly mask the original
error code returned by devm_request_irq()?

A similar pattern exists just above this block for platform_get_irq():

pxafb_probe() {
    ...
	irq = platform_get_irq(dev, 0);
	if (irq < 0) {
		ret = -ENODEV;
		goto failed_free_mem;
	}
    ...
}

If platform_get_irq() returns -EPROBE_DEFER, overriding it to -ENODEV causes
the device probe to fail permanently instead of deferring correctly.

[Severity: Critical]
This is a pre-existing issue, but is the background thread fbi->smart_thread
ever stopped?

Later in pxafb_probe():

pxafb_probe() {
    ...
	ret = pxafb_smart_init(fbi);
	if (ret) {
		dev_err(&dev->dev, "failed to initialize smartpanel\n");
		goto failed_free_mem;
	}
    ...
}

pxafb_smart_init() starts a thread with kthread_run(). If this thread is not
stopped in pxafb_remove() or on probe error paths, and fbi is allocated with
devm_kzalloc(), it will be freed while the thread continues to run. Can this
lead to a use-after-free?

[Severity: Critical]
This is a pre-existing issue, but is cpufreq_unregister_notifier() ever called
to unregister the CPU frequency notifier?

pxafb_probe() {
    ...
#ifdef CONFIG_CPU_FREQ
	fbi->freq_transition.notifier_call = pxafb_freq_transition;
	cpufreq_register_notifier(&fbi->freq_transition,
				CPUFREQ_TRANSITION_NOTIFIER);
#endif
    ...
}

If this is missing from pxafb_remove(), could the cpufreq core retain a
dangling pointer in its notifier list when fbi is freed by devres after
unbind?

[Severity: High]
This is a pre-existing issue, but can the asynchronous work fbi->task be left
running on the error path?

During probe, pxafb_set_par() can schedule this work. If a subsequent step
fails, the error path is taken:

pxafb_probe() {
    ...
failed_free_cmap:
	if (fbi->fb.cmap.len)
		fb_dealloc_cmap(&fbi->fb.cmap);
failed_free_mem:
	free_pages_exact(fbi->video_mem, fbi->video_mem_size);
    ...
}

Returning without cancel_work_sync(&fbi->task) could leave the system
workqueue executing pxafb_task on a freed object since fbi is managed by
devres.

>  		goto failed_free_mem;
>  	}

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

[PATCH 6/7] fbdev: s3c-fb: Remove redundant dev_err()

[Pan Chuang]

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

Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 drivers/video/fbdev/s3c-fb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/s3c-fb.c b/drivers/video/fbdev/s3c-fb.c
index 2f4d707e2e09..82a3bc9de433 100644
--- a/drivers/video/fbdev/s3c-fb.c
+++ b/drivers/video/fbdev/s3c-fb.c
@@ -1421,10 +1421,8 @@ static int s3c_fb_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(dev, sfb->irq_no, s3c_fb_irq,
 			  0, "s3c_fb", sfb);
-	if (ret) {
-		dev_err(dev, "irq request failed\n");
+	if (ret)
 		goto err_lcd_clk;
-	}
 
 	dev_dbg(dev, "got resources (regs %p), probing windows\n", sfb->regs);
 
-- 
2.34.1

Re: [PATCH 6/7] fbdev: s3c-fb: Remove redundant dev_err()

[sashiko-bot]

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

Pre-existing issues:
- [High] Overwriting platform_get_irq() return value breaks deferred probing and error reporting.
- [High] Ignored return values from clk_prepare_enable().
- [High] The error cleanup loop in s3c_fb_probe() passes a NULL pointer to s3c_fb_release_win(), leading to a kernel panic.
- [High] Calling unregister_framebuffer() on an unregistered framebuffer corrupts the fbdev subsystem.
- [High] Repeated initialization of shared waitqueue sfb->vsync_info.wait inside a loop with active IRQs and userspace exposure.
--

--- Patch [6]: [PATCH 6/7] fbdev: s3c-fb: Remove redundant dev_err() ---
commit 0bb07a380253651e3a1f2d4c656dfc05702e59ca
Author: Pan Chuang <panchuang@vivo.com>

fbdev: s3c-fb: Remove redundant dev_err()

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

> diff --git a/drivers/video/fbdev/s3c-fb.c b/drivers/video/fbdev/s3c-fb.c
> index 2f4d707e2e097..82a3bc9de433c 100644
> --- a/drivers/video/fbdev/s3c-fb.c
> +++ b/drivers/video/fbdev/s3c-fb.c
> @@ -1421,10 +1421,8 @@ static int s3c_fb_probe(struct platform_device *pdev)
>  
>  	ret = devm_request_irq(dev, sfb->irq_no, s3c_fb_irq,
>  			  0, "s3c_fb", sfb);
> -	if (ret) {
> -		dev_err(dev, "irq request failed\n");
> +	if (ret)
>  		goto err_lcd_clk;
> -	}
>  
>  	dev_dbg(dev, "got resources (regs %p), probing windows\n", sfb->regs);
>  

[Severity: High]
This is a pre-existing issue, but does this code ignore the return values
from clk_prepare_enable()?

drivers/video/fbdev/s3c-fb.c:s3c_fb_probe() {
...
	clk_prepare_enable(sfb->bus_clk);

	if (!sfb->variant.has_clksel) {
		sfb->lcd_clk = devm_clk_get(dev, "sclk_fimd");
		if (IS_ERR(sfb->lcd_clk)) {
...
		}

		clk_prepare_enable(sfb->lcd_clk);
	}
...
}

If clk_prepare_enable() fails, the driver continues as if the clocks are
active, which can lead to a synchronous external abort when accessing
hardware registers.


[Severity: High]
This isn't a bug introduced by this patch, but looking at s3c_fb_probe(),
is it intended to discard the actual error code returned by
platform_get_irq() and replace it with -ENOENT?

drivers/video/fbdev/s3c-fb.c:s3c_fb_probe() {
...
	sfb->irq_no = platform_get_irq(pdev, 0);
	if (sfb->irq_no < 0) {
		ret = -ENOENT;
		goto err_lcd_clk;
	}
...
}

If platform_get_irq() returns -EPROBE_DEFER, returning -ENOENT breaks
deferred probing and causes the framebuffer initialization to fail
permanently.


[Severity: High]
This is a pre-existing issue, but does this error path loop lead to a NULL
pointer dereference?

drivers/video/fbdev/s3c-fb.c:s3c_fb_probe() {
...
		ret = s3c_fb_probe_win(sfb, win, fbdrv->win[win],
				       &sfb->windows[win]);
		if (ret < 0) {
			dev_err(dev, "failed to create window %d\n", win);
			for (; win >= 0; win--)
				s3c_fb_release_win(sfb, sfb->windows[win]);
...
}

If s3c_fb_probe_win() fails early (e.g. framebuffer_alloc() returns -ENOMEM),
sfb->windows[win] remains NULL.

Then s3c_fb_release_win() evaluates win->fbinfo without checking if win
itself is NULL:

drivers/video/fbdev/s3c-fb.c:s3c_fb_release_win() {
...
static void s3c_fb_release_win(struct s3c_fb *sfb, struct s3c_fb_win *win)
{
	u32 data;

	if (win->fbinfo) {
...
}


[Severity: High]
This is a pre-existing issue, but does s3c_fb_release_win() call
unregister_framebuffer() on a framebuffer that was never registered?

drivers/video/fbdev/s3c-fb.c:s3c_fb_release_win() {
...
	if (win->fbinfo) {
		if (sfb->variant.has_shadowcon) {
...
		}
		unregister_framebuffer(win->fbinfo);
...
}

If s3c_fb_probe_win() fails after fbinfo is allocated but before calling
register_framebuffer() (such as if s3c_fb_alloc_memory() fails), the error
path calls s3c_fb_release_win().

Calling unregister_framebuffer() on an unregistered fbinfo corrupts the
fbdev subsystem counts.


[Severity: High]
This is a pre-existing issue, but is it safe to repeatedly initialize the
shared waitqueue sfb->vsync_info.wait inside a per-window loop?

drivers/video/fbdev/s3c-fb.c:s3c_fb_probe_win() {
...
static int s3c_fb_probe_win(...)
{
...
	init_waitqueue_head(&sfb->vsync_info.wait);
...
}

s3c_fb_probe_win() is called for each window by s3c_fb_probe().

Once the first window is registered, it is exposed to userspace. If
userspace opens it and calls the FBIO_WAITFORVSYNC ioctl, a task can sleep
on the waitqueue.

When the driver probes the next window, it calls init_waitqueue_head()
again on the same shared waitqueue, which corrupts the list head while a
task is actively sleeping on it.

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

[PATCH 7/7] fbdev: sa1100fb: Remove redundant dev_err()

[Pan Chuang]

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

Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
 drivers/video/fbdev/sa1100fb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/sa1100fb.c b/drivers/video/fbdev/sa1100fb.c
index 0d362d2bf0e3..fea0172c45b5 100644
--- a/drivers/video/fbdev/sa1100fb.c
+++ b/drivers/video/fbdev/sa1100fb.c
@@ -1167,10 +1167,8 @@ static int sa1100fb_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(&pdev->dev, irq, sa1100fb_handle_irq, 0,
 			       "LCD", fbi);
-	if (ret) {
-		dev_err(&pdev->dev, "request_irq failed: %d\n", ret);
+	if (ret)
 		return ret;
-	}
 
 	fbi->shannon_lcden = gpiod_get_optional(&pdev->dev, "shannon-lcden",
 						GPIOD_OUT_LOW);
-- 
2.34.1

Re: [PATCH 7/7] fbdev: sa1100fb: Remove redundant dev_err()

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing `.suppress_bind_attrs = true` leads to catastrophic use-after-free on sysfs unbind.
--

--- Patch [7]: [PATCH 7/7] fbdev: sa1100fb: Remove redundant dev_err() ---
commit 811a165c0a2521029ab935181ae6d9d247a19898
Author: Pan Chuang <panchuang@vivo.com>

fbdev: sa1100fb: Remove redundant dev_err()

This commit simplifies the probe function by removing a redundant error
message after devm_request_irq(). The devm variant already logs detailed
error messages internally on failure.

> diff --git a/drivers/video/fbdev/sa1100fb.c b/drivers/video/fbdev/sa1100fb.c
> index 0d362d2bf0e38..fea0172c45b5b 100644
> --- a/drivers/video/fbdev/sa1100fb.c
> +++ b/drivers/video/fbdev/sa1100fb.c
> @@ -1167,10 +1167,8 @@ static int sa1100fb_probe(struct platform_device *pdev)
>  
>  	ret = devm_request_irq(&pdev->dev, irq, sa1100fb_handle_irq, 0,
>  			       "LCD", fbi);
> -	if (ret) {
> -		dev_err(&pdev->dev, "request_irq failed: %d\n", ret);
> +	if (ret)
>  		return ret;
> -	}

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying
sa1100fb_probe(), should we also address a potential use-after-free during
driver unbind?

Looking at the driver definition, it registers resources that require explicit
unregistration, but does not provide a remove callback and does not suppress
sysfs unbind:

drivers/video/fbdev/sa1100fb.c:sa1100fb_driver {
	.probe		= sa1100fb_probe,
	.suspend	= sa1100fb_suspend,
	.resume		= sa1100fb_resume,
	.driver		= {
		.name	= "sa11x0-fb",
	},
};

If a user unbinds the driver via sysfs, the driver core will call
devres_release_all() and free the devm allocated fbi structure.

However, because register_framebuffer() and cpufreq_register_notifier()
are called during probe without ever being unregistered, the fbdev and cpufreq
subsystems will hold dangling pointers to the freed fbi structure.

Any subsequent access to the framebuffer or CPU frequency transitions
could dereference this freed memory.

Would it make sense to set suppress_bind_attrs = true in the driver struct
to prevent sysfs unbind from succeeding, or alternatively implement a proper
remove callback to unregister these resources?

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