[PATCH] fbdev: q40fb: request memory region

[PATCH] fbdev: q40fb: request memory region

[Sukrut Heroorkar]

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.

Signed-off-by: Sukrut Heroorkar <hsukrut3@gmail.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);
+	}
+
 	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;
 }
 
-- 
2.43.0

Re: [PATCH] fbdev: q40fb: request memory region

[David Hunter]

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@gmail.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;
>  }
>  

Re: [PATCH] fbdev: q40fb: request memory region

[sukrut heroorkar]

On Wed, Nov 19, 2025 at 7:27 PM David Hunter
<david.hunter.linux@gmail.com> wrote:
>
> 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.
qemu-system-m68k does not emulate a Q40 machine, thus the change
was compile tested only with W=1 & debugging enabled.
>
> 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.
Thank you. I will make a note of this for the future patches.
> >
> > Signed-off-by: Sukrut Heroorkar <hsukrut3@gmail.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.
I referred to a patch, which was recently accepted, having a similar
implementation.
However, other fbdev drivers with similar implementation, returns a
-EBUSY  when the
If() evaluates true indicating resource already being occupied.  I
will make the necessary
changes and resend the patch as RFT.
>
> >       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;
> >  }
> >
>