[PATCH 0/8] Handle drm race condition

[PATCH 0/8] Handle drm race condition

[Bryce Harrington]

When starting up (on Ubuntu), X can hit an error trying to set the
version on the drm device.  We believe this is a race with plymouth (or
the kernel), since adding some delay to the boot results in a
functioning session for affected users.

So far we have not found a reliable way to reproduce the bug
synthetically.  It appears to affect users on fast booting hardware
(e.g. SSDs) when using the Intel graphics driver.

We have not root-caused the bug yet.  Currently we suspect the actual
breakage is underneath X (plymouth/lightdm/kernel), and are still
experimenting.  However, this patch does seem to improve things for
users, so it or parts of it may be worth your consideration for
inclusion in xserver.

I'm including the patch broken down into easily pickable chunks.  Note
the final patch in the series is highly optional; it handles EAGAIN
being passed from the ioctl - which looks impossible to happen in
practice.

https://bugs.launchpad.net/ubuntu/+source/libdrm/+bug/982889

Bryce Harrington (8):
  xfree86: (Cleanup) Close fd if drm interface 1.4 could not be set.
  xfree86: Track error code and add label for error handling.
  xfree86:  Provide more details on failure
  xfree86: Keep trying to set interface on drm until it succeeds
  xfree86: After 2 sec, abort setting drm interface version.
  xfree86: Fix race condition failure opening drm.
  xfree86: Be verbose if waiting on opening the drm device
  xfree86: Also handle EAGAIN errors from drmSetInterfaceVersion().

 hw/xfree86/os-support/linux/lnx_platform.c |   43 ++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 8 deletions(-)

-- 
1.7.9.5

[PATCH 1/8] xfree86: (Cleanup) Close fd if drm interface 1.4 could not be set.

[Bryce Harrington]

Signed-off-by: Bryce Harrington <bryce@canonical.com>
---
 hw/xfree86/os-support/linux/lnx_platform.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c
index 76f5583..69a5b8c 100644
--- a/hw/xfree86/os-support/linux/lnx_platform.c
+++ b/hw/xfree86/os-support/linux/lnx_platform.c
@@ -34,6 +34,7 @@ get_drm_info(struct OdevAttributes *attribs, char *path)
     sv.drm_dd_minor = -1;       /* Don't care */
     if (drmSetInterfaceVersion(fd, &sv)) {
         ErrorF("setversion 1.4 failed\n");
+	close(fd);
         return FALSE;
     }
 
-- 
1.7.9.5

[PATCH 2/8] xfree86: Track error code and add label for error handling.

[Bryce Harrington]

Signed-off-by: Bryce Harrington <bryce@canonical.com>
---
 hw/xfree86/os-support/linux/lnx_platform.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c
index 69a5b8c..6ee219a 100644
--- a/hw/xfree86/os-support/linux/lnx_platform.c
+++ b/hw/xfree86/os-support/linux/lnx_platform.c
@@ -23,6 +23,7 @@ get_drm_info(struct OdevAttributes *attribs, char *path)
     drmSetVersion sv;
     char *buf;
     int fd;
+    int err = 0;
 
     fd = open(path, O_RDWR, O_CLOEXEC);
     if (fd == -1)
@@ -32,10 +33,10 @@ get_drm_info(struct OdevAttributes *attribs, char *path)
     sv.drm_di_minor = 4;
     sv.drm_dd_major = -1;       /* Don't care */
     sv.drm_dd_minor = -1;       /* Don't care */
-    if (drmSetInterfaceVersion(fd, &sv)) {
+    err = drmSetInterfaceVersion(fd, &sv);
+    if (err) {
         ErrorF("setversion 1.4 failed\n");
-	close(fd);
-        return FALSE;
+	goto out;
     }
 
     xf86_add_platform_device(attribs);
@@ -44,8 +45,9 @@ get_drm_info(struct OdevAttributes *attribs, char *path)
     xf86_add_platform_device_attrib(xf86_num_platform_devices - 1,
                                     ODEV_ATTRIB_BUSID, buf);
     drmFreeBusid(buf);
+out:
     close(fd);
-    return TRUE;
+    return (err == 0);
 }
 
 Bool
-- 
1.7.9.5

