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

[Derek Foreman]

On 15/05/15 05:30 AM, 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.

Does this mean weston-launch is always the wrong thing to do if systemd
support is compiled in?

ie) should we refuse to build weston-launch if systemd support is enabled?