From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752767AbcERJTB (ORCPT ); Wed, 18 May 2016 05:19:01 -0400 Received: from hqemgate15.nvidia.com ([216.228.121.64]:12150 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750792AbcERJS7 (ORCPT ); Wed, 18 May 2016 05:18:59 -0400 X-PGP-Universal: processed; by hqnvupgp08.nvidia.com on Wed, 18 May 2016 02:17:45 -0700 Subject: Re: [PATCH] drm/tegra: Fix crash caused by reference count imbalance To: Daniel Vetter References: <1463502435-29217-1-git-send-email-jonathanh@nvidia.com> <20160517164632.GT27098@phenom.ffwll.local> <573B54F2.4010300@nvidia.com> CC: Thierry Reding , David Airlie , Stephen Warren , Alexandre Courbot , "linux-tegra@vger.kernel.org" , Linux Kernel Mailing List , dri-devel From: Jon Hunter Message-ID: <573C337C.3030009@nvidia.com> Date: Wed, 18 May 2016 10:18:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.21.132.103] X-ClientProxiedBy: UKMAIL102.nvidia.com (10.26.138.15) To UKMAIL101.nvidia.com (10.26.138.13) Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 17/05/16 18:36, Daniel Vetter wrote: > On Tue, May 17, 2016 at 7:29 PM, Jon Hunter wrote: >>>> @@ -764,6 +769,9 @@ tegra_dsi_connector_duplicate_state(struct drm_connector *connector) >>>> if (!copy) >>>> return NULL; >>>> >>>> + if (copy->base.crtc) >>>> + drm_connector_reference(connector); >>>> + >>> >>> Please use __drm_atomic_helper_connector_duplicate_state instead of >>> open-coding it. >> >> Unfortunately, tegra is allocating and duplicating memory for the entire >> tegra_dsi_state structure (of which drm_connector_state is a member) in >> this function and so I was not able to do that. However, may be Thierry >> can comment on whether that is completely necessary and if we can move >> to using __drm_atomic_helper_connector_duplicate_state() instead. > > Check out how other drivers are using this helper - it is explicitly > for the case where you duplicate the entire struct, and it just > initializes the core part from drm. You can then add your own fixup > code afterwards. It also doesn't matter whether you do kmalloc or > kcalloc or kmemdup - it does a memcpy of its own to make sure state > gets copied. I had a look but I don't see anyone using the __drm_atomic_helper_connector_duplicate_state() helper, I only see that drivers are using drm_atomic_helper_connector_duplicate_state() directly. Yes I understand that this helper is doing an explicit copy of the entire drm_connector_state struct and yes I could do something like the following ... static struct drm_connector_state * tegra_dsi_connector_duplicate_state(struct drm_connector *connector) { struct tegra_dsi_state *state = to_dsi_state(connector->state); struct tegra_dsi_state *copy; copy = kmemdup(state, sizeof(*state), GFP_KERNEL); if (!copy) return NULL; __drm_atomic_helper_connector_duplicate_state(connector, ©->base); return ©->base; } ... however, this means that I am copying the drm_connector_state twice and this is what I was trying to avoid. Sorry if I am misunderstanding you here, but I don't see how I can avoid the 2nd copy if I use __drm_atomic_helper_connector_duplicate_state(). Cheers Jon -- nvpublic