Page MenuHomeFreeBSD

xen/intr: add check for intr_register_source() errors
Needs ReviewPublic

Authored by on Sep 16 2021, 10:53 PM.



While unusual, intr_register_source() can return failure. A likely
cause might be another device grabbing from Xen's interrupt range.
This should NOT happen, but could happen due to a bug. As such check
for this and fail if it occurs.

This also covers the condition identified by the KASSERT(), so remove
the now redundant KASSERT().

Diff Detail

rS FreeBSD src repository - subversion
Lint OK
No Unit Test Coverage
Build Status
Buildable 45559
Build 42447: arc lint + arc unit

Event Timeline

This really doesn't fit with D30743. This one is checking for this one situation which should never occur, but reality is perverse.

Most likely trigger is intr_register_source() does a check for intr_lookup_source(vector) and will return EEXIST for the situation identical to the KASSERT(). This though would require something else to grab an interrupt from the Xen code's range; which in turn shouldn't happen.

This doesn't work as part of D30743, since this isn't focused on xen_intr_port_to_isrc[]. This is simply a check which is really needed. Isn't needed as part of the Xen/ARM64 project, just my entire patch series includes this. This shouldn't occur, but safety checks are for the conditions we don't expect to occur.

Looks fine to me. I would suggest keeping the KASSERT though, it is free and makes the point of failure more obvious if this condition did occur.

This revision is now accepted and ready to land.Sep 17 2021, 12:27 PM

On review, at HEAD the logic is inverted. When returning with NULL, the lock needs to be held. Updating to match actual situation.

This revision now requires review to proceed.Oct 23 2021, 8:56 PM

If you're wondering about D30726 referencing the call structures being perverse, this is one of the spots. Alternatively due to commit reordering this got reordered before D30726 which flips the logic. This is what should be done if before D30726.

I prefer pushing D31995 after D30726. That way the original (approved) form can be used and results in some later deltas being smaller. I don't like leaving mines lying around though.

Updating due to concern which arose. If this occurs another spot will need to deal with it. OTOH should this in fact be a panic() since it shouldn't occur?

This is mostly getting handled elsewhere, but update the Phabricator copy.