diff mbox

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

Message ID 20180414115318.14500-18-noralf@tronnes.org (mailing list archive)
State New, archived
Headers show

Commit Message

Noralf Trønnes April 14, 2018, 11:53 a.m. UTC 
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 <daniel.vetter@ffwll.ch>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 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(-)

Comments

Daniel Vetter April 16, 2018, 8:45 a.m. UTC | #1 
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 <daniel.vetter@ffwll.ch>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  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(&dev->master_mutex);
> +	if (dev->master) {
> +		mutex_unlock(&dev->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(&dev->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 <drm/drm_device.h>
>  #include <drm/drm_modes.h>
>  
> +#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
>  		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(&fb_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(&helper->lock);
> -		ret = drm_client_display_restore(helper->display);
> +		ret = drm_client_display_restore(helper->display, true);
>  		if (ret)
>  			error = true;
>  		mutex_unlock(&helper->lock);
> @@ -1254,7 +1254,7 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
>  
>  	pan_set(fb_helper, var->xoffset, var->yoffset);
>  
> -	ret = drm_client_display_restore(fb_helper->display);
> +	ret = drm_client_display_restore(fb_helper->display, false);
>  	if (!ret) {
>  		info->var.xoffset = var->xoffset;
>  		info->var.yoffset = var->yoffset;
> @@ -1423,7 +1423,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  
>  		/* First time: disable all crtc's.. */
>  		if (!fb_helper->deferred_setup && !READ_ONCE(fb_helper->dev->master))
> -			drm_client_display_restore(fb_helper->display);
> +			drm_client_display_restore(fb_helper->display, false);
>  		return -EAGAIN;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 3f5d7706bcc9..f38dcaf139d7 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -92,6 +92,8 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
>  			 struct drm_file *file_priv);
>  int drm_master_open(struct drm_file *file_priv);
>  void drm_master_release(struct drm_file *file_priv);
> +bool drm_master_block(struct drm_device *dev);
> +void drm_master_unblock(struct drm_device *dev);
>  
>  /* drm_sysfs.c */
>  extern struct class *drm_class;
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 27d2a46cd94a..3befd879a0b0 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -46,8 +46,8 @@ drm_client_display_find_modeset(struct drm_client_display *display, struct drm_c
>  bool drm_client_display_panel_rotation(struct drm_connector *connector,
>  				       struct drm_plane *plane,
>  				       unsigned int *rotation);
> -int drm_client_display_restore(struct drm_client_display *display);
> -void drm_client_display_dpms(struct drm_client_display *display, int mode);
> +int drm_client_display_restore(struct drm_client_display *display, bool force);
> +int drm_client_display_dpms(struct drm_client_display *display, int mode);
>  struct drm_client_display *
>  drm_client_find_display(struct drm_device *dev, unsigned int width, unsigned int height);
>  
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

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(&dev->master_mutex);
+	if (dev->master) {
+		mutex_unlock(&dev->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(&dev->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 <drm/drm_device.h>
 #include <drm/drm_modes.h>
 
+#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(&fb_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(&helper->lock);
-		ret = drm_client_display_restore(helper->display);
+		ret = drm_client_display_restore(helper->display, true);
 		if (ret)
 			error = true;
 		mutex_unlock(&helper->lock);
@@ -1254,7 +1254,7 @@  static int pan_display_atomic(struct fb_var_screeninfo *var,
 
 	pan_set(fb_helper, var->xoffset, var->yoffset);
 
-	ret = drm_client_display_restore(fb_helper->display);
+	ret = drm_client_display_restore(fb_helper->display, false);
 	if (!ret) {
 		info->var.xoffset = var->xoffset;
 		info->var.yoffset = var->yoffset;
@@ -1423,7 +1423,7 @@  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 
 		/* First time: disable all crtc's.. */
 		if (!fb_helper->deferred_setup && !READ_ONCE(fb_helper->dev->master))
-			drm_client_display_restore(fb_helper->display);
+			drm_client_display_restore(fb_helper->display, false);
 		return -EAGAIN;
 	}
 
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 3f5d7706bcc9..f38dcaf139d7 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -92,6 +92,8 @@  int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv);
 int drm_master_open(struct drm_file *file_priv);
 void drm_master_release(struct drm_file *file_priv);
+bool drm_master_block(struct drm_device *dev);
+void drm_master_unblock(struct drm_device *dev);
 
 /* drm_sysfs.c */
 extern struct class *drm_class;
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
index 27d2a46cd94a..3befd879a0b0 100644
--- a/include/drm/drm_client.h
+++ b/include/drm/drm_client.h
@@ -46,8 +46,8 @@  drm_client_display_find_modeset(struct drm_client_display *display, struct drm_c
 bool drm_client_display_panel_rotation(struct drm_connector *connector,
 				       struct drm_plane *plane,
 				       unsigned int *rotation);
-int drm_client_display_restore(struct drm_client_display *display);
-void drm_client_display_dpms(struct drm_client_display *display, int mode);
+int drm_client_display_restore(struct drm_client_display *display, bool force);
+int drm_client_display_dpms(struct drm_client_display *display, int mode);
 struct drm_client_display *
 drm_client_find_display(struct drm_device *dev, unsigned int width, unsigned int height);