On 07/01/2015 06:56 AM, Daniel Vetter wrote: > 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). Maybe we should add WARN_ONs to the lockdep_assert macros in the !CONFIG_LOCKDEP case. That would give us documentation, checking in both cases, and everyone would be happy, right? Jesse _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
