Re: [PATCH 08/11] drm: Enforce unlocked ioctl operation for kms driver ioctls

[David Herrmann <dh.herrmann@gmail.com>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel