On Fri, Dec 09, 2016 at 02:54:34PM +0000, Emil Velikov wrote:
On 9 December 2016 at 14:19, Daniel Vetter daniel.vetter@ffwll.ch wrote:
No one looks at the major/minor versions except the unique/busid stuff. If we protect that with the master_mutex (since it also affects the unique of each master, oh well) we can mark these two IOCTL with DRM_UNLOCKED.
While doing this I realized that the comment for the magic_map is outdated, I've forgotten to update it in:
commit d2b34ee62b409a03c6fe43c07b779983be51d017 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 17 09:33:21 2016 +0200
drm: Protect authmagic with master_mutex
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Emil Velikov emil.l.velikov@gmail.com
There's a comment/wild idea below, but the patch itself is
Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
@@ -340,6 +344,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f struct drm_set_version *sv = data; int if_version, retcode = 0;
mutex_lock(&dev->master_mutex); if (sv->drm_di_major != -1) { if (sv->drm_di_major != DRM_IF_MAJOR || sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {
@@ -374,6 +379,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f sv->drm_di_minor = DRM_IF_MINOR; sv->drm_dd_major = dev->driver->major; sv->drm_dd_minor = dev->driver->minor;
mutex_unlock(&dev->master_mutex);
Was going to shout "error paths" only to realise that we clobber the user provided storage, even on error. Realistically one could use that info, but from a _very_ quick look I did see any.
Seems like we have all the "used by KMS drivers" ioctls converted to DRM_UNLOCKED, so I'm just wondering: Is it worth, respinning things to:
- annotate the legacy-only ioctls (mostly as sanity-check) and do
global lock on those, dropping any DRM_UNLOCKED references.
Yeah, that would make sense as a follow-up after this series. There's already a patch to enforce lockless behaviour for all new ioctls. We'd need a DRM_LEGACY_LOCKED to annotate the few places that still need the drm bkl.
- if !legacy-only ioctl, bail out from drm_ioctl() (again, mostly a
sanity check) and dropping the boiler plate from the individual ioctls. Not too sure if this one is correct though.
The boiler-plate isn't the same everywhere, sometimes it checks DRIVER_LEGACY, sometimes also the nouveau special case and the return code isn't the same everywhere (I'm not sure where that all matters). My preference for this would be to leave it as is. -Daniel
if (drm_core_check_feature(dev, DRIVER_MODESET)) return 0;
Thanks Emil