Re: [Intel-gfx] [RFC v4 17/25] drm/client: Bail out if there's a DRM master

[Daniel Vetter]

On Sat, Apr 14, 2018 at 01:53:10PM +0200, Noralf Trønnes wrote:
> If there's a DRM master, return -EBUSY.
> Block userspace from becoming master by taking the master lock while
> the client is setting the mode.
> 
> Suggested-by: Daniel Vetter 
> Signed-off-by: Noralf Trønnes 
> ---
>  drivers/gpu/drm/drm_auth.c  | 33 +
>  drivers/gpu/drm/drm_client.c| 34 +-
>  drivers/gpu/drm/drm_fb_helper.c |  8 
>  drivers/gpu/drm/drm_internal.h  |  2 ++
>  include/drm/drm_client.h|  4 ++--
>  5 files changed, 70 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index d9c0f7573905..d656d0d93da3 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -366,3 +366,36 @@ void drm_master_put(struct drm_master **master)
>   *master = NULL;
>  }
>  EXPORT_SYMBOL(drm_master_put);
> +
> +/**
> + * drm_master_block - Block DRM master operations
> + * @dev: DRM device
> + *
> + * This function checks if there is a master on @dev. If there is no master 
> it
> + * blocks anyone from becoming master. In-kernel clients can use this to know
> + * when they can act as master. Use drm_master_unblock() to unblock.
> + *
> + * Returns:
> + * True if there is no master, false otherwise.
> + */
> +bool drm_master_block(struct drm_device *dev)
> +{
> + mutex_lock(>master_mutex);
> + if (dev->master) {
> + mutex_unlock(>master_mutex);
> + return false;
> + }
> +
> + return true;
> +}
> +
> +/**
> + * drm_master_unblock - Unblock DRM master operations
> + * @dev: DRM device
> + *
> + * Unblock and allow userspace to become master.
> + */
> +void drm_master_unblock(struct drm_device *dev)
> +{
> + mutex_unlock(>master_mutex);
> +}
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 27818a467b09..764c556630b8 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -18,6 +18,8 @@
>  #include 
>  #include 
>  
> +#include "drm_internal.h"
> +
>  struct drm_client_display_offset {
>   int x, y;
>  };
> @@ -313,18 +315,30 @@ static int drm_client_display_restore_legacy(struct 
> drm_client_display *display)
>  /**
>   * drm_client_display_restore() - Restore client display
>   * @display: Client display
> + * @force: If true, restore even if there's a DRM master

This smells a bit like bad interface design. Essentially this is a "should
I lock or not" flag, and if locking should/needs to be done by at least
some callers, then all of them should do that.

To make sure no one screws up, you can add a

drm_client_masters_are_blocked()
{
lockdep_assert_held(client->dev->master_lock);
}

helper and call it at all the places you really want to have all other
masters excluded.

And locking at the code, we really do want to pull the
master_block/unblock critical section all the way out, to make sure that
an fbdev action only ever works or doesn't do any kind of damage at all.
We really don't want another master to take over while fbdev emulation is
doing stuff - drm_client_master_block/unblock is meant to fix these races.
-Daniel

>   *
>   * Restore client display using the current modeset configuration.
>   *
>   * Return:
>   * Zero on succes or negative error code on failure.
>   */
> -int drm_client_display_restore(struct drm_client_display *display)
> +int drm_client_display_restore(struct drm_client_display *display, bool 
> force)
>  {
> - if (drm_drv_uses_atomic_modeset(display->dev))
> - return drm_client_display_restore_atomic(display, true);
> + struct drm_device *dev = display->dev;
> + int ret;
> +
> + if (!force && !drm_master_block(dev))
> + return -EBUSY;
> +
> + if (drm_drv_uses_atomic_modeset(dev))
> + ret = drm_client_display_restore_atomic(display, true);
>   else
> - return drm_client_display_restore_legacy(display);
> + ret = drm_client_display_restore_legacy(display);
> +
> + if (!force)
> + drm_master_unblock(dev);
> +
> + return ret;
>  }
>  EXPORT_SYMBOL(drm_client_display_restore);
>  
> @@ -354,13 +368,23 @@ static void drm_client_display_dpms_legacy(struct 
> drm_client_display *display, i
>   * drm_client_display_dpms() - Set display DPMS mode
>   * @display: Client display
>   * @mode: DPMS mode
> + *
> + * Returns:
> + * Zero on success, -EBUSY if there's a DRM master.
>   */
> -void drm_client_display_dpms(struct drm_client_display *display, int mode)
> +int drm_client_display_dpms(struct drm_client_display *display, int mode)
>  {
> + if (!drm_master_block(display->dev))
> + return -EBUSY;
> +
>   if (drm_drv_uses_atomic_modeset(display->dev))
>   drm_client_display_restore_atomic(display, mode == 
> DRM_MODE_DPMS_ON);
>   else
>   

[RFC v4 17/25] drm/client: Bail out if there's a DRM master

[Noralf Trønnes]

If there's a DRM master, return -EBUSY.
Block userspace from becoming master by taking the master lock while
the client is setting the mode.

Suggested-by: Daniel Vetter 
Signed-off-by: Noralf Trønnes 
---
 drivers/gpu/drm/drm_auth.c  | 33 +
 drivers/gpu/drm/drm_client.c| 34 +-
 drivers/gpu/drm/drm_fb_helper.c |  8 
 drivers/gpu/drm/drm_internal.h  |  2 ++
 include/drm/drm_client.h|  4 ++--
 5 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index d9c0f7573905..d656d0d93da3 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -366,3 +366,36 @@ void drm_master_put(struct drm_master **master)
*master = NULL;
 }
 EXPORT_SYMBOL(drm_master_put);
+
+/**
+ * drm_master_block - Block DRM master operations
+ * @dev: DRM device
+ *
+ * This function checks if there is a master on @dev. If there is no master it
+ * blocks anyone from becoming master. In-kernel clients can use this to know
+ * when they can act as master. Use drm_master_unblock() to unblock.
+ *
+ * Returns:
+ * True if there is no master, false otherwise.
+ */
+bool drm_master_block(struct drm_device *dev)
+{
+   mutex_lock(>master_mutex);
+   if (dev->master) {
+   mutex_unlock(>master_mutex);
+   return false;
+   }
+
+   return true;
+}
+
+/**
+ * drm_master_unblock - Unblock DRM master operations
+ * @dev: DRM device
+ *
+ * Unblock and allow userspace to become master.
+ */
+void drm_master_unblock(struct drm_device *dev)
+{
+   mutex_unlock(>master_mutex);
+}
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 27818a467b09..764c556630b8 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -18,6 +18,8 @@
 #include 
 #include 
 
+#include "drm_internal.h"
+
 struct drm_client_display_offset {
int x, y;
 };
@@ -313,18 +315,30 @@ static int drm_client_display_restore_legacy(struct 
drm_client_display *display)
 /**
  * drm_client_display_restore() - Restore client display
  * @display: Client display
+ * @force: If true, restore even if there's a DRM master
  *
  * Restore client display using the current modeset configuration.
  *
  * Return:
  * Zero on succes or negative error code on failure.
  */
-int drm_client_display_restore(struct drm_client_display *display)
+int drm_client_display_restore(struct drm_client_display *display, bool force)
 {
-   if (drm_drv_uses_atomic_modeset(display->dev))
-   return drm_client_display_restore_atomic(display, true);
+   struct drm_device *dev = display->dev;
+   int ret;
+
+   if (!force && !drm_master_block(dev))
+   return -EBUSY;
+
+   if (drm_drv_uses_atomic_modeset(dev))
+   ret = drm_client_display_restore_atomic(display, true);
else
-   return drm_client_display_restore_legacy(display);
+   ret = drm_client_display_restore_legacy(display);
+
+   if (!force)
+   drm_master_unblock(dev);
+
+   return ret;
 }
 EXPORT_SYMBOL(drm_client_display_restore);
 
@@ -354,13 +368,23 @@ static void drm_client_display_dpms_legacy(struct 
drm_client_display *display, i
  * drm_client_display_dpms() - Set display DPMS mode
  * @display: Client display
  * @mode: DPMS mode
+ *
+ * Returns:
+ * Zero on success, -EBUSY if there's a DRM master.
  */
-void drm_client_display_dpms(struct drm_client_display *display, int mode)
+int drm_client_display_dpms(struct drm_client_display *display, int mode)
 {
+   if (!drm_master_block(display->dev))
+   return -EBUSY;
+
if (drm_drv_uses_atomic_modeset(display->dev))
drm_client_display_restore_atomic(display, mode == 
DRM_MODE_DPMS_ON);
else
drm_client_display_dpms_legacy(display, mode);
+
+   drm_master_unblock(display->dev);
+
+   return 0;
 }
 EXPORT_SYMBOL(drm_client_display_dpms);
 
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 01d8840930a3..98e5bc92c9f2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -181,7 +181,7 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct 
drm_fb_helper *fb_helper)
return 0;
 
mutex_lock(_helper->lock);
-   ret = drm_client_display_restore(fb_helper->display);
+   ret = drm_client_display_restore(fb_helper->display, false);
 
do_delayed = fb_helper->delayed_hotplug;
if (do_delayed)
@@ -243,7 +243,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
continue;
 
mutex_lock(>lock);
-   ret = drm_client_display_restore(helper->display);
+   ret = drm_client_display_restore(helper->display, true);
if (ret)

[RFC v4 17/25] drm/client: Bail out if there's a DRM master

[Noralf Trønnes]

If there's a DRM master, return -EBUSY.
Block userspace from becoming master by taking the master lock while
the client is setting the mode.

Suggested-by: Daniel Vetter 
Signed-off-by: Noralf Trønnes 
---
 drivers/gpu/drm/drm_auth.c  | 33 +
 drivers/gpu/drm/drm_client.c| 34 +-
 drivers/gpu/drm/drm_fb_helper.c |  8 
 drivers/gpu/drm/drm_internal.h  |  2 ++
 include/drm/drm_client.h|  4 ++--
 5 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index d9c0f7573905..d656d0d93da3 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -366,3 +366,36 @@ void drm_master_put(struct drm_master **master)
*master = NULL;
 }
 EXPORT_SYMBOL(drm_master_put);
+
+/**
+ * drm_master_block - Block DRM master operations
+ * @dev: DRM device
+ *
+ * This function checks if there is a master on @dev. If there is no master it
+ * blocks anyone from becoming master. In-kernel clients can use this to know
+ * when they can act as master. Use drm_master_unblock() to unblock.
+ *
+ * Returns:
+ * True if there is no master, false otherwise.
+ */
+bool drm_master_block(struct drm_device *dev)
+{
+   mutex_lock(>master_mutex);
+   if (dev->master) {
+   mutex_unlock(>master_mutex);
+   return false;
+   }
+
+   return true;
+}
+
+/**
+ * drm_master_unblock - Unblock DRM master operations
+ * @dev: DRM device
+ *
+ * Unblock and allow userspace to become master.
+ */
+void drm_master_unblock(struct drm_device *dev)
+{
+   mutex_unlock(>master_mutex);
+}
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 27818a467b09..764c556630b8 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -18,6 +18,8 @@
 #include 
 #include 
 
+#include "drm_internal.h"
+
 struct drm_client_display_offset {
int x, y;
 };
@@ -313,18 +315,30 @@ static int drm_client_display_restore_legacy(struct 
drm_client_display *display)
 /**
  * drm_client_display_restore() - Restore client display
  * @display: Client display
+ * @force: If true, restore even if there's a DRM master
  *
  * Restore client display using the current modeset configuration.
  *
  * Return:
  * Zero on succes or negative error code on failure.
  */
