Message-ID: <CAPM=9tw1L0_6fSX9AUU=V1VmWCOT4BknsYbG+rdnf-Rt7mr5ZQ@mail.gmail.com>
Date:	Fri, 11 Oct 2013 14:28:53 +1000
From:	Dave Airlie <airlied@...il.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Dave Airlie <airlied@...ux.ie>,
	Fengguang Wu <fengguang.wu@...el.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	xen-devel@...ts.xenproject.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [xen] double fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC

> Damn gmail screwed up my reply-all,
>
> anyhoo I get the feeling I'd rather do this like fbdev does and stop using
> an embedded kdev for this if I can. The lifetime of the minor and the sysfs
> objects aren't necessarily that tied together esp with hot unplug of
> USB devices.
>
> e.g. when a USB device goes away I don't want the sysfs files to
> remain hanging around,
> or the userspace device nodes, however I can't remove all the internal
> driver state until userspace finally closes all open device
> files,
>
> I think this is a bit of a bad misdesign of the whole device api that
> sysfs exposure and device exposure is tied
> to objects you are encourgaged to embed.
>
> I think maybe I'll just copy fbdev and do a separate device_create
> with a nonembedded dev.
>

Okay it compiles but not boot tested yet.,attached as well just in
case gmail eats it.

>From be06235b2d2a26c0e52c099e70922b71f1245b9e Mon Sep 17 00:00:00 2001
From: Dave Airlie <airlied@...hat.com>
Date: Fri, 11 Oct 2013 14:07:25 +1000
Subject: [PATCH] drm/sysfs: sort out minor and connector device object
 lifetimes.

So drm was abusing device lifetimes, by having embedded device structures
in the minor and connector it meant that the lifetime of the internal drm
objects (drm_minor and drm_connector) were tied to the lifetime of the device
files in sysfs, so if something kept those files opened the current code
would kfree the objects and things would go downhill from there.

Now in reality there is no need for these lifetimes to be so intertwined,
especailly with hotplugging of devices where we wish to remove the sysfs
and userspace facing pieces before we can unwind the internal objects due
to open userspace files or mmaps, so split the objects out so the struct
device is no longer embedded and do what fbdev does and just allocate
and remove the sysfs inodes separately.

Signed-off-by: Dave Airlie <airlied@...hat.com>
---
 drivers/gpu/drm/drm_sysfs.c       | 94 ++++++++++++++-------------------------
 drivers/gpu/drm/i915/i915_irq.c   |  8 ++--
 drivers/gpu/drm/i915/i915_sysfs.c | 28 ++++++------
 include/drm/drmP.h                |  2 +-
 include/drm/drm_crtc.h            |  2 +-
 5 files changed, 54 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 2290b3b..dae42c7 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -22,8 +22,8 @@
 #include <drm/drm_core.h>
 #include <drm/drmP.h>

-#define to_drm_minor(d) container_of(d, struct drm_minor, kdev)
-#define to_drm_connector(d) container_of(d, struct drm_connector, kdev)
+#define to_drm_minor(d) dev_get_drvdata(d)
+#define to_drm_connector(d) dev_get_drvdata(d)

 static struct device_type drm_sysfs_device_minor = {
     .name = "drm_minor"
@@ -162,20 +162,6 @@ void drm_sysfs_destroy(void)
     drm_class = NULL;
 }

-/**
- * drm_sysfs_device_release - do nothing
- * @dev: Linux device
- *
- * Normally, this would free the DRM device associated with @dev, along
- * with cleaning up any other stuff.  But we do that in the DRM core, so
- * this function can just return and hope that the core does its job.
- */
-static void drm_sysfs_device_release(struct device *dev)
-{
-    memset(dev, 0, sizeof(struct device));
-    return;
-}
-
 /*
  * Connector properties
  */
@@ -394,29 +380,26 @@ int drm_sysfs_connector_add(struct drm_connector
*connector)
     int i;
     int ret;

-    /* We shouldn't get called more than once for the same connector */
-    BUG_ON(device_is_registered(&connector->kdev));
-
-    connector->kdev.parent = &dev->primary->kdev;
-    connector->kdev.class = drm_class;
-    connector->kdev.release = drm_sysfs_device_release;
+    if (connector->kdev)
+        return 0;

+    /* We shouldn't get called more than once for the same connector */
+    connector->kdev = device_create(drm_class, dev->primary->kdev,
+                    0, connector, "card%d-%s",
+                    dev->primary->index, drm_get_connector_name(connector));
     DRM_DEBUG("adding \"%s\" to sysfs\n",
           drm_get_connector_name(connector));

