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

[David Herrmann]

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