Hey
On Fri, Dec 9, 2016 at 2:56 PM, 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 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@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 | 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@xxxxxxxxx>
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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
