From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 03/10] drm/i915: allow the existing error_state to be destroyed Date: Fri, 4 May 2012 21:58:23 +0200 Message-ID: <20120504195823.GG5443@phenom.ffwll.local> References: <1335532667-10597-1-git-send-email-daniel.vetter@ffwll.ch> <1335532667-10597-3-git-send-email-daniel.vetter@ffwll.ch> <20120504115632.GE5443@phenom.ffwll.local> <4FA40EB5.9070503@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-we0-f177.google.com (mail-we0-f177.google.com [74.125.82.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 97C589E8BB for ; Fri, 4 May 2012 12:57:18 -0700 (PDT) Received: by werp11 with SMTP id p11so2404370wer.36 for ; Fri, 04 May 2012 12:57:17 -0700 (PDT) Content-Disposition: inline In-Reply-To: <4FA40EB5.9070503@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: eugeni.dodonov@intel.com Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Fri, May 04, 2012 at 02:15:33PM -0300, Eugeni Dodonov wrote: > On 05/04/2012 08:56 AM, Daniel Vetter wrote: > >>+static int i915_error_state_release(struct inode *inode, struct file *file) > >>+{ > >>+ struct seq_file *m = file->private_data; > >>+ struct i915_error_state_file_priv *error_priv = m->private; > >>+ > >>+ if (error_priv->error) > >>+ kref_put(&error_priv->error->ref, i915_error_state_free); > >>+ kfree(error_priv); > > Maybe a stupid question, but shouldn't we hold the error_lock here as well? That's the cool thing with ref-counting: As long as someone is still holding a referencing, the object won't ever disappear. The tricky part is to ensure that every time someone could still access a reference, it actually does. Let's go through the list: - When creating the error state in the hangcheck code we init the refcount to 1. We drop that reference if the error state capturing fails. On success we transfer this reference to dev_priv->error_state (hence no additional refcounting is necessary). - As long as dev_priv->error_state is non-NULL, it holds a reference onto that error_state. - When we open the error_state debugfs file we grab a reference and only drop it when we close the file. The important bit in that dance is that the refcount held by dev_priv->error_state and whether that pointer is non-NULL need to be always consistent (to avoid somebody reading a non-NULL error_state pointer while the refcount has already dropped to zero). This is the only place we need locking - and what my comment about not mistaking ref-counting for locking alluded to. Cheers, Daniel > But apart from that: > Reviewed-by: Eugeni Dodonov -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48