| Message ID | 1459331120-27864-9-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30 March 2016 at 10:45, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Like in > > commit 0e975980d435d58df2d430d688b8c18778b42218 > Author: Peter Antoine <peter.antoine@intel.com> > Date: Tue Jun 23 08:18:49 2015 +0100 > > drm: Turn off Legacy Context Functions > > we need to again make an exception for nouveau, but everyone else > really doesn't need this. > > Cc: Peter Antoine <peter.antoine@intel.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_bufs.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c > index d92db7007f62..e8a12a4fd400 100644 > --- a/drivers/gpu/drm/drm_bufs.c > +++ b/drivers/gpu/drm/drm_bufs.c > @@ -396,6 +396,10 @@ int drm_legacy_addmap_ioctl(struct drm_device *dev, void *data, > if (!(capable(CAP_SYS_ADMIN) || map->type == _DRM_AGP || map->type == _DRM_SHM)) > return -EPERM; > > + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) && > + drm_core_check_feature(dev, DRIVER_MODESET)) > + return -EINVAL; > + Wondering if making this the first check in the function won't be better ? We have a handful of places which preemptively check DRIVER_MODESET prior to calling drm_legacy functions and similarly some (last time I've looked) drm_legacy functions check for DRIVER_MODESET. Perhaps we can move all the checking into the drm_legacy API alone ? -Emil
On Wed, Mar 30, 2016 at 11:39:01AM +0100, Emil Velikov wrote: > On 30 March 2016 at 10:45, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Like in > > > > commit 0e975980d435d58df2d430d688b8c18778b42218 > > Author: Peter Antoine <peter.antoine@intel.com> > > Date: Tue Jun 23 08:18:49 2015 +0100 > > > > drm: Turn off Legacy Context Functions > > > > we need to again make an exception for nouveau, but everyone else > > really doesn't need this. > > > > Cc: Peter Antoine <peter.antoine@intel.com> > > Cc: Ben Skeggs <bskeggs@redhat.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_bufs.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c > > index d92db7007f62..e8a12a4fd400 100644 > > --- a/drivers/gpu/drm/drm_bufs.c > > +++ b/drivers/gpu/drm/drm_bufs.c > > @@ -396,6 +396,10 @@ int drm_legacy_addmap_ioctl(struct drm_device *dev, void *data, > > if (!(capable(CAP_SYS_ADMIN) || map->type == _DRM_AGP || map->type == _DRM_SHM)) > > return -EPERM; > > > > + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) && > > + drm_core_check_feature(dev, DRIVER_MODESET)) > > + return -EINVAL; > > + > Wondering if making this the first check in the function won't be > better ? We have a handful of places which preemptively check > DRIVER_MODESET prior to calling drm_legacy functions and similarly > some (last time I've looked) drm_legacy functions check for > DRIVER_MODESET. Perhaps we can move all the checking into the > drm_legacy API alone ? This is an ioctl handler, so there's not really any caller we can move this to. In general I'm split, and I just put checks like these wherever it makes sense, pulling them out into callers if there's an entire pile of them who all want the same checks. In the end I don't think it matters, as long as we dutifully combine drm_legacy_* with such checks, so that it's _really_ all dead code for modern drives. -Daniel
On 14 April 2016 at 11:06, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Mar 30, 2016 at 11:39:01AM +0100, Emil Velikov wrote: >> On 30 March 2016 at 10:45, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> > Like in >> > >> > commit 0e975980d435d58df2d430d688b8c18778b42218 >> > Author: Peter Antoine <peter.antoine@intel.com> >> > Date: Tue Jun 23 08:18:49 2015 +0100 >> > >> > drm: Turn off Legacy Context Functions >> > >> > we need to again make an exception for nouveau, but everyone else >> > really doesn't need this. >> > >> > Cc: Peter Antoine <peter.antoine@intel.com> >> > Cc: Ben Skeggs <bskeggs@redhat.com> >> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> > --- >> > drivers/gpu/drm/drm_bufs.c | 12 ++++++++++++ >> > 1 file changed, 12 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c >> > index d92db7007f62..e8a12a4fd400 100644 >> > --- a/drivers/gpu/drm/drm_bufs.c >> > +++ b/drivers/gpu/drm/drm_bufs.c >> > @@ -396,6 +396,10 @@ int drm_legacy_addmap_ioctl(struct drm_device *dev, void *data, >> > if (!(capable(CAP_SYS_ADMIN) || map->type == _DRM_AGP || map->type == _DRM_SHM)) >> > return -EPERM; >> > >> > + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) && >> > + drm_core_check_feature(dev, DRIVER_MODESET)) >> > + return -EINVAL; >> > + >> Wondering if making this the first check in the function won't be >> better ? We have a handful of places which preemptively check >> DRIVER_MODESET prior to calling drm_legacy functions and similarly >> some (last time I've looked) drm_legacy functions check for >> DRIVER_MODESET. Perhaps we can move all the checking into the >> drm_legacy API alone ? > > This is an ioctl handler, so there's not really any caller we can move > this to. Indeed. One could extract the legacy ioctls into separate table and/or use a function alike drm_ioctl_permit() to generalise things. It will allow clear and obvious separation of things, although the particular above will make things a bit annoying. > In general I'm split, and I just put checks like these wherever > it makes sense, pulling them out into callers if there's an entire pile of > them who all want the same checks. > > In the end I don't think it matters, as long as we dutifully combine > drm_legacy_* with such checks, so that it's _really_ all dead code for > modern drives. I'd suspect that there's a few bits that are missing the check, plus going through might be a bit time consuming. Thus thinking of a 'generic' way to handle things. Just thinking out loud :-) -Emil
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index d92db7007f62..e8a12a4fd400 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -396,6 +396,10 @@ int drm_legacy_addmap_ioctl(struct drm_device *dev, void *data, if (!(capable(CAP_SYS_ADMIN) || map->type == _DRM_AGP || map->type == _DRM_SHM)) return -EPERM; + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) && + drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + err = drm_addmap_core(dev, map->offset, map->size, map->type, map->flags, &maplist); @@ -438,6 +442,10 @@ int drm_legacy_getmap_ioctl(struct drm_device *dev, void *data, int idx; int i; + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) && + drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + idx = map->offset; if (idx < 0) return -EINVAL; @@ -569,6 +577,10 @@ int drm_legacy_rmmap_ioctl(struct drm_device *dev, void *data, struct drm_map_list *r_list; int ret; + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) && + drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + mutex_lock(&dev->struct_mutex); list_for_each_entry(r_list, &dev->maplist, head) { if (r_list->map &&
Like in commit 0e975980d435d58df2d430d688b8c18778b42218 Author: Peter Antoine <peter.antoine@intel.com> Date: Tue Jun 23 08:18:49 2015 +0100 drm: Turn off Legacy Context Functions we need to again make an exception for nouveau, but everyone else really doesn't need this. Cc: Peter Antoine <peter.antoine@intel.com> Cc: Ben Skeggs <bskeggs@redhat.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_bufs.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)