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

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

[Daniel Vetter]

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@gmail.com>
Reported-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Vetter <daniel.vetter@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);
+	name = kasprintf(GFP_KERNEL, "controlD%d", minor->index);
+	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);
+	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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Add fake controlD* symlinks for backwards compat

[Greg Kroah-Hartman]

On Fri, Dec 09, 2016 at 11:42:01AM +0100, Daniel Vetter 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*.

In debugfs?  This patch seems to be for sysfs.

> Fixes: 8a357d10043c ("drm: Nerf DRM_CONTROL nodes")
> Cc: Dave Airlie <airlied@gmail.com>
> Reported-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@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);
> +	name = kasprintf(GFP_KERNEL, "controlD%d", minor->index);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	ret = sysfs_create_link(minor->kdev->kobj.parent,
> +				&minor->kdev->kobj,
> +				name);

Ick.  Dropping down to a sysfs call and a kobject isn't nice in driver
code, but I guess the driver core doesn't provide symlinks.  We do
provide the "class_compat" functionality, but that doesn't really apply
here.

So, what happened, you just added a "middle layer" device object for
these control files?

If you need these compatibility symlinks, why even do the original code
at all?  That kind of implies it shouldn't have been made one layer
deeper if it was going to break userspace somehow.

If you add these, what's the odds that they ever can be removed
(probably never, right?)

thanks,

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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@gmail.com>
> Reported-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Add fake controlD* symlinks for backwards compat

[David Herrmann]

Hi Greg

On Fri, Dec 9, 2016 at 11:53 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Dec 09, 2016 at 11:42:01AM +0100, Daniel Vetter 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*.
>
> In debugfs?  This patch seems to be for sysfs.

Yes, typo. It is meant to be sysfs.

>> Fixes: 8a357d10043c ("drm: Nerf DRM_CONTROL nodes")
>> Cc: Dave Airlie <airlied@gmail.com>
>> Reported-by: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Daniel Vetter <daniel.vetter@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);
>> +     name = kasprintf(GFP_KERNEL, "controlD%d", minor->index);
>> +     if (!name)
>> +             return -ENOMEM;
>> +
>> +     ret = sysfs_create_link(minor->kdev->kobj.parent,
>> +                             &minor->kdev->kobj,
>> +                             name);
>
> Ick.  Dropping down to a sysfs call and a kobject isn't nice in driver
> code, but I guess the driver core doesn't provide symlinks.  We do
> provide the "class_compat" functionality, but that doesn't really apply
> here.
>
> So, what happened, you just added a "middle layer" device object for
> these control files?
>
> If you need these compatibility symlinks, why even do the original code
> at all?  That kind of implies it shouldn't have been made one layer
> deeper if it was going to break userspace somehow.
>
> If you add these, what's the odds that they ever can be removed
> (probably never, right?)

Yes. The alternative would be to create a dummy "struct device" and
register it, but do not include any information on it. I don't think
we need the symlink-behavior. Daniel? But the symlink would at least
be kinda useful. The dummy device, on the other hand, would just make
sure the readdir() calls of legacy stuff sees the control nodes (even
though they never use it).

Thanks
David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Fri, Dec 09, 2016 at 11:42:01AM +0100, Daniel Vetter 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.

Note that for -intel, it is in the common code. It's about the 4th
fallback, if the Xserver doesn't pass along the right fd, path or pci and
we are expected to probe from scratch.

> 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*.

s/debugfs/sysfs/
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for drm: Add fake controlD* symlinks for backwards compat

[Patchwork]

== Series Details ==

Series: drm: Add fake controlD* symlinks for backwards compat
URL   : https://patchwork.freedesktop.org/series/16605/
State : success

== Summary ==

Series 16605v1 drm: Add fake controlD* symlinks for backwards compat
https://patchwork.freedesktop.org/api/1.0/series/16605/revisions/1/mbox/


fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-t5700     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

de43f4e755c6bf50ad53b4ccacedf9850f42eda4 drm-tip: 2016y-12m-09d-09h-01m-58s UTC integration manifest
f0f2c4b drm: Add fake controlD* symlinks for backwards compat

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3249/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Add fake controlD* symlinks for backwards compat

[Daniel Vetter]

