linux-kernel - Re: [PATCH] fbdev: q40fb: request memory region

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4ec784a5-0f67-4fd3-9d51-d89a9fa9a385@gmail.com>
Date: Wed, 19 Nov 2025 08:57:21 -0500
From: David Hunter <david.hunter.linux@...il.com>
To: Sukrut Heroorkar <hsukrut3@...il.com>, Helge Deller <deller@....de>,
 "open list:FRAMEBUFFER LAYER" <linux-fbdev@...r.kernel.org>,
 "open list:FRAMEBUFFER LAYER" <dri-devel@...ts.freedesktop.org>,
 open list <linux-kernel@...r.kernel.org>
Cc: shuah@...nel.org, david.hunter.linux@...il.com
Subject: Re: [PATCH] fbdev: q40fb: request memory region

On 11/18/25 04:56, Sukrut Heroorkar wrote:
> The q40fb driver uses a fixed physical address but never reserves
> the corresponding I/O region. Reserve the range  as suggested in
> Documentation/gpu/todo.rst ("Request memory regions in all fbdev drivers").
> 
> No functional change beyond claming the resource. This change is compile
> tested only.

Reserving memory is a significant "functional" change, so you should not
put "No functional change...". I have noticed that in the mentorship
program, mentees might say this often times when they have not done
testing.

Thank you for describing that you did a compile test, but I believe that
more testing should be done before this patch is accepted.

As a result, if you are unable to test this device, I believe that an
RFT tag should be used. Also, the testing information goes below the
"---". This puts it in the change log and would make it so that if a
patch is accepted, everything below the change log is not put in the
commit message.

> 
> Signed-off-by: Sukrut Heroorkar <hsukrut3@...il.com>
> ---
>  drivers/video/fbdev/q40fb.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/video/fbdev/q40fb.c b/drivers/video/fbdev/q40fb.c
> index 1ff8fa176124..935260326c6f 100644
> --- a/drivers/video/fbdev/q40fb.c
> +++ b/drivers/video/fbdev/q40fb.c
> @@ -101,6 +101,12 @@ static int q40fb_probe(struct platform_device *dev)
>  	info->par = NULL;
>  	info->screen_base = (char *) q40fb_fix.smem_start;
>  
> +	if (!request_mem_region(q40fb_fix.smem_start, q40fb_fix.smem_len,
> +				"q40fb")) {
> +		dev_err(&dev->dev, "cannot reserve video memory at 0x%lx\n",
> +			q40fb_fix.smem_start);
> +	}
> +

Is this correct? It seems to me that in the case of an error, all you
are doing is simply logging the error and proceeding. Would this cause
the device to continue to try to use space that it was not able to
reserve? I do not have experience with this device or the driver, but
that does not seem correct to me.

>  	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
>  		framebuffer_release(info);
>  		return -ENOMEM;
> @@ -144,6 +150,7 @@ static int __init q40fb_init(void)
>  		if (ret)
>  			platform_driver_unregister(&q40fb_driver);
>  	}
> +
>  	return ret;
>  }
>