-int drm_client_display_restore(struct drm_client_display *display)
+int drm_client_display_restore(struct drm_client_display *display, bool force)
 {
-   if (drm_drv_uses_atomic_modeset(display->dev))
-   return drm_client_display_restore_atomic(display, true);
+   struct drm_device *dev = display->dev;
+   int ret;
+
+   if (!force && !drm_master_block(dev))
+   return -EBUSY;
+
+   if (drm_drv_uses_atomic_modeset(dev))
+   ret = drm_client_display_restore_atomic(display, true);
else
-   return drm_client_display_restore_legacy(display);
+   ret = drm_client_display_restore_legacy(display);
+
+   if (!force)
+   drm_master_unblock(dev);
+
+   return ret;
 }
 EXPORT_SYMBOL(drm_client_display_restore);
 
@@ -354,13 +368,23 @@ static void drm_client_display_dpms_legacy(struct 
drm_client_display *display, i
  * drm_client_display_dpms() - Set display DPMS mode
  * @display: Client display
  * @mode: DPMS mode
+ *
+ * Returns:
+ * Zero on success, -EBUSY if there's a DRM master.
  */
-void drm_client_display_dpms(struct drm_client_display *display, int mode)
+int drm_client_display_dpms(struct drm_client_display *display, int mode)
 {
+   if (!drm_master_block(display->dev))
+   return -EBUSY;
+
if (drm_drv_uses_atomic_modeset(display->dev))
drm_client_display_restore_atomic(display, mode == 
DRM_MODE_DPMS_ON);
else
drm_client_display_dpms_legacy(display, mode);
+
+   drm_master_unblock(display->dev);
+
+   return 0;
 }
 EXPORT_SYMBOL(drm_client_display_dpms);
 
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 01d8840930a3..98e5bc92c9f2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -181,7 +181,7 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct 
drm_fb_helper *fb_helper)
return 0;
 
mutex_lock(_helper->lock);
-   ret = drm_client_display_restore(fb_helper->display);
+   ret = drm_client_display_restore(fb_helper->display, false);
 
do_delayed = fb_helper->delayed_hotplug;
if (do_delayed)
@@ -243,7 +243,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
continue;
 
mutex_lock(>lock);
-   ret = drm_client_display_restore(helper->display);
+   ret = drm_client_display_restore(helper->display, true);
if (ret)

