Rather than joining the paramters together inside xen_intr_bind_isrc(),
there is some sense in using a temporary on-stack structure. While low
value at this point, this aids later commits.
This is what I was suggesting in D30936. Doesn't add much by itself, but seems plausible as a potential API. This has a distinct advantage for aarch64 as it could use "xen-virq%u", isrc->xi_virq for the intr_event_create() call in case of VIRQ. This seems better for VIRQs since they could get reassigned to different ports.
I distinctly like this approach in some ways. Makes the xen_intr_alloc_isrc() prototype simpler and ensures everything potentially needed gets in. This could also simplify a number of error cases as it becomes valid to use xen_intr_release_isrc() for cleanup sooner. Downside I see right now is the temporary struct xenisrc could use a substantial amount of stack.
Update due to examining elsewhere and noticing a crucial missing piece. I really do find this approach elegant and I suspect it could simplify things. Do need to choose the approach for the issue.
Reverting to previous arrangement. The extra bits didn't seem to go with this delta that well. Trying to grab the full additions would have been too confusing. Do include the reflowing though.
In case comments elsewhere didn't make it obvious, I really like this interface. Yet since it isn't required I continue to keep options open. Getting a decision sooner is highly valuable since this choice effects many follow-on commits substantially.
|310–311 ↗||(On Diff #98342)|
Clang gave an "expected expression" error at the _Static_assert() without the semicolon. This seems like a compiler bug, as it doesn't complain with the exact same CTASSERT() directly after a bracket. If someone has a better explanation I'd like to learn of it.
I have looked through the series (D31188, D33622, D33623, D33624 and D31355), but what's missing is the "why" of all these changes. Does this refactoring enable something specific? IMO the new code is not obviously better than what was present before, just different.
Since none of them apply against main, they are not immediately actionable either.
They may not directly on HEAD since this is a bit into the chain.
The Real question I've been aiming to get answered here is: What should the MI <-> MD interface for isrc allocation look like? D30936 has been tried and it works acceptably. Could this be improved? Likely, issue is what does a better interface look like?
What I've been trying to get feedback on for 6 months is, might a prototype of struct xenisrc *xen_arch_intr_alloc(const struct xenisrc *const params); be better? The advantage is this provides the MD side with more data for setting interrupt sources up.
Meanwhile the rest of the series is pretty much exploring what passing struct xenisrc *const params to xen_intr_bind_isrc() allows. The extras are merging functionality duplicated in several places together.
Many of xen_intr_bind_isrc()'s callers close the event channel in case of failure. The additional data passed in (xi_close) allows that to be handled as common code.
D32866 is yet more functionality with 3 duplicate implementations. Worse, they've drifted apart as at least one piece of functionality was lost at ca7af67ac958 (xen_intr_alloc_and_bind_ipi() used to call intr_event_bind()).
The reduction in lines of code is small, but should have some value.