[PATCH] compositor-fbdev: Wait and retry before failing on reconnect to the framebuffer

[nerdopolis]

On Friday, May 15, 2015 12:30:30 PM David Herrmann wrote:
> Hi
> 
> On Fri, May 15, 2015 at 8:39 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> >
> > On Wed, 13 May 2015 21:43:39 -0400
> > nerdopolis <bluescreen_avenger at verizon.net> wrote:
> >
> > > Resolving https://bugs.freedesktop.org/show_bug.cgi?id=73782
> > > udev might be configured to set the permissions on framebuffer devices with the UACCESS attribute.
> > > Weston currently attempts to reconnect to the framebuffer device before udev can set the permissions back.
> > >
> > > It waits 3 times in case if the system is heavily paging, or slowed down, and prevents udev from working.
> > > In my testing the delay is long enough where it works on the first try
> > > ---
> > >  src/compositor-fbdev.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> > > index 7c505ce..42d2881 100644
> > > --- a/src/compositor-fbdev.c
> > > +++ b/src/compositor-fbdev.c
> > > @@ -663,13 +663,23 @@ fbdev_output_reenable(struct fbdev_compositor *compositor,
> > >       struct fbdev_output *output = to_fbdev_output(base);
> > >       struct fbdev_screeninfo new_screen_info;
> > >       int fb_fd;
> > > +     int retries;
> > >       const char *device;
> > >
> > >       weston_log("Re-enabling fbdev output.\n");
> > >
> > >       /* Create the frame buffer. */
> > > -     fb_fd = fbdev_frame_buffer_open(output, output->device,
> > > +     fb_fd = -1;
> > > +     retries = 0;
> > > +     while (fb_fd < 0 && retries < 3) {
> > > +             usleep(10000);
> > > +             fb_fd = fbdev_frame_buffer_open(output, output->device,
> > >                                       &new_screen_info);
> > > +             if (fb_fd < 0) {
> > > +               weston_log("Creating frame buffer failed. Retrying...\n");
> > > +             }
> > > +             retries++;
> > > +     }
> > >       if (fb_fd < 0) {
> > >               weston_log("Creating frame buffer failed.\n");
> > >               goto err;
> >
> > Hi,
> >
> > I hate sleeps. I'd really want an explanation in the commit message on
> > why this cannot be fixed properly at all, and you need a delay to have
> > it work.
> >
> > It is possible there is no better way, but I would like to understand
> > why first.
> >
> > For instance, should reenable() be waiting for some sort of udev
> > advertisement of the fb device? Is there any event from udev to signal
> > this?
> >
> > Do you have systemd involved in your use case? Could this be solved with
> > systemd or logind? Or should we let weston-launch open the fb device
> > instead?
> >
> > I'm CC'ing David, maybe he might have an idea?
> 
> systemd-logind listens to VT events via poll(2) on
> /sys/class/tty/tty0/active. On VT switches it changes ACL permissions
> on dev-nodes underneath /dev, if the 'uaccess' tag is set on a device.
> But it cannot delay a VT-switch, it just reacts to it.
> 
> It is inherent to this approach, that the permissions are set _after_
> the VT switch. Therefore, there's a race and I guess it's what is hit
> here. However, systemd-logind sends out dbus signals after it fully
> changed the permissions. Hence, if you rely on the 'uaccess'
> functionality by systemd, then you better also use the systemd APIs
> (either sd_login_monitor_new() or DBus). If you don't want to use the
> systemd APIs, then you cannot rely on 'uaccess' (i.e., you have to set
> static group/user permissions on your device nodes; it will not
> interfere with 'uaccess', logind allows both in parallel).
> 
> Long story short: Please run weston directly (_without_ weston-launch,
> instead directly invoking weston from within a logged-in VT), with
> systemd support compiled it. It should work fine with 'uaccess'. If
> you don't have systemd support compiled in, please use static access
> permissions.
> 
> Thanks
> David
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Hi.

It seems that I am a bit mistaken. My scripts used to call Weston using weston-launch, and then I configured fbdev for uaccess permissions, and stopped setting static permissions.
I created the workaround locally... Then later on I updated my login script manager to not use weston-launch anymore, and call systemd enabled login sessions on TTYs with runuser... 

I didn't bring it upstream until now, because I wasn't sure if it was actually a good way to fix it... 

I actually haven't tested it with _vanilla_ weston until now. Removing my patch and tty switching with /dev/fb0 set with UACCESS, with --backend=fbdev-backend _without_ weston-launch does *not* cause a crash... I called weston directly, and tty switched fine on fbdev.

However, It still fails under weston-launch even when compiling against systemd. TTY switching results in the race condition, and Weston crashes. But now most systems, should have systemd >v209, and to use that. 
There still might be the possibility that users might want the UACCESS attribute on /dev/fb* , on a non systemd system, or are <v209 (I.E: existing Ubuntu LTS versions), on systems that don't support kernel mode setting?

Thanks.