linux-kernel - Re: [PATCH 1/4] HID: picolcd: fix deferred_io init/cleanup to (un)register

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100630075657.47a325a6@pluto.restena.lu>
Date:	Wed, 30 Jun 2010 07:56:57 +0200
From:	Bruno Prémont <bonbons@...ux-vserver.org>
To:	Jaya Kumar <jayakumar.lkml@...il.com>
Cc:	Jiri Kosina <jkosina@...e.cz>, linux-fbdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] HID: picolcd: fix deferred_io init/cleanup to 
 (un)register_framebuffer ordering

Hi Jaya,

On Wed, 30 Jun 2010 09:52:13 Jaya Kumar wrote:
> On Tue, Jun 29, 2010 at 4:29 AM, Bruno Prémont
> <bonbons@...ux-vserver.org> wrote:
> > We need to call fb_deferred_io_init() before we
> > register_framebuffer() as otherwise, in case fbcon uses our
> > framebuffer, we will get a BUG() because in picolcd_fb_imageblit()
> > we schedule defio which has not been initialized yet.
> 
> Hi Bruno,
> 
> The comment above seems confusing to me in that it talks about fbcon
> and defio schedule.

Well I talk about fbcon as that's the trigger I have seen causing an
issue.

I'm fine with rewriting the changelog as to just talk about the
correct/expected order of initialization.

Thanks for looking at it,
Bruno


> What I see is that you originally had in picolcd:
> 
> >        error = register_framebuffer(info);
> ...
> >        fb_deferred_io_init(info);
> 
> which was a bug because it called register_framebuffer (ie: made the
> framebuffer available for use) and only then initialized the defio
> handlers which were needed for that framebuffer memory to be used.
> Drivers must always call defio_init _before_ register_framebuffer. The
> bug fix to picolcd below looks correct because it now does that
> sequence in the correct order.
> 
> Thanks,
> jaya
> 
> 
> 
> >
> > Note: this BUG() deadlocks ttys.
> >
> > Signed-off-by: Bruno Prémont <bonbons@...ux-vserver.org>
> > ---
> >  drivers/hid/hid-picolcd.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
> > index 95253b3..883d720 100644
> > --- a/drivers/hid/hid-picolcd.c
> > +++ b/drivers/hid/hid-picolcd.c
> > @@ -707,18 +707,19 @@ static int picolcd_init_framebuffer(struct
> > picolcd_data *data) dev_err(dev, "failed to create sysfs
> > attributes\n"); goto err_cleanup;
> >        }
> > +       fb_deferred_io_init(info);
> >        data->fb_info    = info;
> >        error = register_framebuffer(info);
> >        if (error) {
> >                dev_err(dev, "failed to register framebuffer\n");
> >                goto err_sysfs;
> >        }
> > -       fb_deferred_io_init(info);
> >        /* schedule first output of framebuffer */
> >        schedule_delayed_work(&info->deferred_work, 0);
> >        return 0;
> >
> >  err_sysfs:
> > +       fb_deferred_io_cleanup(info);
> >        device_remove_file(dev, &dev_attr_fb_update_rate);
> >  err_cleanup:
> >        data->fb_vbitmap = NULL;
> > @@ -747,7 +748,6 @@ static void picolcd_exit_framebuffer(struct
> > picolcd_data *data) data->fb_bpp     = 0;
> >        data->fb_info    = NULL;
> >        device_remove_file(&data->hdev->dev,
> > &dev_attr_fb_update_rate);
> > -       fb_deferred_io_cleanup(info);
> >        unregister_framebuffer(info);
> >        vfree(fb_bitmap);
> >        kfree(fb_vbitmap);
> > --
> > 1.7.1
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/