| Message ID | 74664a556a56d0eceb0029bbd77ffc1d771b0628.1450089383.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > > - 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
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. 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
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
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; - 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; }
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(-)