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: <CAHCkknoKsS7bjgR5WaV0-1e_2mB1XiqXi_Y_UWyAeC5N8PfPRw@mail.gmail.com>
Date: Thu, 20 Nov 2025 23:30:46 +0530
From: sukrut heroorkar <hsukrut3@...il.com>
To: David Hunter <david.hunter.linux@...il.com>
Cc: 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>, 
	shuah@...nel.org
Subject: Re: [PATCH] fbdev: q40fb: request memory region

On Wed, Nov 19, 2025 at 7:27 PM David Hunter
<david.hunter.linux@...il.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@...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.
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;
> >  }
> >
>