Re: [PATCH 5/7] fbdev: pxafb: 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:
- [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