From owner-freebsd-current@FreeBSD.ORG Tue Jan 31 08:57:27 2012 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 28FE71065672 for ; Tue, 31 Jan 2012 08:57:27 +0000 (UTC) (envelope-from fjoe@samodelkin.net) Received: from mail-tul01m020-f182.google.com (mail-tul01m020-f182.google.com [209.85.214.182]) by mx1.freebsd.org (Postfix) with ESMTP id EA9FF8FC12 for ; Tue, 31 Jan 2012 08:57:26 +0000 (UTC) Received: by obcwo16 with SMTP id wo16so7413374obc.13 for ; Tue, 31 Jan 2012 00:57:26 -0800 (PST) MIME-Version: 1.0 Received: by 10.182.179.70 with SMTP id de6mr34517254obc.22.1328000246128; Tue, 31 Jan 2012 00:57:26 -0800 (PST) Received: by 10.182.149.8 with HTTP; Tue, 31 Jan 2012 00:57:26 -0800 (PST) X-Originating-IP: [93.92.220.178] In-Reply-To: <1327945445.1686.36.camel@revolution.hippie.lan> References: <1327945445.1686.36.camel@revolution.hippie.lan> Date: Tue, 31 Jan 2012 15:57:26 +0700 Message-ID: From: Max Khon To: Ian Lepore Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: current@freebsd.org Subject: Re: FILTER_SCHEDULE_THREAD is not a bit-value X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 31 Jan 2012 08:57:27 -0000 Hello, On Tue, Jan 31, 2012 at 12:44 AM, Ian Lepore wrote: >> sys/bus.h documents the following semantics for FILTER_SCHEDULE_THREAD: >> >> /** >> =C2=A0* @brief Driver interrupt filter return values >> =C2=A0* >> =C2=A0* If a driver provides an interrupt filter routine it must return = an >> =C2=A0* integer consisting of oring together zero or more of the followi= ng >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^^^^^^^ >> =C2=A0* flags: >> =C2=A0* >> =C2=A0* =C2=A0 =C2=A0 =C2=A0FILTER_STRAY =C2=A0 =C2=A0- this device did = not trigger the interrupt >> =C2=A0* =C2=A0 =C2=A0 =C2=A0FILTER_HANDLED =C2=A0- the interrupt has bee= n fully handled and can be EOId >> =C2=A0* =C2=A0 =C2=A0 =C2=A0FILTER_SCHEDULE_THREAD - the threaded interr= upt handler should be >> =C2=A0* =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0scheduled to execute >> =C2=A0* >> =C2=A0* If the driver does not provide a filter, then the interrupt code= will >> =C2=A0* act is if the filter had returned FILTER_SCHEDULE_THREAD. =C2=A0= Note that it >> =C2=A0* is illegal to specify any other flag with FILTER_STRAY and that = it is >> =C2=A0* illegal to not specify either of FILTER_HANDLED or FILTER_SCHEDU= LE_THREAD >> =C2=A0* if FILTER_STRAY is not specified. >> =C2=A0*/ >> #define FILTER_STRAY =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00x01 >> #define FILTER_HANDLED =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00x02 >> #define FILTER_SCHEDULE_THREAD =C2=A00x04 >> >> But actually FILTER_SCHEDULE_THREAD is not used as a bit-value (see >> kern/kern_intr.c): >> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!thread) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 if (ret =3D=3D FILTER_SCHEDULE_THREAD) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 thread =3D 1; >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } >> >> There is at least one in-tree driver that could be broken because of >> this (asmc(8), but I found the problem with some other out-of-tree >> driver). >> This should be "if (ret & FILTER_SCHEDULE_THREAD)" instead. Attached >> patch fixes the problem. >> >> What do you think? >> >> Max > > I think returning (FILTER_HANDLED | FILTER_SCHEDULE_THREAD) makes no > sense given the definition "the interrupt has been fully handled and can > be EOId". =C2=A0If you EOI in the primary interrupt context and then sche= dule > a threaded handler to run as well you're likely to need complex locking > between the primary and threaded interrupt handlers and I was under the > impression that's just the sort of thing the filter/threaded scheme was > designed to avoid. I see no sense here. 1) You would have to implement locking anyway to protect concurrent access from ithread/filter and other driver methods (char device or network device callbacks) 2) ithread and filter can already be executed simultaneously even when only FILTER_SCHEDULE_THREAD is returned: when ithread is scheduled to be executed the device can emit a new interrupt and it will be preempted by filter > In other words, the part about ORing together values seems to be staking > out room for future growth, because the current set of flags and the > words about how to use them imply that only one of the current set of > values should be returned at once. No, the text does not imply that only one of the values is supposed to be returned (where did you see it). See also KASSERT checks in intr_event_handle() -- they clearly show that the intention was to allow FILTER_HANDLED and FILTER_SCHEDULE_THREAD to be returned simultaneously. Max