[Intel-gfx] [PATCH] drm: Add fake controlD* symlinks for backwards compat

[David Herrmann]

Hey

On Fri, Dec 9, 2016 at 11:42 AM, Daniel Vetter <daniel.vetter at ffwll.ch> 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 at gmail.com>
> Reported-by: Alex Deucher <alexander.deucher at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: David Herrmann <dh.herrmann at gmail.com>
> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  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
>