From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932521AbcERQDx (ORCPT ); Wed, 18 May 2016 12:03:53 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:35475 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932410AbcERQDv (ORCPT ); Wed, 18 May 2016 12:03:51 -0400 Date: Wed, 18 May 2016 18:03:49 +0200 From: Daniel Vetter To: Jon Hunter Cc: Thierry Reding , David Airlie , Daniel Vetter , Stephen Warren , Alexandre Courbot , dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V3] drm/tegra: Fix crash caused by reference count imbalance Message-ID: <20160518160349.GH27098@phenom.ffwll.local> Mail-Followup-To: Jon Hunter , Thierry Reding , David Airlie , Stephen Warren , Alexandre Courbot , dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org References: <1463585856-16606-1-git-send-email-jonathanh@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1463585856-16606-1-git-send-email-jonathanh@nvidia.com> X-Operating-System: Linux phenom 4.6.0-rc5+ User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 18, 2016 at 04:37:36PM +0100, Jon Hunter wrote: > Commit d2307dea14a4 ("drm/atomic: use connector references (v3)") added > reference counting for DRM connectors and this caused a crash when > exercising system suspend on Tegra114 Dalmore. > > The Tegra DSI driver implements a Tegra specific function, > tegra_dsi_connector_duplicate_state(), to duplicate the connector state > and destroys the state using the generic helper function, > drm_atomic_helper_connector_destroy_state(). Following commit > d2307dea14a4 ("drm/atomic: use connector references (v3)") there is > now an imbalance in the connector reference count because the Tegra > function to duplicate state does not take a reference when duplicating > the state information. However, the generic helper function to destroy > the state information assumes a reference has been taken and during > system suspend, when the connector state is destroyed, this leads to a > crash because we attempt to put the reference for an object that has > already been freed. > > Fix this by calling __drm_atomic_helper_connector_duplicate_state() from > tegra_dsi_connector_duplicate_state() to ensure that we take a reference > on a connector if crtc is set. Note that this will also copy the > connector state a 2nd time, but this should be harmless. > > By fixing tegra_dsi_connector_duplicate_state() to take a reference, > although a crash was no longer seen, it was then observed that after > each system suspend-resume cycle, the reference would be one greater > than before the suspend-resume cycle. Following commit d2307dea14a4 > ("drm/atomic: use connector references (v3)"), it was found that we > also need to put the reference when calling the function > tegra_dsi_connector_reset() before freeing the state. Fix this by > updating tegra_dsi_connector_reset() to call the function > __drm_atomic_helper_connector_destroy_state() in order to put the > reference for the connector. > > Fixes: d2307dea14a4 ("drm/atomic: use connector references (v3)") > > Signed-off-by: Jon Hunter > Reviewed-by: Daniel Vetter > Acked-by: Thierry Reding Applied to drm-misc, thanks. -Daniel > --- > > V3 changes: > - Dropped WARN_ON > > V2 changes: > - Updated to next-20160518 > - Replaced open coding of call to drm_connector_reference() with > __drm_atomic_helper_connector_duplicate_state() per Daniel's feedback. > > drivers/gpu/drm/tegra/dsi.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c > index 44e102799195..d1239ebc190f 100644 > --- a/drivers/gpu/drm/tegra/dsi.c > +++ b/drivers/gpu/drm/tegra/dsi.c > @@ -745,13 +745,17 @@ static void tegra_dsi_soft_reset(struct tegra_dsi *dsi) > > static void tegra_dsi_connector_reset(struct drm_connector *connector) > { > - struct tegra_dsi_state *state = > - kzalloc(sizeof(*state), GFP_KERNEL); > + struct tegra_dsi_state *state = kzalloc(sizeof(*state), GFP_KERNEL); > > - if (state) { > + if (!state) > + return; > + > + if (connector->state) { > + __drm_atomic_helper_connector_destroy_state(connector->state); > kfree(connector->state); > - __drm_atomic_helper_connector_reset(connector, &state->base); > } > + > + __drm_atomic_helper_connector_reset(connector, &state->base); > } > > static struct drm_connector_state * > @@ -764,6 +768,9 @@ tegra_dsi_connector_duplicate_state(struct drm_connector *connector) > if (!copy) > return NULL; > > + __drm_atomic_helper_connector_duplicate_state(connector, > + ©->base); > + > return ©->base; > } > > -- > 2.1.4 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch