[Intel-gfx] [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 at 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 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);
> >> +     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