From patchwork Wed Nov 19 17:38:08 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 5339871 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 1B02C9F2ED for ; Wed, 19 Nov 2014 17:38:03 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id E0B60201EF for ; Wed, 19 Nov 2014 17:38:01 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id D071420172 for ; Wed, 19 Nov 2014 17:38:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 62F616EA9C; Wed, 19 Nov 2014 09:37:58 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wg0-f48.google.com (mail-wg0-f48.google.com [74.125.82.48]) by gabe.freedesktop.org (Postfix) with ESMTP id 1AD206EA9E for ; Wed, 19 Nov 2014 09:37:57 -0800 (PST) Received: by mail-wg0-f48.google.com with SMTP id y19so1409576wgg.21 for ; Wed, 19 Nov 2014 09:37:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=b5pHPoCXeOzlLBwjZCrKuDbkZCVWoFgWqA9RiPQa4Dw=; b=QX5AFt3G6qQLopsfo3sqCx0VCeEd8w+xP8y4IJuq+v6NdrSCi2lQNTa88Cbj+B9LVy mstJiM+ujLSo4zhI7eFPzjs7mgQIQ08kEnjN9qPql34dcshQZfPYSD7i46w7upLHp7AS oJmNKU82CFm8KXHJjd26qUrISsvnqRmgP1Y+M= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=b5pHPoCXeOzlLBwjZCrKuDbkZCVWoFgWqA9RiPQa4Dw=; b=VUnj0irhyLKA/kD9ivZ5nnaRaZqm2yxEj5h5DzaLJlLCAbAAPd3E86L+lP6W+uew5l 0aJDU9Yp0LJmFMXQXDVUgymTt8ouSoh9Q+i1TA9vs8XfVClZHgOC4UGNw1othJU8SNQ0 ZGftZh94EPGbnDyZIM2iubQu7qDh926sUroijltRWLlvjiajTF9megfeqxH1y1mDi04n nsO195FNoS35c1CNkil7bfW+scIlOylftMRgzljr2zwvhSQ5oJFJBstYuzFe86/CcJNk pN7JJPacddkoO2vA+P3v/pg1zBEvOnJMm2osclfo57cZH60+nafZk1u7QnVUmOYQU2wS stxQ== X-Gm-Message-State: ALoCoQk7wxvpN7KFar/SwnMEDthqVUhJIkNRbjdFwwIWTgA+xBSishlFSqR0NN4PE+2KiWQZypYF X-Received: by 10.180.108.167 with SMTP id hl7mr7664926wib.68.1416418676381; Wed, 19 Nov 2014 09:37:56 -0800 (PST) Received: from phenom.ffwll.local (84-73-67-144.dclient.hispeed.ch. [84.73.67.144]) by mx.google.com with ESMTPSA id h2sm567617wix.5.2014.11.19.09.37.54 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 19 Nov 2014 09:37:55 -0800 (PST) From: Daniel Vetter To: DRI Development Subject: [PATCH 3/6] drm/atomic: Don't overrun the connector array when hotplugging Date: Wed, 19 Nov 2014 18:38:08 +0100 Message-Id: <1416418691-24747-3-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.1.1 In-Reply-To: <1416418691-24747-1-git-send-email-daniel.vetter@ffwll.ch> References: <1416418691-24747-1-git-send-email-daniel.vetter@ffwll.ch> Cc: Daniel Vetter , Intel Graphics Development , Daniel Vetter X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Yet another fallout from not considering DP MST hotplug. With the previous patches we have stable indices, but it might still happen that a connector gets added between when we allocate the array and when we actually add a connector. Especially when we back off due to ww mutex contention or similar issues. So store the sizes of the arrays in struct drm_atomic_state and double check them. We don't really care about races except that we want to use a consistent value, so ACCESS_ONCE is all we need. And if we indeed notice that we'd overrun the array then just give up and restart the entire ioctl. Signed-off-by: Daniel Vetter Reviewed-by: Rob Clark --- drivers/gpu/drm/drm_atomic.c | 26 +++++++++++++++++++++----- drivers/gpu/drm/drm_atomic_helper.c | 23 ++++++++--------------- include/drm/drm_crtc.h | 2 ++ 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 67c1dc894bd9..3624632084e2 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -56,6 +56,8 @@ drm_atomic_state_alloc(struct drm_device *dev) if (!state) return NULL; + state->num_connector = ACCESS_ONCE(dev->mode_config.num_connector); + state->crtcs = kcalloc(dev->mode_config.num_crtc, sizeof(*state->crtcs), GFP_KERNEL); if (!state->crtcs) @@ -72,12 +74,12 @@ drm_atomic_state_alloc(struct drm_device *dev) sizeof(*state->plane_states), GFP_KERNEL); if (!state->plane_states) goto fail; - state->connectors = kcalloc(dev->mode_config.num_connector, + state->connectors = kcalloc(state->num_connector, sizeof(*state->connectors), GFP_KERNEL); if (!state->connectors) goto fail; - state->connector_states = kcalloc(dev->mode_config.num_connector, + state->connector_states = kcalloc(state->num_connector, sizeof(*state->connector_states), GFP_KERNEL); if (!state->connector_states) @@ -117,7 +119,7 @@ void drm_atomic_state_clear(struct drm_atomic_state *state) DRM_DEBUG_KMS("Clearing atomic state %p\n", state); - for (i = 0; i < config->num_connector; i++) { + for (i = 0; i < state->num_connector; i++) { struct drm_connector *connector = state->connectors[i]; if (!connector) @@ -304,6 +306,21 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state, index = drm_connector_index(connector); + /* + * Construction of atomic state updates can race with a connector + * hot-add which might overflow. In this case flip the table and just + * restart the entire ioctl - no one is fast enough to livelock a cpu + * with physical hotplug events anyway. + * + * Note that we only grab the indexes once we have the right lock to + * prevent hotplug/unplugging of connectors. So removal is no problem, + * at most the array is a bit too large. + */ + if (index >= state->num_connector) { + DRM_DEBUG_KMS("Hot-added connector would overflow state array, restarting\n"); + return -EAGAIN; + } + if (state->connector_states[index]) return state->connector_states[index]; @@ -499,10 +516,9 @@ int drm_atomic_connectors_for_crtc(struct drm_atomic_state *state, struct drm_crtc *crtc) { - int nconnectors = state->dev->mode_config.num_connector; int i, num_connected_connectors = 0; - for (i = 0; i < nconnectors; i++) { + for (i = 0; i < state->num_connector; i++) { struct drm_connector_state *conn_state; conn_state = state->connector_states[i]; diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0cd054615920..99095ef147ef 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -249,7 +249,6 @@ static int mode_fixup(struct drm_atomic_state *state) { int ncrtcs = state->dev->mode_config.num_crtc; - int nconnectors = state->dev->mode_config.num_connector; struct drm_crtc_state *crtc_state; struct drm_connector_state *conn_state; int i; @@ -264,7 +263,7 @@ mode_fixup(struct drm_atomic_state *state) drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode); } - for (i = 0; i < nconnectors; i++) { + for (i = 0; i < state->num_connector; i++) { struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder; @@ -336,7 +335,6 @@ drm_atomic_helper_check_prepare(struct drm_device *dev, struct drm_atomic_state *state) { int ncrtcs = dev->mode_config.num_crtc; - int nconnectors = dev->mode_config.num_connector; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; int i, ret; @@ -361,7 +359,7 @@ drm_atomic_helper_check_prepare(struct drm_device *dev, } } - for (i = 0; i < nconnectors; i++) { + for (i = 0; i < state->num_connector; i++) { /* * This only sets crtc->mode_changed for routing changes, * drivers must set crtc->mode_changed themselves when connector @@ -485,10 +483,9 @@ static void disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) { int ncrtcs = old_state->dev->mode_config.num_crtc; - int nconnectors = old_state->dev->mode_config.num_connector; int i; - for (i = 0; i < nconnectors; i++) { + for (i = 0; i < old_state->num_connector; i++) { struct drm_connector_state *old_conn_state; struct drm_connector *connector; struct drm_encoder_helper_funcs *funcs; @@ -553,12 +550,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) static void set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state) { - int nconnectors = dev->mode_config.num_connector; int ncrtcs = old_state->dev->mode_config.num_crtc; int i; /* clear out existing links */ - for (i = 0; i < nconnectors; i++) { + for (i = 0; i < old_state->num_connector; i++) { struct drm_connector *connector; connector = old_state->connectors[i]; @@ -573,7 +569,7 @@ set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state) } /* set new links */ - for (i = 0; i < nconnectors; i++) { + for (i = 0; i < old_state->num_connector; i++) { struct drm_connector *connector; connector = old_state->connectors[i]; @@ -608,7 +604,6 @@ static void crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) { int ncrtcs = old_state->dev->mode_config.num_crtc; - int nconnectors = old_state->dev->mode_config.num_connector; int i; for (i = 0; i < ncrtcs; i++) { @@ -626,7 +621,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) funcs->mode_set_nofb(crtc); } - for (i = 0; i < nconnectors; i++) { + for (i = 0; i < old_state->num_connector; i++) { struct drm_connector *connector; struct drm_crtc_state *new_crtc_state; struct drm_encoder_helper_funcs *funcs; @@ -687,7 +682,6 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev, struct drm_atomic_state *old_state) { int ncrtcs = old_state->dev->mode_config.num_crtc; - int nconnectors = old_state->dev->mode_config.num_connector; int i; for (i = 0; i < ncrtcs; i++) { @@ -706,7 +700,7 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev, funcs->commit(crtc); } - for (i = 0; i < nconnectors; i++) { + for (i = 0; i < old_state->num_connector; i++) { struct drm_connector *connector; struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder; @@ -1304,7 +1298,6 @@ static int update_output_state(struct drm_atomic_state *state, { struct drm_device *dev = set->crtc->dev; struct drm_connector_state *conn_state; - int nconnectors = state->dev->mode_config.num_connector; int ncrtcs = state->dev->mode_config.num_crtc; int ret, i, j; @@ -1333,7 +1326,7 @@ static int update_output_state(struct drm_atomic_state *state, } /* Then recompute connector->crtc links and crtc enabling state. */ - for (i = 0; i < nconnectors; i++) { + for (i = 0; i < state->num_connector; i++) { struct drm_connector *connector; connector = state->connectors[i]; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 91c09520aad3..cdbae7bdac70 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -845,6 +845,7 @@ struct drm_bridge { * @plane_states: pointer to array of plane states pointers * @crtcs: pointer to array of CRTC pointers * @crtc_states: pointer to array of CRTC states pointers + * @num_connector: size of the @connectors and @connector_states arrays * @connectors: pointer to array of connector pointers * @connector_states: pointer to array of connector states pointers * @acquire_ctx: acquire context for this atomic modeset state update @@ -856,6 +857,7 @@ struct drm_atomic_state { struct drm_plane_state **plane_states; struct drm_crtc **crtcs; struct drm_crtc_state **crtc_states; + int num_connector; struct drm_connector **connectors; struct drm_connector_state **connector_states;