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

[sashiko-bot@kernel.org]

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