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

[sashiko-bot@kernel.org]

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