drm: Tighten locking in drm_mode_getconnector

- Modeset state needs mode_config->connection mutex, that covers
  figuring out the encoder, and reading properties (since in the
  atomic case those need to look at connector->state).

- Don't hold any locks for stuff that's invariant (i.e. possible
  connectors).

- Same for connector lookup and unref, those don't need any locks.

- And finally the probe stuff is only protected by mode_config->mutex.

While at it updated the kerneldoc for these fields in drm_connector
and add docs explaining what's protected by which locks.

Reviewed-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20161213230814.19598-10-daniel.vetter@ffwll.ch

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b40401278077565c3cf1a758909835c9e901bd77..3115db2ae6b1d088af57aaf966005141c0c2d24c 100644 (file)
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1163,43 +1163,65 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 
        memset(&u_mode, 0, sizeof(struct drm_mode_modeinfo));
 
-       mutex_lock(&dev->mode_config.mutex);
-
        connector = drm_connector_lookup(dev, out_resp->connector_id);
-       if (!connector) {
-               ret = -ENOENT;
-               goto out_unlock;
-       }
+       if (!connector)
+               return -ENOENT;
+
+       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+       encoder = drm_connector_get_encoder(connector);
+       if (encoder)
+               out_resp->encoder_id = encoder->base.id;
+       else
+               out_resp->encoder_id = 0;
+
+       ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,
+                       (uint32_t __user *)(unsigned long)(out_resp->props_ptr),
+                       (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr),
+                       &out_resp->count_props);
+       drm_modeset_unlock(&dev->mode_config.connection_mutex);
+       if (ret)
+               goto out_unref;
 
        for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
                if (connector->encoder_ids[i] != 0)
                        encoders_count++;
 
+       if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
+               copied = 0;
+               encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr);
+               for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
+                       if (connector->encoder_ids[i] != 0) {
+                               if (put_user(connector->encoder_ids[i],
+                                            encoder_ptr + copied)) {
+                                       ret = -EFAULT;
+                                       goto out_unref;
+                               }
+                               copied++;
+                       }
+               }
+       }
+       out_resp->count_encoders = encoders_count;
+
+       out_resp->connector_id = connector->base.id;
+       out_resp->connector_type = connector->connector_type;
+       out_resp->connector_type_id = connector->connector_type_id;
+
+       mutex_lock(&dev->mode_config.mutex);
        if (out_resp->count_modes == 0) {
                connector->funcs->fill_modes(connector,
                                             dev->mode_config.max_width,
                                             dev->mode_config.max_height);
        }
 
-       /* delayed so we get modes regardless of pre-fill_modes state */
-       list_for_each_entry(mode, &connector->modes, head)
-               if (drm_mode_expose_to_userspace(mode, file_priv))
-                       mode_count++;
-
-       out_resp->connector_id = connector->base.id;
-       out_resp->connector_type = connector->connector_type;
-       out_resp->connector_type_id = connector->connector_type_id;
        out_resp->mm_width = connector->display_info.width_mm;
        out_resp->mm_height = connector->display_info.height_mm;
        out_resp->subpixel = connector->display_info.subpixel_order;
        out_resp->connection = connector->status;
 
-       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-       encoder = drm_connector_get_encoder(connector);
-       if (encoder)
-               out_resp->encoder_id = encoder->base.id;
-       else
-               out_resp->encoder_id = 0;
+       /* delayed so we get modes regardless of pre-fill_modes state */
+       list_for_each_entry(mode, &connector->modes, head)
+               if (drm_mode_expose_to_userspace(mode, file_priv))
+                       mode_count++;
 
        /*
         * This ioctl is called twice, once to determine how much space is
@@ -1222,36 +1244,10 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
                }
        }
        out_resp->count_modes = mode_count;
-
-       ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,
-                       (uint32_t __user *)(unsigned long)(out_resp->props_ptr),
-                       (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr),
-                       &out_resp->count_props);
-       if (ret)
-               goto out;
-
-       if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
-               copied = 0;
-               encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr);
-               for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-                       if (connector->encoder_ids[i] != 0) {
-                               if (put_user(connector->encoder_ids[i],
-                                            encoder_ptr + copied)) {
-                                       ret = -EFAULT;
-                                       goto out;
-                               }
-                               copied++;
-                       }
-               }
-       }
-       out_resp->count_encoders = encoders_count;
-
 out:
-       drm_modeset_unlock(&dev->mode_config.connection_mutex);
-
-       drm_connector_unreference(connector);
-out_unlock:
        mutex_unlock(&dev->mode_config.mutex);
+out_unref:
+       drm_connector_unreference(connector);
 
        return ret;
 }

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index e4f23cc2aaab2ff1a64c77d1a6541e994be9b36f..b018e7684e9bf200bbc2d6f63b6172a50a94ac77 100644 (file)
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -563,9 +563,6 @@ struct drm_cmdline_mode {
  * @interlace_allowed: can this connector handle interlaced modes?
  * @doublescan_allowed: can this connector handle doublescan?
  * @stereo_allowed: can this connector handle stereo modes?
- * @modes: modes available on this connector (from fill_modes() + user)
- * @status: one of the drm_connector_status enums (connected, not, or unknown)
- * @probed_modes: list of modes derived directly from the display
  * @funcs: connector control functions
  * @edid_blob_ptr: DRM property containing EDID if present
  * @properties: property tracking for this connector
@@ -635,11 +632,27 @@ struct drm_connector {
         * Protected by @mutex.
         */
        bool registered;
+
+       /**
+        * @modes:
+        * Modes available on this connector (from fill_modes() + user).
+        * Protected by dev->mode_config.mutex.
+        */
        struct list_head modes; /* list of modes on this connector */
 
+       /**
+        * @status:
+        * One of the drm_connector_status enums (connected, not, or unknown).
+        * Protected by dev->mode_config.mutex.
+        */
        enum drm_connector_status status;
 
-       /* these are modes added by probing with DDC or the BIOS */
+       /**
+        * @probed_modes:
+        * These are modes added by probing with DDC or the BIOS, before
+        * filtering is applied. Used by the probe helpers.Protected by
+        * dev->mode_config.mutex.
+        */
        struct list_head probed_modes;
 
        /**
@@ -648,6 +661,8 @@ struct drm_connector {
         * flat panels in embedded systems, the driver should initialize the
         * display_info.width_mm and display_info.height_mm fields with the
         * physical size of the display.
+        *
+        * Protected by dev->mode_config.mutex.
         */
        struct drm_display_info display_info;
        const struct drm_connector_funcs *funcs;