| Message ID | 1379368603-18577-1-git-send-email-Tom.O'Rourke@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 16, 2013 at 02:56:43PM -0700, Tom.O'Rourke@intel.com wrote: > From: Tom O'Rourke <Tom.O'Rourke@intel.com> > > Enabling rps (turbo setup) was put in a work queue because it may > take quite awhile. This change flushes the work queue to initialize > rps values before use by sysfs or debugfs. Specifically, > rps.delayed_resume_work is flushed before using rps.hw_max, > rps.max_delay, rps.min_delay, or rps.cur_delay. > > This change fixes a problem in sysfs where show functions using > uninitialized values show incorrect values and store functions > using uninitialized values in range checks incorrectly fail to > store valid input values. This change also addresses similar use > before initialized problems in debugfs. > > Change-Id: Ib9c4f2066b65013094cb9278fc17958a964836e7 > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com> I think we can exercise this by going through a suspend/resume cycle and racing concurrent reads/writes of the sysfs files in a 2nd process. With the igt_fork and igt_system_suspend_autoresume helpers in our kernel testsuite repro this should be a really quick testcase to write ... I'd go for a generic "read all sysfs files" approach to increase the chances of catching other similar fallout in the future. -Daniel > --- > drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++ > drivers/gpu/drm/i915/i915_sysfs.c | 10 ++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 1d77624..52e90a1 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -843,6 +843,8 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused) > drm_i915_private_t *dev_priv = dev->dev_private; > int ret; > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > if (IS_GEN5(dev)) { > u16 rgvswctl = I915_READ16(MEMSWCTL); > u16 rgvstat = I915_READ16(MEMSTAT_ILK); > @@ -1321,6 +1323,8 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused) > return 0; > } > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); > if (ret) > return ret; > @@ -1972,6 +1976,8 @@ i915_max_freq_get(void *data, u64 *val) > if (!(IS_GEN6(dev) || IS_GEN7(dev))) > return -ENODEV; > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); > if (ret) > return ret; > @@ -1996,6 +2002,8 @@ i915_max_freq_set(void *data, u64 val) > if (!(IS_GEN6(dev) || IS_GEN7(dev))) > return -ENODEV; > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > DRM_DEBUG_DRIVER("Manually setting max freq to %llu\n", val); > > ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); > @@ -2034,6 +2042,8 @@ i915_min_freq_get(void *data, u64 *val) > if (!(IS_GEN6(dev) || IS_GEN7(dev))) > return -ENODEV; > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); > if (ret) > return ret; > @@ -2058,6 +2068,8 @@ i915_min_freq_set(void *data, u64 val) > if (!(IS_GEN6(dev) || IS_GEN7(dev))) > return -ENODEV; > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > DRM_DEBUG_DRIVER("Manually setting min freq to %llu\n", val); > > ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > index d572435..270892b 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -213,6 +213,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev, > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > mutex_lock(&dev_priv->rps.hw_lock); > if (IS_VALLEYVIEW(dev_priv->dev)) { > u32 freq; > @@ -245,6 +247,8 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > mutex_lock(&dev_priv->rps.hw_lock); > if (IS_VALLEYVIEW(dev_priv->dev)) > ret = vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.max_delay); > @@ -269,6 +273,8 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, > if (ret) > return ret; > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > mutex_lock(&dev_priv->rps.hw_lock); > > if (IS_VALLEYVIEW(dev_priv->dev)) { > @@ -317,6 +323,8 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > mutex_lock(&dev_priv->rps.hw_lock); > if (IS_VALLEYVIEW(dev_priv->dev)) > ret = vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.min_delay); > @@ -341,6 +349,8 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, > if (ret) > return ret; > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > mutex_lock(&dev_priv->rps.hw_lock); > > if (IS_VALLEYVIEW(dev)) { > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Sep 17, 2013 at 12:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Sep 16, 2013 at 02:56:43PM -0700, Tom.O'Rourke@intel.com wrote: >> From: Tom O'Rourke <Tom.O'Rourke@intel.com> >> >> Enabling rps (turbo setup) was put in a work queue because it may >> take quite awhile. This change flushes the work queue to initialize >> rps values before use by sysfs or debugfs. Specifically, >> rps.delayed_resume_work is flushed before using rps.hw_max, >> rps.max_delay, rps.min_delay, or rps.cur_delay. >> >> This change fixes a problem in sysfs where show functions using >> uninitialized values show incorrect values and store functions >> using uninitialized values in range checks incorrectly fail to >> store valid input values. This change also addresses similar use >> before initialized problems in debugfs. >> >> Change-Id: Ib9c4f2066b65013094cb9278fc17958a964836e7 >> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com> > > I think we can exercise this by going through a suspend/resume cycle and > racing concurrent reads/writes of the sysfs files in a 2nd process. With > the igt_fork and igt_system_suspend_autoresume helpers in our kernel > testsuite repro this should be a really quick testcase to write ... > > I'd go for a generic "read all sysfs files" approach to increase the > chances of catching other similar fallout in the future. To clarify: I'd like to see a testcase for this so that we can make sure it keeps working. I guess we unfortunately can't test for functional correctness by just resuming since we won't see uninitialized data. But at least we can test for the flush_delayed_work deadlock implications, which have bitten us badly in the past on numerous occasions. And maybe we can provoke hangs if we adjust the rps values while the setup hasn't completed yet, so might also give us weak coverage there. Cheers, Daniel
On Tue, Sep 17, 2013 at 06:05:27PM +0200, Daniel Vetter wrote: > On Tue, Sep 17, 2013 at 12:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Sep 16, 2013 at 02:56:43PM -0700, Tom.O'Rourke@intel.com wrote: > >> From: Tom O'Rourke <Tom.O'Rourke@intel.com> > >> > >> Enabling rps (turbo setup) was put in a work queue because it may > >> take quite awhile. This change flushes the work queue to initialize > >> rps values before use by sysfs or debugfs. Specifically, > >> rps.delayed_resume_work is flushed before using rps.hw_max, > >> rps.max_delay, rps.min_delay, or rps.cur_delay. > >> > >> This change fixes a problem in sysfs where show functions using > >> uninitialized values show incorrect values and store functions > >> using uninitialized values in range checks incorrectly fail to > >> store valid input values. This change also addresses similar use > >> before initialized problems in debugfs. > >> > >> Change-Id: Ib9c4f2066b65013094cb9278fc17958a964836e7 > >> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com> > > > > I think we can exercise this by going through a suspend/resume cycle and > > racing concurrent reads/writes of the sysfs files in a 2nd process. With > > the igt_fork and igt_system_suspend_autoresume helpers in our kernel > > testsuite repro this should be a really quick testcase to write ... > > > > I'd go for a generic "read all sysfs files" approach to increase the > > chances of catching other similar fallout in the future. > > To clarify: I'd like to see a testcase for this so that we can make > sure it keeps working. I guess we unfortunately can't test for > functional correctness by just resuming since we won't see > uninitialized data. But at least we can test for the > flush_delayed_work deadlock implications, which have bitten us badly > in the past on numerous occasions. And maybe we can provoke hangs if > we adjust the rps values while the setup hasn't completed yet, so > might also give us weak coverage there. I've stitched something together with little enthusiasm ... *insert maintainer rant* Patch merged. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 1d77624..52e90a1 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -843,6 +843,8 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused) drm_i915_private_t *dev_priv = dev->dev_private; int ret; + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + if (IS_GEN5(dev)) { u16 rgvswctl = I915_READ16(MEMSWCTL); u16 rgvstat = I915_READ16(MEMSTAT_ILK); @@ -1321,6 +1323,8 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused) return 0; } + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); if (ret) return ret; @@ -1972,6 +1976,8 @@ i915_max_freq_get(void *data, u64 *val) if (!(IS_GEN6(dev) || IS_GEN7(dev))) return -ENODEV; + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); if (ret) return ret; @@ -1996,6 +2002,8 @@ i915_max_freq_set(void *data, u64 val) if (!(IS_GEN6(dev) || IS_GEN7(dev))) return -ENODEV; + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + DRM_DEBUG_DRIVER("Manually setting max freq to %llu\n", val); ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); @@ -2034,6 +2042,8 @@ i915_min_freq_get(void *data, u64 *val) if (!(IS_GEN6(dev) || IS_GEN7(dev))) return -ENODEV; + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); if (ret) return ret; @@ -2058,6 +2068,8 @@ i915_min_freq_set(void *data, u64 val) if (!(IS_GEN6(dev) || IS_GEN7(dev))) return -ENODEV; + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + DRM_DEBUG_DRIVER("Manually setting min freq to %llu\n", val); ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock); diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index d572435..270892b 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -213,6 +213,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev, struct drm_i915_private *dev_priv = dev->dev_private; int ret; + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + mutex_lock(&dev_priv->rps.hw_lock); if (IS_VALLEYVIEW(dev_priv->dev)) { u32 freq; @@ -245,6 +247,8 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute struct drm_i915_private *dev_priv = dev->dev_private; int ret; + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + mutex_lock(&dev_priv->rps.hw_lock); if (IS_VALLEYVIEW(dev_priv->dev)) ret = vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.max_delay); @@ -269,6 +273,8 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, if (ret) return ret; + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + mutex_lock(&dev_priv->rps.hw_lock); if (IS_VALLEYVIEW(dev_priv->dev)) { @@ -317,6 +323,8 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute struct drm_i915_private *dev_priv = dev->dev_private; int ret; + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + mutex_lock(&dev_priv->rps.hw_lock); if (IS_VALLEYVIEW(dev_priv->dev)) ret = vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.min_delay); @@ -341,6 +349,8 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, if (ret) return ret; + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + mutex_lock(&dev_priv->rps.hw_lock); if (IS_VALLEYVIEW(dev)) {