[PATCH 3/8] xfree86: Provide more details on failure

[Bryce Harrington]

Signed-off-by: Bryce Harrington <bryce@canonical.com>
---
 hw/xfree86/os-support/linux/lnx_platform.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c
index 6ee219a..3ae2db1 100644
--- a/hw/xfree86/os-support/linux/lnx_platform.c
+++ b/hw/xfree86/os-support/linux/lnx_platform.c
@@ -7,6 +7,8 @@
 #include <xf86drm.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <errno.h>
+#include <string.h>
 
 /* Linux platform device support */
 #include "xf86_OSproc.h"
@@ -35,7 +37,7 @@ get_drm_info(struct OdevAttributes *attribs, char *path)
     sv.drm_dd_minor = -1;       /* Don't care */
     err = drmSetInterfaceVersion(fd, &sv);
     if (err) {
-        ErrorF("setversion 1.4 failed\n");
+        ErrorF("setversion 1.4 failed: %s\n", strerror(-err));
 	goto out;
     }
 
-- 
1.7.9.5

[PATCH 4/8] xfree86: Keep trying to set interface on drm until it succeeds

[Bryce Harrington]

Signed-off-by: Bryce Harrington <bryce@canonical.com>
---
 hw/xfree86/os-support/linux/lnx_platform.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c
index 3ae2db1..c4d128e 100644
--- a/hw/xfree86/os-support/linux/lnx_platform.c
+++ b/hw/xfree86/os-support/linux/lnx_platform.c
@@ -31,11 +31,16 @@ get_drm_info(struct OdevAttributes *attribs, char *path)
     if (fd == -1)
         return FALSE;
 