On Fri, Dec 09, 2016 at 11:57:34AM +0100, David Herrmann wrote:
> Hi Greg
> 
> On Fri, Dec 9, 2016 at 11:53 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Dec 09, 2016 at 11:42:01AM +0100, Daniel Vetter 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*.
> >
> > In debugfs?  This patch seems to be for sysfs.
> 
> Yes, typo. It is meant to be sysfs.
> 
> >> Fixes: 8a357d10043c ("drm: Nerf DRM_CONTROL nodes")
> >> Cc: Dave Airlie <airlied@gmail.com>
> >> Reported-by: Alex Deucher <alexander.deucher@amd.com>
> >> Cc: Alex Deucher <alexander.deucher@amd.com>
> >> Cc: David Herrmann <dh.herrmann@gmail.com>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@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);
> >> +     name = kasprintf(GFP_KERNEL, "controlD%d", minor->index);
> >> +     if (!name)
> >> +             return -ENOMEM;
> >> +
> >> +     ret = sysfs_create_link(minor->kdev->kobj.parent,
> >> +                             &minor->kdev->kobj,
> >> +                             name);
> >
> > Ick.  Dropping down to a sysfs call and a kobject isn't nice in driver
> > code, but I guess the driver core doesn't provide symlinks.  We do
> > provide the "class_compat" functionality, but that doesn't really apply
> > here.
> >
> > So, what happened, you just added a "middle layer" device object for
> > these control files?
> >
> > If you need these compatibility symlinks, why even do the original code
> > at all?  That kind of implies it shouldn't have been made one layer
> > deeper if it was going to break userspace somehow.
> >
> > If you add these, what's the odds that they ever can be removed
> > (probably never, right?)
> 
> Yes. The alternative would be to create a dummy "struct device" and
> register it, but do not include any information on it. I don't think
> we need the symlink-behavior. Daniel? But the symlink would at least
> be kinda useful. The dummy device, on the other hand, would just make
> sure the readdir() calls of legacy stuff sees the control nodes (even
> though they never use it).

So more context: It doesn't matter what it is, as long as readdir can read
it. We've never had userspace that used these special controlD* charactar
devices (which exposed a slightly different flavour of kms ioctls compared
to the primary chardev), but by accident the existence of that node is
used to figure out whether the driver supports modesetting or not. At
least by some drivers - other drivers just ask for modeset resources
through the corresponding ioctl and if that fails or returns nothing it's
obviously not a modeset but a render only driver.

Here's the simplified libdrm code:

int drmCheckModesettingSupported(const char *busid)
{
	char pci_dev_dir[1024];
	int domain, bus, dev, func;
	DIR *sysdir;
	struct dirent *dent;
	int found = 0, ret;

	ret = sscanf(busid, "pci:%04x:%02x:%02x.%d", &domain, &bus, &dev, &func);
	if (ret != 4)
		return -EINVAL;

	sprintf(pci_dev_dir, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/drm",
		domain, bus, dev, func);

	sysdir = opendir(pci_dev_dir);
	if (sysdir) {
		dent = readdir(sysdir);
		while (dent) {
			if (!strncmp(dent->d_name, "controlD", 8))
				return 0;

			dent = readdir(sysdir);
		}
		closedir(sysdir);
	}

	return -ENOSYS;
}

Adding a symlink in the glue directory to the primary node with the name
of the controlD node that we've killed seemed like the least horrible fix.
Alternative like David said would be a full-blown device in the drm_class,
which would be a rather superb and confusing lie ;-)

But in the end it doesn't matter what kind of file it is, since the only
code we've found thus far using these controlD* chardevs is the above
thing, and that doesn't even do a stat(). So it can be whatever you want
it to.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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@gmail.com>
Reported-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Vetter <daniel.vetter@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;
+
+	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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Add fake controlD* symlinks for backwards compat

[Emil Velikov]

On 9 December 2016 at 13:56, Daniel Vetter <daniel.vetter@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.
Sincere apologies for missing that one. Must have been looking at one
of the branches where I've neutered it.

Fwiw, the patch is
Acked-by: Emil Velikov <emil.l.velikov@gmail.com>

Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@gmail.com>
> Reported-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@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@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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Add fake controlD* symlinks for backwards compat

[Greg Kroah-Hartman]

On Fri, Dec 09, 2016 at 02:56:56PM +0100, Daniel Vetter 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@gmail.com>
> Reported-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>


Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm: Add fake controlD* symlinks for backwards compat (rev2)

[Patchwork]

== Series Details ==

Series: drm: Add fake controlD* symlinks for backwards compat (rev2)
URL   : https://patchwork.freedesktop.org/series/16605/
State : success

== Summary ==

Series 16605v2 drm: Add fake controlD* symlinks for backwards compat
https://patchwork.freedesktop.org/api/1.0/series/16605/revisions/2/mbox/


fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-t5700     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

de43f4e755c6bf50ad53b4ccacedf9850f42eda4 drm-tip: 2016y-12m-09d-09h-01m-58s UTC integration manifest
40c6206 drm: Add fake controlD* symlinks for backwards compat

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3252/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx