From owner-svn-src-head@FreeBSD.ORG Mon Jul 18 12:52:49 2011 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4A89E1065675; Mon, 18 Jul 2011 12:52:49 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id E2F928FC14; Mon, 18 Jul 2011 12:52:47 +0000 (UTC) Received: from odyssey.starpoint.kiev.ua (alpha-e.starpoint.kiev.ua [212.40.38.101]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id PAA22624; Mon, 18 Jul 2011 15:52:46 +0300 (EEST) (envelope-from avg@FreeBSD.org) Message-ID: <4E242C9E.6010604@FreeBSD.org> Date: Mon, 18 Jul 2011 15:52:46 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:5.0) Gecko/20110705 Thunderbird/5.0 MIME-Version: 1.0 To: Hans Petter Selasky References: <201107132107.p6DL7ojq099900@svn.freebsd.org> In-Reply-To: <201107132107.p6DL7ojq099900@svn.freebsd.org> X-Enigmail-Version: 1.2pre Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r223989 - head/sys/dev/usb/input X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Jul 2011 12:52:49 -0000 on 14/07/2011 00:07 Hans Petter Selasky said the following: > Author: hselasky > Date: Wed Jul 13 21:07:50 2011 > New Revision: 223989 > URL: http://svn.freebsd.org/changeset/base/223989 > > Log: > Fix for dump after shutdown with USB keyboard plugged in. It appears that the > system timer is stopped during shutdown and that the pause() statement in ukbd > causes infinite hang in this regard. The fix is to use mi_switch() instead of > pause() to do the required task switch to ensure that the required USB processes > get executed. I have a question partially about this particular change, partially about USB/ukbd polling mode in general. See below: > Reported by: Mike_Karels@mcafee.com > MFC after: 1 week > > Modified: > head/sys/dev/usb/input/ukbd.c > > Modified: head/sys/dev/usb/input/ukbd.c > ============================================================================== > --- head/sys/dev/usb/input/ukbd.c Wed Jul 13 21:07:41 2011 (r223988) > +++ head/sys/dev/usb/input/ukbd.c Wed Jul 13 21:07:50 2011 (r223989) > @@ -59,6 +59,8 @@ __FBSDID("$FreeBSD$"); > #include > #include > #include > +#include > +#include > #include > > #include > @@ -386,6 +388,33 @@ ukbd_put_key(struct ukbd_softc *sc, uint > } > > static void > +ukbd_yield(void) > +{ > + struct thread *td = curthread; > + uint32_t old_prio; > + > + DROP_GIANT(); > + > + thread_lock(td); > + > + /* get current priority */ > + old_prio = td->td_base_pri; > + > + /* set new priority */ > + sched_prio(td, td->td_user_pri); > + > + /* cause a task switch */ > + mi_switch(SW_INVOL | SWT_RELINQUISH, NULL); > + > + /* restore priority */ > + sched_prio(td, old_prio); > + > + thread_unlock(td); > + > + PICKUP_GIANT(); > +} > + > +static void > ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait) > { > DPRINTFN(2, "polling\n"); > @@ -396,8 +425,9 @@ ukbd_do_poll(struct ukbd_softc *sc, uint > > if (kdb_active == 0) { The question is: why this special subcase is needed at all. If I understand correctly, the polling mode is used only in some special situations/contexts. So why not always use the generic code (after this if-block) that does "proper" polling? What do we win when using this special case that seems to depend on the scheduler? Unfortunately I couldn't fully understand commit log of r203896. One of the reasons I am asking about this is that soon-ish we may have changes that disable scheduler in a context where panicstr != NULL. > while (sc->sc_inputs == 0) { > - /* make sure the USB code gets a chance to run */ > - pause("UKBD", 1); > + > + /* give USB threads a chance to run */ > + ukbd_yield(); > > /* check if we should wait */ > if (!wait) -- Andriy Gapon