On Wed, Jul 01, 2015 at 08:17:37AM -0700, Jesse Barnes wrote: > 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? tbh never tried that ;-) -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
