FILTER_SCHEDULE_THREAD is not a bit-value

[Max Khon]

Hello,

On Tue, Jan 31, 2012 at 12:44 AM, Ian Lepore
<freebsd at damnhippie.dyndns.org> wrote:

>> sys/bus.h documents the following semantics for FILTER_SCHEDULE_THREAD:
>>
>> /**
>>  * @brief Driver interrupt filter return values
>>  *
>>  * If a driver provides an interrupt filter routine it must return an
>>  * integer consisting of oring together zero or more of the following
>>                                  ^^^^^^^
>>  * flags:
>>  *
>>  *      FILTER_STRAY    - this device did not trigger the interrupt
>>  *      FILTER_HANDLED  - the interrupt has been fully handled and can be EOId
>>  *      FILTER_SCHEDULE_THREAD - the threaded interrupt handler should be
>>  *                        scheduled to execute
>>  *
>>  * If the driver does not provide a filter, then the interrupt code will
>>  * act is if the filter had returned FILTER_SCHEDULE_THREAD.  Note that it
>>  * is illegal to specify any other flag with FILTER_STRAY and that it is
>>  * illegal to not specify either of FILTER_HANDLED or FILTER_SCHEDULE_THREAD
>>  * if FILTER_STRAY is not specified.
>>  */
>> #define FILTER_STRAY            0x01
>> #define FILTER_HANDLED          0x02
>> #define FILTER_SCHEDULE_THREAD  0x04
>>
>> But actually FILTER_SCHEDULE_THREAD is not used as a bit-value (see
>> kern/kern_intr.c):
>>
>>                 if (!thread) {
>>                         if (ret == FILTER_SCHEDULE_THREAD)
>>                                 thread = 1;
>>                 }
>>
>> 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".  If you EOI in the primary interrupt context and then schedule
> 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