Date: Thu, 25 May 2006 21:01:09 -0600 From: Scott Long <scottl@samsco.org> To: Warner Losh <imp@FreeBSD.org> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/syscons/apm apm_saver.c src/sys/i386/bios apm.c apm.h Message-ID: <44766F75.9060100@samsco.org> In-Reply-To: <200605252306.k4PN6cCS081708@repoman.freebsd.org> References: <200605252306.k4PN6cCS081708@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Warner Losh wrote: > imp 2006-05-25 23:06:38 UTC > > FreeBSD src repository > > Modified files: > sys/dev/syscons/apm apm_saver.c > sys/i386/bios apm.c apm.h > Log: > APM was calling the suspend process from a timeout. This meant that > other timeouts could not happen while suspending, including timeouts > for things like msleep. This caused the system to hang on suspend > when the cbb was enabled, since its suspend path powered down the > socket which used a timeout to wait for it to be done. > > APM now creates a thread when it is enabled, and deletes the thread > when it is disabled. This thread takes the place of the timeout by > doing its polling every ~.9s. When the thread is disabled, it will > wakeup early, otherwise it times out and polls the varius things the > old timeout polled (APM events, suspend delays, etc). > > This makes my Sony VAIO 505TS suspend/resume correctly when APM is > enabled (ACPI is black listed on my 505TS). > > This will likely fix other problems with the suspend path where > drivers would sleep with msleep and/or do other timeouts. Maybe > there's some special case code that would use DELAY while suspending > and msleep otherwise that can be revisited and removed. > > This was also tested by glebius@, who pointed out that in the patch I > sent him, I'd forgotten apm_saver.c > > MFC After: 3 weeks In the past, I've been against mandating that callouts/timeouts/generic taskqueues should not be allowed to sleep. However, after looking over the history of this problem as well as others, it seems that it's just too easy for driver authors to make bad assumptions and wind up with a priority inversion/deadlock like this. It would be relatively trivial to mark these contexts as being non-sleepable and have the msleep code enforce it, like is done with ithreads. What do you think? Anyways, thanks for looking at this and fixing it. Scott
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?44766F75.9060100>