sysfs registration/teardown fixups

A check in drm_sysfs_connector_remove was supposed to allow it to be called
even with unregistered objects, to make cleanup paths a little simpler.
However, device_is_regsitered didn't always seem to return what we thought it
would, so we'd sometimes end up leaving objects lying around rather than
unregistering them.

Fix this situation up by requiring devices to be registered before being
removed.  Any problems resulting from this change should be easier to track
down than the alternative (which is leaving kobjects registered after unload).

diff --git a/linux-core/drm_sysfs.c b/linux-core/drm_sysfs.c
index c148256..92371c2 100644 (file)
--- a/linux-core/drm_sysfs.c
+++ b/linux-core/drm_sysfs.c
@@ -239,20 +239,26 @@ static struct bin_attribute edid_attr = {
  * properties (so far, connection status, dpms, mode list & edid) and
  * generate a hotplug event so userspace knows there's a new connector
  * available.
+ *
+ * Note:
+ * This routine should only be called *once* for each DRM minor registered.
+ * A second call for an already registered device will trigger the BUG_ON
+ * below.
  */
 int drm_sysfs_connector_add(struct drm_connector *connector)
 {
        struct drm_device *dev = connector->dev;
        int ret = 0, i, j;
 
-       if (device_is_registered(&connector->kdev))
-           return 0;
+       /* 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;
 
-       DRM_DEBUG("adding \"%s\" to sysfs\n", drm_get_connector_name(connector));
+       DRM_DEBUG("adding \"%s\" to sysfs\n",
+                 drm_get_connector_name(connector));
 
        snprintf(connector->kdev.bus_id, BUS_ID_SIZE, "card%d-%s",
                 dev->primary->index, drm_get_connector_name(connector));
@@ -296,15 +302,19 @@ EXPORT_SYMBOL(drm_sysfs_connector_add);
  * Remove @connector and its associated attributes from sysfs.  Note that
  * the device model core will take care of sending the "remove" uevent
  * at this time, so we don't need to do it.
+ *
+ * Note:
+ * This routine should only be called if the connector was previously
+ * successfully registered.  If @connector hasn't been registered yet,
+ * you'll likely see a panic somewhere deep in sysfs code when called.
  */
 void drm_sysfs_connector_remove(struct drm_connector *connector)
 {
        int i;
 
-       if (!device_is_registered(&connector->kdev))
-               return;
+       DRM_DEBUG("removing \"%s\" from sysfs\n",
+                 drm_get_connector_name(connector));
 
-       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);
@@ -342,6 +352,11 @@ static struct device_attribute dri_attrs[] = {
  * Add a DRM device to the DRM's device model class.  We use @dev's PCI device
  * as the parent for the Linux device, and make sure it has a file containing
  * the driver we're using (for userspace compatibility).
+ *
+ * Note:
+ * This routine should only be called *once* for each DRM minor registered.
+ * A second call for an already registered device will trigger the BUG_ON
+ * below.
  */
 int drm_sysfs_device_add(struct drm_minor *minor)
 {
@@ -362,6 +377,11 @@ int drm_sysfs_device_add(struct drm_minor *minor)
        
        snprintf(minor->kdev.bus_id, BUS_ID_SIZE, minor_str, minor->index);
 
+       /* Shouldn't register more than once */
+       BUG_ON(device_is_registered(&minor->kdev));
+
+       DRM_DEBUG("registering DRM device \"%s\"\n", minor->kdev.bus_id);
+
        err = device_register(&minor->kdev);
        if (err) {
                DRM_ERROR("device add failed: %d\n", err);

diff --git a/linux-core/intel_crt.c b/linux-core/intel_crt.c
index e32a955..b9e8ee6 100644 (file)
--- a/linux-core/intel_crt.c
+++ b/linux-core/intel_crt.c
@@ -268,18 +268,20 @@ void intel_crt_init(struct drm_device *dev)
                return;
 
        connector = &intel_output->base;
-       drm_connector_init(dev, &intel_output->base, &intel_crt_connector_funcs, DRM_MODE_CONNECTOR_VGA);
+       drm_connector_init(dev, &intel_output->base,
+                          &intel_crt_connector_funcs, DRM_MODE_CONNECTOR_VGA);
 
-       drm_encoder_init(dev, &intel_output->enc, &intel_crt_enc_funcs, DRM_MODE_ENCODER_DAC);
+       drm_encoder_init(dev, &intel_output->enc, &intel_crt_enc_funcs,
+                        DRM_MODE_ENCODER_DAC);
 
-       drm_mode_connector_attach_encoder(&intel_output->base, &intel_output->enc);
+       drm_mode_connector_attach_encoder(&intel_output->base,
+                                         &intel_output->enc);
 
        /* Set up the DDC bus. */
        intel_output->ddc_bus = intel_i2c_create(dev, GPIOA, "CRTDDC_A");
        if (!intel_output->ddc_bus) {
                dev_printk(KERN_ERR, &dev->pdev->dev, "DDC bus registration "
                           "failed.\n");
-               intel_crt_destroy(connector);
                return;
        }
 
@@ -291,6 +293,4 @@ void intel_crt_init(struct drm_device *dev)
        drm_connector_helper_add(connector, &intel_crt_connector_helper_funcs);
 
        drm_sysfs_connector_add(connector);
-
-
 }

diff --git a/linux-core/intel_lvds.c b/linux-core/intel_lvds.c
index 8d65c16..04e110e 100644 (file)
--- a/linux-core/intel_lvds.c
+++ b/linux-core/intel_lvds.c
@@ -329,7 +329,8 @@ static void intel_lvds_destroy(struct drm_connector *connector)
 {
        struct intel_output *intel_output = to_intel_output(connector);
 
-       intel_i2c_destroy(intel_output->ddc_bus);
+       if (intel_output->ddc_bus)
+               intel_i2c_destroy(intel_output->ddc_bus);
        drm_sysfs_connector_remove(connector);
        drm_connector_cleanup(connector);
        kfree(connector);
@@ -425,8 +426,7 @@ void intel_lvds_init(struct drm_device *dev)
        if (!intel_output->ddc_bus) {
                dev_printk(KERN_ERR, &dev->pdev->dev, "DDC bus registration "
                           "failed.\n");
-               intel_lvds_destroy(connector);
-               return;
+               goto failed;
        }
 
        /*
@@ -470,19 +470,18 @@ void intel_lvds_init(struct drm_device *dev)
        if (!dev_priv->panel_fixed_mode)
                goto failed;
 
-#if 0
        /* FIXME: detect aopen & mac mini type stuff automatically? */
        /*
         * Blacklist machines with BIOSes that list an LVDS panel without
         * actually having one.
         */
-       if (dev_priv->PciInfo->chipType == PCI_CHIP_I945_GM) {
+       if (IS_I945GM(dev)) {
                /* aopen mini pc */
-               if (dev_priv->PciInfo->subsysVendor == 0xa0a0)
-                       goto disable_exit;
+               if (dev->pdev->subsystem_vendor == 0xa0a0)
+                       goto failed;
 
-               if ((dev_priv->PciInfo->subsysVendor == 0x8086) &&
-                   (dev_priv->PciInfo->subsysCard == 0x7270)) {
+               if ((dev->pdev->subsystem_vendor == 0x8086) &&
+                   (dev->pdev->subsystem_device == 0x7270)) {
                        /* It's a Mac Mini or Macbook Pro.
                         *
                         * Apple hardware is out to get us.  The macbook pro
@@ -494,23 +493,23 @@ void intel_lvds_init(struct drm_device *dev)
                         */
 
                        if (dev_priv->panel_fixed_mode != NULL &&
-                           dev_priv->panel_fixed_mode->HDisplay == 800 &&
-                           dev_priv->panel_fixed_mode->VDisplay == 600)
-                       {
-                               xf86DrvMsg(pScrn->scrnIndex, X_INFO,
-                                          "Suspected Mac Mini, ignoring the LVDS\n");
-                               goto disable_exit;
+                           dev_priv->panel_fixed_mode->hdisplay == 800 &&
+                           dev_priv->panel_fixed_mode->vdisplay == 600) {
+                               DRM_DEBUG("Suspected Mac Mini, ignoring the LVDS\n");
+                               goto failed;
                        }
                }
        }
 
-#endif
 
 out:
        drm_sysfs_connector_add(connector);
        return;
 
 failed:
-        DRM_DEBUG("No LVDS modes found, disabling.\n");
-       intel_lvds_destroy(connector);
+       DRM_DEBUG("No LVDS modes found, disabling.\n");
+       if (intel_output->ddc_bus)
+               intel_i2c_destroy(intel_output->ddc_bus);
+       drm_connector_cleanup(connector);
+       kfree(connector);
 }

diff --git a/linux-core/intel_sdvo.c b/linux-core/intel_sdvo.c
index ef67ef9..f0a47e2 100644 (file)
--- a/linux-core/intel_sdvo.c
+++ b/linux-core/intel_sdvo.c
@@ -1036,10 +1036,8 @@ void intel_sdvo_init(struct drm_device *dev, int output_device)
        else
                i2cbus = intel_i2c_create(dev, GPIOE, "SDVOCTRL_E for SDVOC");
 
-       if (i2cbus == NULL) {
-               intel_sdvo_destroy(connector);
-               return;
-       }
+       if (!i2cbus)
+               goto err_connector;
 
        sdvo_priv->i2c_bus = i2cbus;
 
@@ -1061,8 +1059,7 @@ void intel_sdvo_init(struct drm_device *dev, int output_device)
                if (!intel_sdvo_read_byte(intel_output, i, &ch[i])) {
                        DRM_DEBUG("No SDVO device found on SDVO%c\n",
                                  output_device == SDVOB ? 'B' : 'C');
-                       intel_sdvo_destroy(connector);
-                       return;
+                       goto err_i2c;
                }
        }
 
@@ -1107,8 +1104,7 @@ void intel_sdvo_init(struct drm_device *dev, int output_device)
                DRM_DEBUG("%s: No active RGB or TMDS outputs (0x%02x%02x)\n",
                          SDVO_NAME(sdvo_priv),
                          bytes[0], bytes[1]);
-               intel_sdvo_destroy(connector);
-               return;
+               goto err_i2c;
        }
        
        drm_encoder_init(dev, &intel_output->enc, &intel_sdvo_enc_funcs, encoder_type);
@@ -1143,6 +1139,15 @@ void intel_sdvo_init(struct drm_device *dev, int output_device)
                  sdvo_priv->caps.output_flags & 
                        (SDVO_OUTPUT_TMDS1 | SDVO_OUTPUT_RGB1) ? 'Y' : 'N');
 
-       intel_output->ddc_bus = i2cbus; 
+       intel_output->ddc_bus = i2cbus;
+
+       return;
 
+err_i2c:
+       intel_i2c_destroy(intel_output->i2c_bus);
+err_connector:
+       drm_connector_cleanup(connector);
+       kfree(intel_output);
+
+       return;
 }