Re: [PATCH] drm: Assert correct locking for drm_send_vblank_event

[Peter Hurley <peter@hurleysoftware.com>]

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 <peterz@infradead.org> 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