-    dev_set_name(&connector->kdev, "card%d-%s",
-             dev->primary->index, drm_get_connector_name(connector));
-    ret = device_register(&connector->kdev);
-
-    if (ret) {
-        DRM_ERROR("failed to register connector device: %d\n", ret);
+    if (IS_ERR(connector->kdev)) {
+        DRM_ERROR("failed to register connector device: %ld\n",
PTR_ERR(connector->kdev));
+        ret = PTR_ERR(connector->kdev);
         goto out;
     }

     /* Standard attributes */

     for (attr_cnt = 0; attr_cnt < ARRAY_SIZE(connector_attrs); attr_cnt++) {
-        ret = device_create_file(&connector->kdev, &connector_attrs[attr_cnt]);
+        ret = device_create_file(connector->kdev, &connector_attrs[attr_cnt]);
         if (ret)
             goto err_out_files;
     }
@@ -433,7 +416,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
         case DRM_MODE_CONNECTOR_Component:
         case DRM_MODE_CONNECTOR_TV:
             for (opt_cnt = 0; opt_cnt <
ARRAY_SIZE(connector_attrs_opt1); opt_cnt++) {
-                ret = device_create_file(&connector->kdev,
&connector_attrs_opt1[opt_cnt]);
+                ret = device_create_file(connector->kdev,
&connector_attrs_opt1[opt_cnt]);
                 if (ret)
                     goto err_out_files;
             }
@@ -442,7 +425,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
             break;
     }

-    ret = sysfs_create_bin_file(&connector->kdev.kobj, &edid_attr);
+    ret = sysfs_create_bin_file(&connector->kdev->kobj, &edid_attr);
     if (ret)
         goto err_out_files;

@@ -453,10 +436,11 @@ int drm_sysfs_connector_add(struct drm_connector
*connector)

 err_out_files:
     for (i = 0; i < opt_cnt; i++)
-        device_remove_file(&connector->kdev, &connector_attrs_opt1[i]);
+        device_remove_file(connector->kdev, &connector_attrs_opt1[i]);
     for (i = 0; i < attr_cnt; i++)
-        device_remove_file(&connector->kdev, &connector_attrs[i]);
-    device_unregister(&connector->kdev);
+        device_remove_file(connector->kdev, &connector_attrs[i]);
+    put_device(connector->kdev);
+    device_unregister(connector->kdev);

 out:
     return ret;
@@ -480,16 +464,17 @@ void drm_sysfs_connector_remove(struct
drm_connector *connector)
 {
     int i;

-    if (!connector->kdev.parent)
+    if (!connector->kdev)
         return;
     DRM_DEBUG("removing \"%s\" from sysfs\n",
           drm_get_connector_name(connector));

     for (i = 0; i < ARRAY_SIZE(connector_attrs); i++)
-        device_remove_file(&connector->kdev, &connector_attrs[i]);
-    sysfs_remove_bin_file(&connector->kdev.kobj, &edid_attr);
-    device_unregister(&connector->kdev);
-    connector->kdev.parent = NULL;
+        device_remove_file(connector->kdev, &connector_attrs[i]);
+    sysfs_remove_bin_file(&connector->kdev->kobj, &edid_attr);
+    put_device(connector->kdev);
+    device_unregister(connector->kdev);
+    connector->kdev = NULL;
 }
 EXPORT_SYMBOL(drm_sysfs_connector_remove);

@@ -508,7 +493,7 @@ void drm_sysfs_hotplug_event(struct drm_device *dev)

     DRM_DEBUG("generating hotplug event\n");

-    kobject_uevent_env(&dev->primary->kdev.kobj, KOBJ_CHANGE, envp);
+    kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, envp);
 }
 EXPORT_SYMBOL(drm_sysfs_hotplug_event);

@@ -523,15 +508,8 @@ EXPORT_SYMBOL(drm_sysfs_hotplug_event);
  */
 int drm_sysfs_device_add(struct drm_minor *minor)
 {
-    int err;
     char *minor_str;

-    minor->kdev.parent = minor->dev->dev;
-
-    minor->kdev.class = drm_class;
-    minor->kdev.release = drm_sysfs_device_release;
-    minor->kdev.devt = minor->device;
-    minor->kdev.type = &drm_sysfs_device_minor;
     if (minor->type == DRM_MINOR_CONTROL)
         minor_str = "controlD%d";
         else if (minor->type == DRM_MINOR_RENDER)
@@ -539,18 +517,14 @@ int drm_sysfs_device_add(struct drm_minor *minor)
         else
                 minor_str = "card%d";

-    dev_set_name(&minor->kdev, minor_str, minor->index);
-
-    err = device_register(&minor->kdev);
-    if (err) {
-        DRM_ERROR("device add failed: %d\n", err);
-        goto err_out;
+    minor->kdev = device_create(drm_class, minor->dev->dev,
+                    MKDEV(DRM_MAJOR, minor->index),
+                    minor, minor_str, minor->index);
+    if (IS_ERR(minor->kdev)) {
+        DRM_ERROR("device create failed %ld\n", PTR_ERR(minor->kdev));
+        return PTR_ERR(minor->kdev);
     }
-
     return 0;
-
-err_out:
-    return err;
 }

 /**
@@ -562,9 +536,9 @@ err_out:
  */
 void drm_sysfs_device_remove(struct drm_minor *minor)
 {
-    if (minor->kdev.parent)
-        device_unregister(&minor->kdev);
-    minor->kdev.parent = NULL;
+    if (minor->kdev)
+        device_destroy(drm_class, MKDEV(DRM_MAJOR, minor->index));
+    minor->kdev = NULL;
 }


diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b356dc1..8d133bf 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -933,7 +933,7 @@ static void ivybridge_parity_work(struct work_struct *work)
         parity_event[4] = kasprintf(GFP_KERNEL, "SLICE=%d", slice);
         parity_event[5] = NULL;

-        kobject_uevent_env(&dev_priv->dev->primary->kdev.kobj,
+        kobject_uevent_env(&dev_priv->dev->primary->kdev->kobj,
                    KOBJ_CHANGE, parity_event);

         DRM_DEBUG("Parity error: Slice = %d, Row = %d, Bank = %d, Sub
bank = %d.\n",
@@ -1530,7 +1530,7 @@ static void i915_error_work_func(struct work_struct *work)
     char *reset_done_event[] = { I915_ERROR_UEVENT "=0", NULL };
     int ret;

-    kobject_uevent_env(&dev->primary->kdev.kobj, KOBJ_CHANGE, error_event);
+    kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, error_event);

     /*
      * Note that there's only one work item which does gpu resets, so we
@@ -1544,7 +1544,7 @@ static void i915_error_work_func(struct work_struct *work)
      */
     if (i915_reset_in_progress(error) && !i915_terminally_wedged(error)) {
         DRM_DEBUG_DRIVER("resetting chip\n");
-        kobject_uevent_env(&dev->primary->kdev.kobj, KOBJ_CHANGE,
+        kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE,
                    reset_event);

         /*
@@ -1571,7 +1571,7 @@ static void i915_error_work_func(struct work_struct *work)
             smp_mb__before_atomic_inc();
             atomic_inc(&dev_priv->gpu_error.reset_counter);

-            kobject_uevent_env(&dev->primary->kdev.kobj,
+            kobject_uevent_env(&dev->primary->kdev->kobj,
                        KOBJ_CHANGE, reset_done_event);
         } else {
             atomic_set(&error->reset_counter, I915_WEDGED);
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
b/drivers/gpu/drm/i915/i915_sysfs.c
index 44f4c1a..868aed8 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -48,14 +48,14 @@ static u32 calc_residency(struct drm_device *dev,
const u32 reg)
 static ssize_t
 show_rc6_mask(struct device *kdev, struct device_attribute *attr, char *buf)
 {
-    struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev);
+    struct drm_minor *dminor = dev_get_drvdata(kdev);
     return snprintf(buf, PAGE_SIZE, "%x\n", intel_enable_rc6(dminor->dev));
 }

 static ssize_t
 show_rc6_ms(struct device *kdev, struct device_attribute *attr, char *buf)
 {
-    struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev);
+    struct drm_minor *dminor = dev_get_drvdata(kdev);
     u32 rc6_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6);
     return snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
 }
@@ -63,7 +63,7 @@ show_rc6_ms(struct device *kdev, struct
device_attribute *attr, char *buf)
 static ssize_t
 show_rc6p_ms(struct device *kdev, struct device_attribute *attr, char *buf)
 {
-    struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev);
+    struct drm_minor *dminor = dev_get_drvdata(kdev);
     u32 rc6p_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6p);
     if (IS_VALLEYVIEW(dminor->dev))
         rc6p_residency = 0;
@@ -73,7 +73,7 @@ show_rc6p_ms(struct device *kdev, struct
device_attribute *attr, char *buf)
 static ssize_t
 show_rc6pp_ms(struct device *kdev, struct device_attribute *attr, char *buf)
 {
-    struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev);
+    struct drm_minor *dminor = dev_get_drvdata(kdev);
     u32 rc6pp_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6pp);
     if (IS_VALLEYVIEW(dminor->dev))
         rc6pp_residency = 0;
@@ -119,7 +119,7 @@ i915_l3_read(struct file *filp, struct kobject *kobj,
          loff_t offset, size_t count)
 {
     struct device *dev = container_of(kobj, struct device, kobj);
-    struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+    struct drm_minor *dminor = dev_get_drvdata(dev);
     struct drm_device *drm_dev = dminor->dev;
     struct drm_i915_private *dev_priv = drm_dev->dev_private;
     int slice = (int)(uintptr_t)attr->private;
@@ -155,7 +155,7 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
           loff_t offset, size_t count)
 {
     struct device *dev = container_of(kobj, struct device, kobj);
-    struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+    struct drm_minor *dminor = dev_get_drvdata(dev);
     struct drm_device *drm_dev = dminor->dev;
     struct drm_i915_private *dev_priv = drm_dev->dev_private;
     struct i915_hw_context *ctx;
@@ -523,7 +523,7 @@ void i915_setup_sysfs(struct drm_device *dev)

 #ifdef CONFIG_PM
     if (INTEL_INFO(dev)->gen >= 6) {
-        ret = sysfs_merge_group(&dev->primary->kdev.kobj,
+        ret = sysfs_merge_group(&dev->primary->kdev->kobj,
                     &rc6_attr_group);
         if (ret)
             DRM_ERROR("RC6 residency sysfs setup failed\n");
@@ -544,13 +544,13 @@ void i915_setup_sysfs(struct drm_device *dev)

     ret = 0;
     if (IS_VALLEYVIEW(dev))
-        ret = sysfs_create_files(&dev->primary->kdev.kobj, vlv_attrs);
+        ret = sysfs_create_files(&dev->primary->kdev->kobj, vlv_attrs);
     else if (INTEL_INFO(dev)->gen >= 6)
-        ret = sysfs_create_files(&dev->primary->kdev.kobj, gen6_attrs);
+        ret = sysfs_create_files(&dev->primary->kdev->kobj, gen6_attrs);
     if (ret)
         DRM_ERROR("RPS sysfs setup failed\n");

-    ret = sysfs_create_bin_file(&dev->primary->kdev.kobj,
+    ret = sysfs_create_bin_file(&dev->primary->kdev->kobj,
                     &error_state_attr);
     if (ret)
         DRM_ERROR("error_state sysfs setup failed\n");
@@ -558,14 +558,14 @@ void i915_setup_sysfs(struct drm_device *dev)

 void i915_teardown_sysfs(struct drm_device *dev)
 {
-    sysfs_remove_bin_file(&dev->primary->kdev.kobj, &error_state_attr);
+    sysfs_remove_bin_file(&dev->primary->kdev->kobj, &error_state_attr);
     if (IS_VALLEYVIEW(dev))
-        sysfs_remove_files(&dev->primary->kdev.kobj, vlv_attrs);
+        sysfs_remove_files(&dev->primary->kdev->kobj, vlv_attrs);
     else
-        sysfs_remove_files(&dev->primary->kdev.kobj, gen6_attrs);
+        sysfs_remove_files(&dev->primary->kdev->kobj, gen6_attrs);
     device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs_1);
     device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
 #ifdef CONFIG_PM
-    sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
+    sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6_attr_group);
 #endif
 }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 9ba6a38..33eaba6 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1043,7 +1043,7 @@ struct drm_minor {
     int index;            /**< Minor device number */
     int type;                       /**< Control or render */
     dev_t device;            /**< Device number for mknod */
-    struct device kdev;        /**< Linux device */
+    struct device *kdev;        /**< Linux device */
     struct drm_device *dev;

     struct dentry *debugfs_root;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 92e7820..c7ce4e2 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -587,7 +587,7 @@ enum drm_connector_force {
  */
 struct drm_connector {
     struct drm_device *dev;
-    struct device kdev;
+    struct device *kdev;
     struct device_attribute *attr;
     struct list_head head;

-- 
1.8.3.1

Download attachment "0001-drm-sysfs-sort-out-minor-and-connector-device-object.patch" of type "application/octet-stream" (14634 bytes)

Powered by blists - more mailing lists