Re: [PATCH 6/7] fbdev: s3c-fb: 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] 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