-    sv.drm_di_major = 1;
-    sv.drm_di_minor = 4;
-    sv.drm_dd_major = -1;       /* Don't care */
-    sv.drm_dd_minor = -1;       /* Don't care */
-    err = drmSetInterfaceVersion(fd, &sv);
+    while (1) {
+	sv.drm_di_major = 1;
+	sv.drm_di_minor = 4;
+	sv.drm_dd_major = -1;       /* Don't care */
+	sv.drm_dd_minor = -1;       /* Don't care */
+	err = drmSetInterfaceVersion(fd, &sv);
+	if (!err)
+	    break;
+	usleep(1000);
+    }
     if (err) {
         ErrorF("setversion 1.4 failed: %s\n", strerror(-err));
 	goto out;
-- 
1.7.9.5

[PATCH 5/8] xfree86: After 2 sec, abort setting drm interface version.

[Bryce Harrington]

And if we've had to delay booting due to not being able to set the
interface, fess up.

Signed-off-by: Bryce Harrington <bryce@canonical.com>
---
 hw/xfree86/os-support/linux/lnx_platform.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c
index c4d128e..4094866 100644
--- a/hw/xfree86/os-support/linux/lnx_platform.c
+++ b/hw/xfree86/os-support/linux/lnx_platform.c
@@ -26,19 +26,24 @@ get_drm_info(struct OdevAttributes *attribs, char *path)
     char *buf;
     int fd;
     int err = 0;
+    int tries = 0;
 
     fd = open(path, O_RDWR, O_CLOEXEC);
     if (fd == -1)
         return FALSE;
 
-    while (1) {
+    while (tries++ < 2000) {
 	sv.drm_di_major = 1;
 	sv.drm_di_minor = 4;
 	sv.drm_dd_major = -1;       /* Don't care */
 	sv.drm_dd_minor = -1;       /* Don't care */
+
 	err = drmSetInterfaceVersion(fd, &sv);
-	if (!err)
+	if (!err) {
+	    if (tries > 1)
+		LogMessage(X_INFO, "setversion 1.4 succeeded on try #%d\n", tries);
 	    break;
+	}
 	usleep(1000);
     }
     if (err) {
-- 
1.7.9.5

Re: [PATCH 5/8] xfree86: After 2 sec, abort setting drm interface version.

[Chris Wilson]

On Fri, Mar 15, 2013 at 11:02:55AM -0700, Bryce Harrington wrote:
> And if we've had to delay booting due to not being able to set the
> interface, fess up.

I would squash this into the previous patch. Adding an infinite loop is
unpleasant, and therefore guaranteed to be hit on every bisection.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/8] xfree86: After 2 sec, abort setting drm interface version.

[Bryce Harrington]

On Mon, Mar 18, 2013 at 11:00:55AM +0000, Chris Wilson wrote:
> On Fri, Mar 15, 2013 at 11:02:55AM -0700, Bryce Harrington wrote:
> > And if we've had to delay booting due to not being able to set the
> > interface, fess up.
> 
> I would squash this into the previous patch. Adding an infinite loop is
> unpleasant, and therefore guaranteed to be hit on every bisection.
> -Chris

Sounds good, I've resubmitted the series.

Thanks,
Bryce

[PATCH 6/8] xfree86: Fix race condition failure opening drm.

[Bryce Harrington]

If other processes have had drm open previously, xserver may attempt to
open the device too early and fail, with xserver error exit "Cannot
run in framebuffer mode" or Xorg.0.log messages about "setversion 1.4
failed".

In this situation, we're receiving back -EACCES from libdrm.  To address
this we need to re-set ourselves as the drm master, and keep trying to
set the interface until it works (or until we give up).

See https://bugs.launchpad.net/ubuntu/+source/libdrm/+bug/982889

Signed-off-by: Bryce Harrington <bryce@canonical.com>
---
 hw/xfree86/os-support/linux/lnx_platform.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c
index 4094866..bb76d90 100644
--- a/hw/xfree86/os-support/linux/lnx_platform.c
+++ b/hw/xfree86/os-support/linux/lnx_platform.c
@@ -43,8 +43,14 @@ get_drm_info(struct OdevAttributes *attribs, char *path)
 	    if (tries > 1)
 		LogMessage(X_INFO, "setversion 1.4 succeeded on try #%d\n", tries);
 	    break;
+	} if (err != -EACCES) {
+	    break;
 	}
+
 	usleep(1000);
+
+	if (!drmSetMaster(fd))
+	    LogMessage(X_INFO, "drmSetMaster succeeded\n");
     }
     if (err) {
         ErrorF("setversion 1.4 failed: %s\n", strerror(-err));
-- 
1.7.9.5

[PATCH 7/8] xfree86: Be verbose if waiting on opening the drm device

[Bryce Harrington]

Signed-off-by: Bryce Harrington <bryce@canonical.com>
---
 hw/xfree86/os-support/linux/lnx_platform.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c
index bb76d90..3386b67 100644
--- a/hw/xfree86/os-support/linux/lnx_platform.c
+++ b/hw/xfree86/os-support/linux/lnx_platform.c
@@ -43,7 +43,10 @@ get_drm_info(struct OdevAttributes *attribs, char *path)
 	    if (tries > 1)
 		LogMessage(X_INFO, "setversion 1.4 succeeded on try #%d\n", tries);
 	    break;
-	} if (err != -EACCES) {
+	} if (err == -EACCES) {
+	    if (tries % 500 == 0)
+		LogMessage(X_INFO, "waiting on drm device...\n");
+	} else {
 	    break;
 	}
 
-- 
1.7.9.5

[PATCH 8/8] xfree86: Also handle EAGAIN errors from drmSetInterfaceVersion().

[Bryce Harrington]

It has been suggested that the kernel may pass EAGAIN when the device is
unavailable.  This hasn't been seen in practice, and examination of the
function definition in libdrm suggests EAGAIN is handled internally so
would not be seen by the xserver when making this call.  So, this patch
is probably unneeded.  But include support anyway.

Signed-off-by: Bryce Harrington <bryce@canonical.com>
---
 hw/xfree86/os-support/linux/lnx_platform.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c
index 3386b67..b05719d 100644
--- a/hw/xfree86/os-support/linux/lnx_platform.c
+++ b/hw/xfree86/os-support/linux/lnx_platform.c
@@ -46,6 +46,9 @@ get_drm_info(struct OdevAttributes *attribs, char *path)
 	} if (err == -EACCES) {
 	    if (tries % 500 == 0)
 		LogMessage(X_INFO, "waiting on drm device...\n");
+	} if (err == -EAGAIN) {
+	    if (tries % 500 == 0)
+		LogMessage(X_INFO, "drm device busy...\n");
 	} else {
 	    break;
 	}
-- 
1.7.9.5