[Intel-gfx] [PATCH] drm: Assert correct locking for drm_send_vblank_event
On Fri, Sep 12, 2014 at 01:03:51PM -0400, Peter Hurley wrote: > On 09/12/2014 12:04 PM, Chris Wilson wrote: > > On Fri, Sep 12, 2014 at 05:34:56PM +0200, Daniel Vetter wrote: > >> On Fri, Sep 12, 2014 at 04:23:29PM +0100, Chris Wilson wrote: > >>> On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote: > The comment says that the caller must hold the dev->event_lock > spinlock, so let's enforce this. > > A quick audit over all driver shows that except for the one place in > i915 which motivated this all callers fullfill this requirement > already. > >>> > >>> Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in > >>> send_vblank_event() as well then. > >> > >> Meh, I've missed that one, that's actually better I think. I'll drop my > >> patch here. > > > > I thought assert_spin_lock was the preferred form? > > Actually, lockdep_assert_held() is the preferred form. > > See https://lkml.org/lkml/2014/9/3/171 Which unfortunately doesn't warn for all the normal users which are not insane enough to enable lockdep and so is totally useless to validate a driver that runs on metric piles of different chips (with a resulting combinatorial explosion of code-paths because hw designers are creative). And we rely a lot on random drive-by testers to report such issues. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[Intel-gfx] [PATCH] drm: Assert correct locking for drm_send_vblank_event
On Fri, Sep 12, 2014 at 04:23:29PM +0100, Chris Wilson wrote: > On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote: > > The comment says that the caller must hold the dev->event_lock > > spinlock, so let's enforce this. > > > > A quick audit over all driver shows that except for the one place in > > i915 which motivated this all callers fullfill this requirement > > already. > > Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in > send_vblank_event() as well then. Meh, I've missed that one, that's actually better I think. I'll drop my patch here. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[Intel-gfx] [PATCH] drm: Assert correct locking for drm_send_vblank_event
On Fri, Sep 12, 2014 at 05:34:56PM +0200, Daniel Vetter wrote: > On Fri, Sep 12, 2014 at 04:23:29PM +0100, Chris Wilson wrote: > > On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote: > > > The comment says that the caller must hold the dev->event_lock > > > spinlock, so let's enforce this. > > > > > > A quick audit over all driver shows that except for the one place in > > > i915 which motivated this all callers fullfill this requirement > > > already. > > > > Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in > > send_vblank_event() as well then. > > Meh, I've missed that one, that's actually better I think. I'll drop my > patch here. I thought assert_spin_lock was the preferred form? -Chris -- Chris Wilson, Intel Open Source Technology Centre
[Intel-gfx] [PATCH] drm: Assert correct locking for drm_send_vblank_event
On 09/12/2014 01:25 PM, Daniel Vetter wrote: > On Fri, Sep 12, 2014 at 01:03:51PM -0400, Peter Hurley wrote: >> On 09/12/2014 12:04 PM, Chris Wilson wrote: >>> On Fri, Sep 12, 2014 at 05:34:56PM +0200, Daniel Vetter wrote: On Fri, Sep 12, 2014 at 04:23:29PM +0100, Chris Wilson wrote: > On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote: >> The comment says that the caller must hold the dev->event_lock >> spinlock, so let's enforce this. >> >> A quick audit over all driver shows that except for the one place in >> i915 which motivated this all callers fullfill this requirement >> already. > > Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in > send_vblank_event() as well then. Meh, I've missed that one, that's actually better I think. I'll drop my patch here. >>> >>> I thought assert_spin_lock was the preferred form? >> >> Actually, lockdep_assert_held() is the preferred form. >> >> See https://lkml.org/lkml/2014/9/3/171 > > Which unfortunately doesn't warn for all the normal users which are not > insane enough to enable lockdep and so is totally useless to validate a > driver that runs on metric piles of different chips (with a resulting > combinatorial explosion of code-paths because hw designers are creative). > And we rely a lot on random drive-by testers to report such issues. I know. When I wrote [in that thread linked above]: On Wed, Sep 03, 2014 at 10:50:01AM -0400, Peter Hurley wrote: > So a lockdep-only assert is unlikely to draw attention to existing bugs, > especially in established drivers. here's the replies I got: Peter Zijlstra wrote: > By the same logic lockdep will not find locking errors in established > drivers. and On 09/04/2014 01:14 AM, Ingo Molnar wrote: > Indeed, this patch is ill-advised in several ways: > > - it extends an API variant that we want to phase > > - emits a warning even if say lockdep has already emitted a > warning and locking state is not guaranteed to be consistent. > > - makes the kernel more expensive once fully debugged, in that > non-fatal checks are unconditional. :/ Regards, Peter Hurley
[Intel-gfx] [PATCH] drm: Assert correct locking for drm_send_vblank_event
On 09/12/2014 12:04 PM, Chris Wilson wrote: > On Fri, Sep 12, 2014 at 05:34:56PM +0200, Daniel Vetter wrote: >> On Fri, Sep 12, 2014 at 04:23:29PM +0100, Chris Wilson wrote: >>> On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote: The comment says that the caller must hold the dev->event_lock spinlock, so let's enforce this. A quick audit over all driver shows that except for the one place in i915 which motivated this all callers fullfill this requirement already. >>> >>> Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in >>> send_vblank_event() as well then. >> >> Meh, I've missed that one, that's actually better I think. I'll drop my >> patch here. > > I thought assert_spin_lock was the preferred form? Actually, lockdep_assert_held() is the preferred form. See https://lkml.org/lkml/2014/9/3/171 Regards, Peter Hurley
