On Tue, Jun 30, 2015 at 01:30:27PM -0700, Jesse Barnes wrote: > On 06/30/2015 07:36 AM, Chris Wilson wrote: > > On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote: > >> 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>: > >>> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: > >>>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > >>>> > >>>> Let's make sure the future Paulos don't forget that we need > >>>> struct_mutex when touching dev_priv->mm.stolen. > >>>> > >>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > >>>> --- > >>>> drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ > >>>> 1 file changed, 13 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > >>>> index 793bcba..cac1bce 100644 > >>>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > >>>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > >>>> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev, > >>>> int compression_threshold = 1; > >>>> int ret; > >>>> > >>>> + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > >>> > >>> I'm not a huge fan of vague mutex warnings that don't even check the owner. > >>> I'm espcially not a fan of adding a WARN and not handling the error. > >> > >> But then, what exactly is your proposal? What would you like to see here? > >> > >> We can discard this patch if you want. But I hope you're not > >> advocating for lockdep_assert_held(), because if I switch to lockdep, > >> then Daniel is going to deny it again. Also, this type of WARN_ON is a > >> common pattern on our codebase... > > > > I'm just trying to convince Daniel that blindly using this pattern is > > the wrong approach and encouraging a proliferation of unhandled WARN_ON > > doesn't improve driver robustness. > > I think they serve as useful documentation at the very least, whether in > lockdep form, WARN form, or BUG form. It's not really something we can > recover from either (maybe returning early before touching data?), so... Not grabbing a lock is generally a harmless error since real races out there are rare with X being single-threaded and all that. Especially in stuff called from modeset code. Hence I think just WARN_ON plus continuing on with blissful ignorance is the best approach. I don't the lockdep versions personally since they don't work when lockdep is disabled, which is pretty much always the case. Might be useful to do an assert_mutex_held which always does the most paranoid check (i.e. WARN_ON without lockdep, lockdep_assert_held with lockdep). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
