Re: [PATCH 4/5] drm: add core support for unplugging a device

On Mon, Feb 20, 2012 at 04:13:48PM +0000, Dave Airlie wrote:

From: Dave Airlie airlied@redhat.com

Two parts to this, one is simple unplug from sysfs for the device node.

The second adds an unplugged state, if we have device opens, we just set the unplugged state and return, if we have no device opens we drop the drm device.

If after a lastclose we discover we are unplugged we then drop the drm device.

Signed-off-by: Dave Airlie airlied@redhat.com

I like the unplugged state to handle whole-device removal, but I think this needs a few extensions: - I think we should unconditionally unregister the drm dev minors. Simply checking dev->unplugged should do the trick because we can't just tear down the minor structs (we can only unregister the dev node). - writing to dev->unplugged needs to be protected by the same mutex (i.e. drm_global_mutex) as the actual unregister step. - I think we should add a simply drm_device_is_unplugged so that drivers can check the unplugged state in a race-free manner. Adding an smp_wmb() after writing to dev->unplugged and an smb_rmb() before reading it in that helper (plus ensuring that dev->unplugged is a separate word in drm_device) should do the trick without overhead. - I think we wan't the drm core to return -ENODEV for any ioctls an open calls and return a SIGBUS on the fault handler. Currently you roll that yourself in udl, but I can't think of a use-case where we want to stick the interfaces around after a hotremove of the entire device.

Cheers, Daniel

---

drivers/gpu/drm/drm_fops.c | 2 ++ drivers/gpu/drm/drm_stub.c | 22 ++++++++++++++++++++++ include/drm/drmP.h | 2 ++ 3 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 6263b01..154cfac 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -579,6 +579,8 @@ int drm_release(struct inode *inode, struct file *filp) retcode = -EBUSY; } else retcode = drm_lastclose(dev);

• if (dev->unplugged)
  

• 	drm_put_dev(dev);
  

  } mutex_unlock(&drm_global_mutex);

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 6d7b083..b04c92d 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -429,6 +429,11 @@ int drm_put_minor(struct drm_minor **minor_p) return 0; }

+static void drm_unplug_minor(struct drm_minor *minor) +{

• drm_sysfs_device_remove(minor);

+}

/**

• Called via drm_exit() at module unload time or when pci device is
• unplugged.

@@ -492,3 +497,20 @@ void drm_put_dev(struct drm_device *dev) kfree(dev); } EXPORT_SYMBOL(drm_put_dev);

+void drm_unplug_dev(struct drm_device *dev) +{

• /* for a USB device */
• if (drm_core_check_feature(dev, DRIVER_MODESET))

• drm_unplug_minor(dev->control);
  

• drm_unplug_minor(dev->primary);
• dev->unplugged = true;
• mutex_lock(&drm_global_mutex);
• if (dev->open_count == 0) {

• drm_put_dev(dev);
  

• }
• mutex_unlock(&drm_global_mutex);

+} +EXPORT_SYMBOL(drm_unplug_dev); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 92f0981..8c11797 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1170,6 +1170,7 @@ struct drm_device { struct idr object_name_idr; /*@} */ int switch_power_state;

• bool unplugged; /* device has been unplugged or gone away */

};

#define DRM_SWITCH_POWER_ON 0 @@ -1464,6 +1465,7 @@ extern void drm_master_put(struct drm_master **master);

extern void drm_put_dev(struct drm_device *dev); extern int drm_put_minor(struct drm_minor **minor); +extern void drm_unplug_dev(struct drm_device *dev); extern unsigned int drm_debug;

extern unsigned int drm_vblank_offdelay;

1.7.6

---

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