Re: FILTER_SCHEDULE_THREAD is not a bit-value

On Mon, 2012-01-30 at 22:50 +0600, Max Khon wrote:
> Hello!
> 
> sys/bus.h documents the following semantics for FILTER_SCHEDULE_THREAD:
> 
> /**
>  * _at_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.

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.

On the other hand, the words are also self-contradictory, in that they
say "oring together zero or more" but then later when saying which flags
can be used together it's defined as erronious to return zero.

-- Ian