Re: [PATCH] drm/atomic_helper: Stop modesets on unregistered connectors harder

On Mon, Oct 15, 2018 at 10:33:06PM -0400, Lyude Paul wrote:

Unfortunately, it appears our fix in: commit b5d29843d8ef ("drm/atomic_helper: Allow DPMS On<->Off changes for unregistered connectors")

Which attempted to work around the problems introduced by: commit 4d80273976bf ("drm/atomic_helper: Disallow new modesets on unregistered connectors")

Is still not the right solution, as modesets can still be triggered outside of drm_atomic_set_crtc_for_connector().

So in order to fix this, while still being careful that we don't break modesets that a driver may perform before being registered with userspace, we replace connector->registered with a tristate member, connector->registration_state. This allows us to keep track of whether or not a connector is still initializing and hasn't been exposed to userspace, is currently registered and exposed to userspace, or has been legitimately removed from the system after having once been present.

Using this info, we can prevent userspace from performing new modesets on unregistered connectors while still allowing the driver to perform modesets on unregistered connectors before the driver has finished being registered.

Fixes: b5d29843d8ef ("drm/atomic_helper: Allow DPMS On<->Off changes for unregistered connectors") Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Rodrigo Vivi rodrigo.vivi@intel.com Cc: stable@vger.kernel.org Cc: David Airlie airlied@linux.ie Signed-off-by: Lyude Paul lyude@redhat.com

---

drivers/gpu/drm/drm_atomic_helper.c | 60 +++++++++++++++++++++---- drivers/gpu/drm/drm_atomic_uapi.c | 21 --------- drivers/gpu/drm/drm_connector.c | 10 ++--- drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++-- include/drm/drm_connector.h | 68 ++++++++++++++++++++++++++++- 5 files changed, 127 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 6f66777dca4b..6cadeaf28ae4 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -529,6 +529,35 @@ mode_valid(struct drm_atomic_state *state) return 0; } +static int +unregistered_connector_check(struct drm_atomic_state *state,

• 	     struct drm_connector *connector,
  

• 	     struct drm_connector_state *old_conn_state,
  

• 	     struct drm_connector_state *new_conn_state)
  

+{

• struct drm_crtc_state *crtc_state;
• struct drm_crtc *crtc;
• if (!drm_connector_unregistered(connector))

• return 0;
  

• crtc = new_conn_state->crtc;
• if (!crtc)

• return 0;
  

• crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
• if (!crtc_state || !drm_atomic_crtc_needs_modeset(crtc_state))

• return 0;
  

• if (crtc_state->mode_changed) {

• DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] can't change mode on unregistered connector\n",
  

• 		 connector->base.id, connector->name);
  

• return -EINVAL;
  

• }
• return 0;

+}

/**

• drm_atomic_helper_check_modeset - validate state object for modeset changes
• @dev: DRM device

@@ -684,18 +713,33 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, return ret; }

• /*

• * Iterate over all connectors again, to make sure atomic_check()
  

• * has been called on them when a modeset is forced.
  

• */
  

  for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) { const struct drm_connector_helper_funcs *funcs = connector->helper_private;

• if (connectors_mask & BIT(i))
  

• 	continue;
  

• /* Make sure atomic_check() is called on any unchecked
  

•  * connectors when a modeset has been forced
  

•  */
  

• if (connectors_mask & BIT(i) && funcs->atomic_check) {
  

• 	ret = funcs->atomic_check(connector,
  

• 				  new_connector_state);
  

• 	if (ret)
  

• 		return ret;
  

• }
  

• if (funcs->atomic_check)
  

• 	ret = funcs->atomic_check(connector, new_connector_state);
  

• /*
  

•  * Prevent userspace from turning on new displays or setting
  

•  * new modes using connectors which have been removed from
  

•  * userspace. This is racy since an unplug could happen at any
  

•  * time including after this check, but that's OK: we only
  

•  * care about preventing userspace from trying to set invalid
  

•  * state using destroyed connectors that it's been notified
  

•  * about. No one can save us after the atomic check completes
  

•  * but ourselves.
  

•  */
  

• ret = unregistered_connector_check(state,
  

• 				   connector,
  

• 				   old_connector_state,
  

• 				   new_connector_state);
  

I expect a functional revert of b5d29843d8ef ("drm/atomic_helper: Allow DPMS On<->Off changes for unregistered connectors"), but with drm_connector_unregistered() instead of the READ_ONCE. This here is entirely different, and I'm not clear on why.

Aside from this looks good to me.

[snip]

+/**

• • drm_connector_unregistered - has the connector been unregistered from
• • userspace?
• • @connector: DRM connector
• • Checks whether or not @connector has been unregistered from userspace.
• • Returns:
• • True if the connector was unregistered, false if the connector is
• • registered or has not yet been registered with userspace.
• */

+static inline bool drm_connector_unregistered(struct drm_connector *connector)

Bikeshed: I'd call this drm_connector_is_unregistered(). C is imperative, functions without verbs always look a bit funny to me :-)

Feel free to ignore the bikeshed.

Cheers, Daniel

+{

• return READ_ONCE(connector->registration_state) ==

• DRM_CONNECTOR_UNREGISTERED;
  

+}

const char *drm_get_connector_status_name(enum drm_connector_status status); const char *drm_get_subpixel_order_name(enum subpixel_order order); const char *drm_get_dpms_name(int val); -- 2.17.2