[08/11] drm: Enforce unlocked ioctl operation for kms driver ioctls
diff mbox

Message ID 1441713391-24732-9-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Sept. 8, 2015, 11:56 a.m. UTC
With the prep patches for i915 all kms drivers either have
DRM_UNLOCKED on all their ioctls. Or the ioctl always directly returns
with an invariant return value when in modeset mode. But that's only
the case for i915 and radeon. The drm core ioctls are unfortunately
too much a mess still to dare this.

Follow-up patches will remove DRM_UNLOCKED from all kms drivers to
prove that this is indeed the case.

Also update the documentation.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/DocBook/drm.tmpl | 4 +++-
 drivers/gpu/drm/drm_ioctl.c    | 6 +++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Gustavo Padovan Sept. 8, 2015, 6:45 p.m. UTC | #1
Hi Daniel,

2015-09-08 Daniel Vetter <daniel.vetter@ffwll.ch>:

> With the prep patches for i915 all kms drivers either have
> DRM_UNLOCKED on all their ioctls. Or the ioctl always directly returns
> with an invariant return value when in modeset mode. But that's only
> the case for i915 and radeon. The drm core ioctls are unfortunately
> too much a mess still to dare this.
> 
> Follow-up patches will remove DRM_UNLOCKED from all kms drivers to
> prove that this is indeed the case.
> 
> Also update the documentation.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/DocBook/drm.tmpl | 4 +++-
>  drivers/gpu/drm/drm_ioctl.c    | 6 +++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)

Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

	Gustavo
David Herrmann Sept. 28, 2015, 3:21 p.m. UTC | #2
Hi

On Tue, Sep 8, 2015 at 1:56 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> With the prep patches for i915 all kms drivers either have
> DRM_UNLOCKED on all their ioctls. Or the ioctl always directly returns
> with an invariant return value when in modeset mode. But that's only
> the case for i915 and radeon. The drm core ioctls are unfortunately
> too much a mess still to dare this.
>
> Follow-up patches will remove DRM_UNLOCKED from all kms drivers to
> prove that this is indeed the case.
>
> Also update the documentation.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

drm_setclientcap() should probably lock _something_. It's not very
crucial, but I think we should guarantee consistency when setting
multiple values. struct_mutex of the corresponding DRM device sounds
sufficient, though not very promising. But drm_file doesn't have any
suitable lock..

drm_setversion(): This definitely needs _some_ lock. DRM_MASTER is not
reliable (we never merged the master-reliability patches).

...skipping review of the other ioctls...
...re-reading patch description...

So this patch is just meant to drop DRM_UNLOCKED from driver-ioctls,
right? See below.

> ---
>  Documentation/DocBook/drm.tmpl | 4 +++-
>  drivers/gpu/drm/drm_ioctl.c    | 6 +++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index cfb43203a6a7..55dc106686df 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -3747,7 +3747,9 @@ int num_ioctls;</synopsis>
>             </para></listitem>
>              <listitem><para>
>               DRM_UNLOCKED - The ioctl handler will be called without locking
> -             the DRM global mutex
> +             the DRM global mutex. This is the enforced default for kms drivers
> +             (i.e. using the DRIVER_MODESET flag) and hence shouldn't be used
> +             any more for new drivers.
>             </para></listitem>
>           </itemizedlist>
>         </para>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 75df8ea87cd7..a5a4aa89b1b4 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -728,6 +728,7 @@ long drm_ioctl(struct file *filp,
>         }
>
>         retcode = drm_ioctl_permit(ioctl->flags, file_priv);
> +

Weird new-line. I actually prefer the previous style, anyway.

>         if (unlikely(retcode))
>                 goto err_i1;
>
> @@ -755,7 +756,10 @@ long drm_ioctl(struct file *filp,
>                 memset(kdata, 0, usize);
>         }
>
> -       if (ioctl->flags & DRM_UNLOCKED)
> +       /* Enforce sane locking for kms driver ioctls. Core ioctls are
> +        * too messy still. */
> +       if (drm_core_check_feature(dev, DRIVER_MODESET) ||
> +           (ioctl->flags & DRM_UNLOCKED))
>                 retcode = func(dev, kdata, file_priv);

This looks.. weird. Now we *never* lock *anything* for MODESET
drivers? This contradicts your commit-message, which rather tells me
you want _driver_ ioctls of modeset drivers to never rely on
drm_global_mutex. Several ways to make that work (and I'd review it
gladly), but this looks.. weird.

Care to elaborate?

Thanks
David

>         else {
>                 mutex_lock(&drm_global_mutex);
> --
> 2.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch
diff mbox

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index cfb43203a6a7..55dc106686df 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -3747,7 +3747,9 @@  int num_ioctls;</synopsis>
 	    </para></listitem>
             <listitem><para>
 	      DRM_UNLOCKED - The ioctl handler will be called without locking
-	      the DRM global mutex
+	      the DRM global mutex. This is the enforced default for kms drivers
+	      (i.e. using the DRIVER_MODESET flag) and hence shouldn't be used
+	      any more for new drivers.
 	    </para></listitem>
 	  </itemizedlist>
 	</para>
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 75df8ea87cd7..a5a4aa89b1b4 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -728,6 +728,7 @@  long drm_ioctl(struct file *filp,
 	}
 
 	retcode = drm_ioctl_permit(ioctl->flags, file_priv);
+
 	if (unlikely(retcode))
 		goto err_i1;
 
@@ -755,7 +756,10 @@  long drm_ioctl(struct file *filp,
 		memset(kdata, 0, usize);
 	}
 
-	if (ioctl->flags & DRM_UNLOCKED)
+	/* Enforce sane locking for kms driver ioctls. Core ioctls are
+	 * too messy still. */
+	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
+	    (ioctl->flags & DRM_UNLOCKED))
 		retcode = func(dev, kdata, file_priv);
 	else {
 		mutex_lock(&drm_global_mutex);