From: Sean Paul <seanp...@chromium.org>

This patch adds some protection against connectors being destroyed
before the HDCP workers are finished.

For check_work, we do a synchronous cancel after the connector is
unregistered which will ensure that it is finished before destruction.

In the case of prop_work, we can't do a synchronous wait since it needs
to take connection_mutex which could cause deadlock. Instead, we'll take
a reference on the connector when scheduling prop_work and give it up
once we're done.

Reviewed-by: Ramalingam C <ramalinga...@intel.com>
Signed-off-by: Sean Paul <seanp...@chromium.org>
Link: 
https://patchwork.freedesktop.org/patch/msgid/20191212190230.188505-8-s...@poorly.run
 #v2
Link: 
https://patchwork.freedesktop.org/patch/msgid/20200117193103.156821-8-s...@poorly.run
 #v3
Link: 
https://patchwork.freedesktop.org/patch/msgid/20200218220242.107265-8-s...@poorly.run
 #v4
Link: 
https://patchwork.freedesktop.org/patch/msgid/20200305201236.152307-8-s...@poorly.run
 #v5

Changes in v2:
-Added to the set
Changes in v3:
-Change the WARN_ON condition in intel_hdcp_cleanup to allow for
 initializing connectors as well
Changes in v4:
-None
Changes in v5:
-Change WARN_ON to drm_WARN_ON
Changes in v6:
-None
---
 drivers/gpu/drm/i915/display/intel_hdcp.c | 44 ++++++++++++++++++++---
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c 
b/drivers/gpu/drm/i915/display/intel_hdcp.c
index ec02eaa43d39..a4446d47ef27 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
@@ -888,8 +888,10 @@ static void intel_hdcp_update_value(struct intel_connector 
*connector,
                return;
 
        hdcp->value = value;
-       if (update_property)
+       if (update_property) {
+               drm_connector_get(&connector->base);
                schedule_work(&hdcp->prop_work);
+       }
 }
 
 /* Implements Part 3 of the HDCP authorization procedure */
@@ -981,6 +983,8 @@ static void intel_hdcp_prop_work(struct work_struct *work)
 
        mutex_unlock(&hdcp->mutex);
        drm_modeset_unlock(&dev_priv->drm.mode_config.connection_mutex);
+
+       drm_connector_put(&connector->base);
 }
 
 bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
@@ -1860,6 +1864,9 @@ static void intel_hdcp_check_work(struct work_struct 
*work)
                                               check_work);
        struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
 
+       if (drm_connector_is_unregistered(&connector->base))
+               return;
+
        if (!intel_hdcp2_check_link(connector))
                schedule_delayed_work(&hdcp->check_work,
                                      DRM_HDCP2_CHECK_PERIOD_MS);
@@ -2175,12 +2182,39 @@ void intel_hdcp_component_fini(struct drm_i915_private 
*dev_priv)
 
 void intel_hdcp_cleanup(struct intel_connector *connector)
 {
-       if (!connector->hdcp.shim)
+       struct intel_hdcp *hdcp = &connector->hdcp;
+
+       if (!hdcp->shim)
                return;
 
-       mutex_lock(&connector->hdcp.mutex);
-       kfree(connector->hdcp.port_data.streams);
-       mutex_unlock(&connector->hdcp.mutex);
+       /*
+        * If the connector is registered, it's possible userspace could kick
+        * off another HDCP enable, which would re-spawn the workers.
+        */
+       drm_WARN_ON(connector->base.dev,
+               connector->base.registration_state == DRM_CONNECTOR_REGISTERED);
+
+       /*
+        * Now that the connector is not registered, check_work won't be run,
+        * but cancel any outstanding instances of it
+        */
+       cancel_delayed_work_sync(&hdcp->check_work);
+
+       /*
+        * We don't cancel prop_work in the same way as check_work since it
+        * requires connection_mutex which could be held while calling this
+        * function. Instead, we rely on the connector references grabbed before
+        * scheduling prop_work to ensure the connector is alive when prop_work
+        * is run. So if we're in the destroy path (which is where this
+        * function should be called), we're "guaranteed" that prop_work is not
+        * active (tl;dr This Should Never Happen).
+        */
+       drm_WARN_ON(connector->base.dev, work_pending(&hdcp->prop_work));
+
+       mutex_lock(&hdcp->mutex);
+       kfree(hdcp->port_data.streams);
+       hdcp->shim = NULL;
+       mutex_unlock(&hdcp->mutex);
 }
 
 void intel_hdcp_atomic_check(struct drm_connector *connector,
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

Reply via email to