Project

General

Profile

Actions

Bug #14966

open

ddi_intr_set_cap() allowed flags inconsistency

Added by Thirteen Oxide 3 months ago.

Status:
New
Priority:
Low
Assignee:
-
Category:
-
Start date:
Due date:
% Done:

0%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:
External Bug:

Description

Neither the introduction of the ddi_intr_set_cap(9f) interface via PSARC 2004/253 nor the current contents of the illumos manual clearly specify the semantics of this interface with respect to the set of capability flags that may be passed in. There is a comment in the implementation of this function that does not match the actual behaviour:

        /* Only DDI_INTR_FLAG_LEVEL or DDI_INTR_FLAG_EDGE are allowed */
        if (!(flags & (DDI_INTR_FLAG_EDGE | DDI_INTR_FLAG_LEVEL))) {
                DDI_INTR_APIDBG((CE_CONT, "%s%d: only LEVEL or EDGE capability " 
                    "can be set\n", ddi_driver_name(hdlp->ih_dip),
                    ddi_get_instance(hdlp->ih_dip)));
                rw_exit(&hdlp->ih_rwlock);
                return (DDI_EINVAL);
        }

All sources, then, are agreed that either EDGE or LEVEL must be set in the flags that are passed in, and that it is possible to invoke this only on a device for which ddi_intr_get_cap(9f) returns a mask that includes the trigger type to be set.

Unfortunately, the actual behaviour is such that:
  1. Other flags may also be set, and are passed through to the implementing nexus even if they are invalid. This may have been intended to allow for nexus-private flags, but this is not mentioned in the original ARC case nor was any namespace reserved for this purpose.
  2. It is possible for the caller to set both the EDGE and LEVEL flags simultaneously, which has no valid interpretation.
  3. It is impossible to set the trigger type if the type requested is the only one supported by the implementing nexus.

Some of these semantics suggest that it was intended for a consumer to first call ddi_intr_get_cap(9f), then pass the resulting capabilities flags to ddi_intr_set_cap(9f) after modifying it to select the desired trigger type; however, the implementation does not reliably allow this, either. There are no in-gate consumers to guide or constrain us, and it is likely that this interface has never been used.

The most reasonable semantics would seem to be either:

  1. Constrain the flags argument to exactly one of DDI_INTR_FLAGS_EDGE or DDI_INTR_FLAGS_LEVEL and nothing else, provided that the requested trigger would have been returned by ddi_intr_get_cap(9f) when invoked with the same handle. This could also allow future expansion to include additional flags that are not mutually exclusive and documented to be read-write.
  2. Allow any number of read-only flags to be passed in, in addition to any valid combination of read-write flags, provided that the set of read-only flags does not exceed the set of read-only flags that would be returned by ddi_intr_get_cap(9f) when invoked with the same handle (i.e., mask off all the read-only capabilities associated with this handle prior to evaluating the caller's argument, and never pass read-only capabilities to the nexus). This also allows for future expansion in the same manner as (1).
  3. As above, but pass the caller-supplied set of read-only capabilities to the nexus.

If the set of read-only flags does not conform, DDI_EINVAL should be returned. I would suggest that option (2) or (3) offers greater flexibility, in that callers may then perform a single call to ddi_intr_get_cap(9f), mask out all RW flags, set the desired trigger mode (and any future RW flags desired), then call ddi_intr_set_cap(9f) with the resulting flags. Option (3) further allows for future expansion such that a capability may be read-only for some nexi and read-write for others. Any option seems preferable to what we're doing today, however. Regardless of which option we prefer, we also need to decide what combinations of EDGE and LEVEL are allowed. The choices here are:

  1. Exactly one or the other is allowed, iff both are included in the reported capabilities. This is most similar to existing behaviour.
  2. At most one is allowed, and if one is set it must be among the reported capabilities. A request with neither set is a nop.

In either case, if both are set, the result should be DDI_EINVAL. For case (1), setting neither flag should result in DDI_EINVAL; it's less obvious what should happen if the caller requests the sole supported trigger mode -- today that results in DDI_ENOTSUP which seems surprising to say the least. For that reason I would suggest we implement case (2) instead, and return DDI_ENOTSUP only if the requested trigger mode is actually unsupported or the implementation returns that error code.

There is one other option: we could remove this interface entirely. That seems unwise because it is possible that a third-party driver somewhere uses it and it is a Committed interface. That said, the semantics are so poorly-defined and of so little real utility that it's unlikely. Note that the choices I have recommended should allow every useful invocation that currently succeeds to continue doing so with the same effects that result today. Regardless of whether we change the semantics at all (and we should), the comment in `ddi_intr_set_cap()` should be fixed to reflect what they actually are.

No data to display

Actions

Also available in: Atom PDF