Re: [PATCH 09/11] drm/i915: don't use a temp buffer for opregion debugfs file

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: Deepak M <m.deepak@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 09/11] drm/i915: don't use a temp buffer for opregion debugfs file
Date: Mon, 14 Dec 2015 16:45:17 +0200	[thread overview]
Message-ID: <20151214144517.GN4437@intel.com> (raw)
In-Reply-To: <87lh8xt7r1.fsf@intel.com>

On Mon, Dec 14, 2015 at 04:38:58PM +0200, Jani Nikula wrote:
> On Mon, 14 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Dec 14, 2015 at 12:50:53PM +0200, Jani Nikula wrote:
> >> Hasn't been necessary since
> >> 
> >> commit 115719fceaa733d646e39cdce83cc32ddb891a49
> >> Author: Williams, Dan J <dan.j.williams@intel.com>
> >> Date:   Mon Oct 12 21:12:57 2015 +0000
> >> 
> >>     i915: switch from acpi_os_ioremap to memremap
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_debugfs.c | 11 ++---------
> >>  1 file changed, 2 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index 24318b79bcfc..a9e1f18c36d1 100644
> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> @@ -1842,25 +1842,18 @@ static int i915_opregion(struct seq_file *m, void *unused)
> >>  	struct drm_device *dev = node->minor->dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_opregion *opregion = &dev_priv->opregion;
> >> -	void *data = kmalloc(OPREGION_SIZE, GFP_KERNEL);
> >>  	int ret;
> >>  
> >> -	if (data == NULL)
> >> -		return -ENOMEM;
> >> -
> >>  	ret = mutex_lock_interruptible(&dev->struct_mutex);
> >>  	if (ret)
> >>  		goto out;
> >
> > A bit off topic, but I wonder what this locking is supposed to protect?
> 
> I thought about it when Chris mentioned locking in the next patch. Some
> opregion mailboxes do change due to driver <-> bios notifications, even
> if the VBT doesn't. Not sure if there's any chance this could lead to us
> dumping an inconsistent opregion, and if so, what difference that could
> make. So I thought better safe than sorry and just let it be.

Do we actually take struct_mutex in places where we intentionally poke
at opregion mailboxes?

The BIOS notifications could anyway be fully asynchronous no? So no way
we could protect against that, except maybe via some ACPI AML lock.

> 
> BR,
> Jani.
> 
> >
> >>  
> >> -	if (opregion->header) {
> >> -		memcpy(data, opregion->header, OPREGION_SIZE);
> >> -		seq_write(m, data, OPREGION_SIZE);
> >> -	}
> >> +	if (opregion->header)
> >> +		seq_write(m, opregion->header, OPREGION_SIZE);
> >>  
> >>  	mutex_unlock(&dev->struct_mutex);
> >>  
> >>  out:
> >> -	kfree(data);
> >>  	return 0;
> >>  }
> >>  
> >> -- 
> >> 2.1.4
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx