Re: [PATCH 03/10] drm/i915: allow the existing error_state to be destroyed

[Daniel Vetter <daniel@ffwll.ch>]

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 <eugeni.dodonov@intel.com>


-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48