[RFC v4 17/25] drm/client: Bail out if there's a DRM master

[Noralf Trønnes]

If there's a DRM master, return -EBUSY.
Block userspace from becoming master by taking the master lock while
the client is setting the mode.

Suggested-by: Daniel Vetter 
Signed-off-by: Noralf Trønnes 
---
 drivers/gpu/drm/drm_auth.c  | 33 +
 drivers/gpu/drm/drm_client.c| 34 +-
 drivers/gpu/drm/drm_fb_helper.c |  8 
 drivers/gpu/drm/drm_internal.h  |  2 ++
 include/drm/drm_client.h|  4 ++--
 5 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index d9c0f7573905..d656d0d93da3 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -366,3 +366,36 @@ void drm_master_put(struct drm_master **master)
*master = NULL;
 }
 EXPORT_SYMBOL(drm_master_put);
+
+/**
+ * drm_master_block - Block DRM master operations
+ * @dev: DRM device
+ *
+ * This function checks if there is a master on @dev. If there is no master it
+ * blocks anyone from becoming master. In-kernel clients can use this to know
+ * when they can act as master. Use drm_master_unblock() to unblock.
+ *
+ * Returns:
+ * True if there is no master, false otherwise.
+ */
+bool drm_master_block(struct drm_device *dev)
+{
+   mutex_lock(>master_mutex);
+   if (dev->master) {
+   mutex_unlock(>master_mutex);
+   return false;
+   }
+
+   return true;
+}
+
+/**
+ * drm_master_unblock - Unblock DRM master operations
+ * @dev: DRM device
+ *
+ * Unblock and allow userspace to become master.
+ */
+void drm_master_unblock(struct drm_device *dev)
+{
+   mutex_unlock(>master_mutex);
+}
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 27818a467b09..764c556630b8 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -18,6 +18,8 @@
 #include 
 #include 
 
+#include "drm_internal.h"
+
 struct drm_client_display_offset {
int x, y;
 };
@@ -313,18 +315,30 @@ static int drm_client_display_restore_legacy(struct 
drm_client_display *display)
 /**
  * drm_client_display_restore() - Restore client display
  * @display: Client display
+ * @force: If true, restore even if there's a DRM master
  *
  * Restore client display using the current modeset configuration.
  *
  * Return:
  * Zero on succes or negative error code on failure.
  */
-int drm_client_display_restore(struct drm_client_display *display)
+int drm_client_display_restore(struct drm_client_display *display, bool force)
 {
-   if (drm_drv_uses_atomic_modeset(display->dev))
-   return drm_client_display_restore_atomic(display, true);
+   struct drm_device *dev = display->dev;
+   int ret;
+
+   if (!force && !drm_master_block(dev))
+   return -EBUSY;
+
+   if (drm_drv_uses_atomic_modeset(dev))
+   ret = drm_client_display_restore_atomic(display, true);
else
-   return drm_client_display_restore_legacy(display);
+   ret = drm_client_display_restore_legacy(display);
+
+   if (!force)
+   drm_master_unblock(dev);
+
+   return ret;
 }
 EXPORT_SYMBOL(drm_client_display_restore);
 
@@ -354,13 +368,23 @@ static void drm_client_display_dpms_legacy(struct 
drm_client_display *display, i
  * drm_client_display_dpms() - Set display DPMS mode
  * @display: Client display
  * @mode: DPMS mode
+ *
+ * Returns:
+ * Zero on success, -EBUSY if there's a DRM master.
  */
-void drm_client_display_dpms(struct drm_client_display *display, int mode)
+int drm_client_display_dpms(struct drm_client_display *display, int mode)
 {
+   if (!drm_master_block(display->dev))
+   return -EBUSY;
+
if (drm_drv_uses_atomic_modeset(display->dev))
drm_client_display_restore_atomic(display, mode == 
DRM_MODE_DPMS_ON);
else
drm_client_display_dpms_legacy(display, mode);
+
+   drm_master_unblock(display->dev);
+
+   return 0;
 }
 EXPORT_SYMBOL(drm_client_display_dpms);
 
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 01d8840930a3..98e5bc92c9f2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -181,7 +181,7 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct 
drm_fb_helper *fb_helper)
return 0;
 
mutex_lock(_helper->lock);
-   ret = drm_client_display_restore(fb_helper->display);
+   ret = drm_client_display_restore(fb_helper->display, false);
 
do_delayed = fb_helper->delayed_hotplug;
if (do_delayed)
@@ -243,7 +243,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
continue;
 
mutex_lock(>lock);
-   ret = drm_client_display_restore(helper->display);
+   ret = drm_client_display_restore(helper->display, true);
if (ret)