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

[David Herrmann]

Hey

On Fri, Dec 9, 2016 at 2:56 PM, 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 are using this.
> -modesetting do not, and on -intel it's only about the 4th fallback
> path for device lookup, 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 sysfs from controlD* to card*.
>
> v2:
> - Fix error path handling by adding if (!minor) return checks (David)
> - Fix the controlD* numbers to match what's been there (David)
> - Add a comment what exactly userspace minimally needs.
> - Correct the analysis for -intel (Chris).
>
> 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 | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 4ec61ac27477..4a7b3e98d586 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -650,6 +650,62 @@ 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 0;
> +
> +       /*
> +        * Some existing userspace out there uses the existing of the controlD*
> +        * sysfs files to figure out whether it's a modeset driver. It only does
> +        * readdir, hence a symlink is sufficient (and the least confusing
> +        * option). Otherwise controlD* is entirely unused.
> +        *
> +        * Old controlD chardev have been allocated in the range
> +        * 64-127.
> +        */
> +       name = kasprintf(GFP_KERNEL, "controlD%d", minor->index + 64);
> +       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;
> +
> +       name = kasprintf(GFP_KERNEL, "controlD%d", minor->index);
> +       if (!name)
> +               return;

"%d" is at most 11 characters:

    char name[8 + 11 + 1];

    snprintf(name, sizeof(name), "controlD%d", minor->index);
    sysfs_{create,remove}_link(...);

Makes the code in both paths a lot simpler, at the cost of doing
buffer-length calculations. Also, it avoids failure in the cleanup
path (even though kasprintf() failure is impossible for small
buffers). I prefer anything that is not asprintf(). Up to you.

Reviewed-by: David Herrmann <dh.herrmann at gmail.com>

Thanks
David

> +
> +       sysfs_remove_link(minor->kdev->kobj.parent, name);
> +
> +       kfree(name);
> +}
> +
>  /**
>   * drm_dev_register - Register DRM device
>   * @dev: Device to register
> @@ -688,6 +744,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 +761,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 +802,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
>