Hey
On Fri, Dec 9, 2016 at 11:42 AM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> We thought that no userspace is using them, but oops libdrm is using
> them to figure out whether a driver supports modesetting. Check out
> drmCheckModesettingSupported but maybe don't because it's horrible and
> totally runs counter to where we want to go with libdrm device
> handling. The function looks in the device hierarchy for whether
> controlD* exist using the following format string:
>
> /sys/bus/pci/devices/%04x:%02x:%02x.%d/drm/controlD%d
>
> The "/drm" subdirectory is the glue directory from the sysfs class
> stuff, and the only way to get at it seems to through
> kdev->kobj.parent (when kdev is represents e.g. the card0 chardev
> instance in sysfs). Git grep says we're not the only ones touching
> that, so I hope it's ok we dig into such internals - I couldn't find a
> proper interface for getting at the glue directory.
>
> Quick git grep shows that at least -amdgpu, -ati and UXA in -intel are
> using this. -modesetting and SNA in -intel do not, which is why this
> didn't blow up earlier.
>
> Oh well, we need to keep it working, and the simplest way is to add a
> symlink at the right place in debugfs from controlD* to card*.
>
> Fixes: 8a357d10043c ("drm: Nerf DRM_CONTROL nodes")
> Cc: Dave Airlie <airlied@xxxxxxxxx>
> Reported-by: Alex Deucher <alexander.deucher@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Cc: David Herrmann <dh.herrmann@xxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_drv.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 4ec61ac27477..5baec7202558 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -650,6 +650,47 @@ void drm_dev_unref(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_dev_unref);
>
> +static int create_compat_control_link(struct drm_device *dev)
> +{
> + struct drm_minor *minor;
> + char *name;
> + int ret;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return 0;
> +
> + minor = *drm_minor_get_slot(dev, DRM_MINOR_PRIMARY);
if (!minor)
return -EINVAL;
(I don't insist on that one, but see below)
> + name = kasprintf(GFP_KERNEL, "controlD%d", minor->index);
Wrong index, right? This would use '0' rather than '64'. Probably does
not matter, though.
> + if (!name)
> + return -ENOMEM;
> +
> + ret = sysfs_create_link(minor->kdev->kobj.parent,
> + &minor->kdev->kobj,
> + name);
> +
> + kfree(name);
> +
> + return ret;
> +}
> +
> +static void remove_compat_control_link(struct drm_device *dev)
> +{
> + struct drm_minor *minor;
> + char *name;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return;
> +
> + minor = *drm_minor_get_slot(dev, DRM_MINOR_PRIMARY);
if (!minor)
return;
I mean, our error-paths often coalesce multiple similar destructor
calls, and expect the destructor calls. So no reason to assume the
primary-node is initialized. In fact, our drm_dev_register() creates
the primary node last, so if the render-node creation fails we will
call here and NULL-deref.
Otherwise looks good to me.
Thanks
David
> + name = kasprintf(GFP_KERNEL, "controlD%d", minor->index);
> + if (!name)
> + return;
> +
> + sysfs_remove_link(minor->kdev->kobj.parent, name);
> +
> + kfree(name);
> +}
> +
> /**
> * drm_dev_register - Register DRM device
> * @dev: Device to register
> @@ -688,6 +729,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> if (ret)
> goto err_minors;
>
> + ret = create_compat_control_link(dev);
> + if (ret)
> + goto err_minors;
> +
> if (dev->driver->load) {
> ret = dev->driver->load(dev, flags);
> if (ret)
> @@ -701,6 +746,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
> goto out_unlock;
>
> err_minors:
> + remove_compat_control_link(dev);
> drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
> drm_minor_unregister(dev, DRM_MINOR_RENDER);
> drm_minor_unregister(dev, DRM_MINOR_CONTROL);
> @@ -741,6 +787,7 @@ void drm_dev_unregister(struct drm_device *dev)
> list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head)
> drm_legacy_rmmap(dev, r_list->map);
>
> + remove_compat_control_link(dev);
> drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
> drm_minor_unregister(dev, DRM_MINOR_RENDER);
> drm_minor_unregister(dev, DRM_MINOR_CONTROL);
> --
> 2.10.2
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
