From: "Theo de Raadt" <deraadt@openbsd.org>
Subject: Re: audio(4): postpone volume changes to the end of DVACT_WAKEUP
Cc: Alexandre Ratchov <alex@caoua.org>, Mark Kettenis <mark.kettenis@xs4all.nl>, tech@openbsd.org
Date: Tue, 13 Aug 2024 21:12:38 -0600

Theo de Raadt <deraadt@openbsd.org> wrote:

> > > That is a good idea.  Not sure how you spotted this, but I was playing
> > > around with having azalia(4) do its suspend/resume in
> > > DVACT_QUIESCE/DVACT_WAKEUP and this fixes the
> > > 
> > 
> > Same here, moved all the suspend/reaume logic to
> > DVACT_{QUIESCE,WAKEUP} which just worked for S3 suspend, btw.
> 
> I disagree strongly with that idea.  It may work in one driver, but it
> won't work in all of them.
> 
> But then someone will write or modify another driver, and look at the
> wrong example, and muddle it all up.
> 
> Please keep this seperate.  There are actions best done while cold.

A good example of this problem is in the dwiic driver.

I tried all my delay-related changes by keeping it DVACT_WAKEUP.
I got nowhere.  As soon as I moved the register restore logic to DVACT_RESUME,
things were immediately better.

During DVACT_WAKEUP, other stuff is happening in parallel, see subr_suspend.c,
I've removed all the #ifdef and irrelevant things:

        intr_disable_wakeup();
        cold = 0;
        intr_enable();
        splx(s);

        inittodr(gettime());
        clockintr_cpu_init(NULL);
        clockintr_trigger();

        sleep_resume(v);
        resume_randomness(rndbuf, rndbuflen);
        resume_mp();
        sched_start_secondary_cpus();
        vfs_stall(curproc, 0);
        bufq_restart();
        ^^^^^^^^^^ thundering herd of kernel parallism begins, including
        scheduler, timers, tasks, etc.

        config_suspend_all(DVACT_WAKEUP);

        ^^^^^^^^^^ why do sensitive operations here, when they can be done
        in a safer earlier step?

But most of all, during DVACT_RESUME there is no chance of an interrupt
happening and advancing the IO inside a driver -- due to a driver design
bug.

Also, you start doing things in DVACT_WAKEUP, there should be a consideration
for mutexes and interrupt blocking, against other subsystems which believe the
system is operational.

So I think the concept of changing DVACT_RESUME -> DVACT_WAKEUP is going to
be wrong 99% of the time; instead when we see something weird we should
rewrite to have both DVACT_RESUME *and* DVACT_WAKEUP methods for their
specific roles.