Add architectural support for shared interrupts #61331

LaurentiuM1234 · 2023-08-09T10:55:50Z

This series of patches introduces architectural support for shared interrupts.

This works by keeping track of all of the ISR/arg pairs passed to z_isr_install() and calling
them in our own "dummy" ISR.

In theory, this patch series shouldn't affect the current architecture as this is an opt-in option and the series only contains additions and no modifications of current code.

Currently, there's a weird dependency on the ARM64 architecture. This is because I was only able to test this change on said architecture.

For the associated issue please see: #61278.

This commit adds the "__weak" adnotation to the z_isr_install function. The reason for this is because the z_isr_install symbol needs to be overwritten when enabling support for shared interrupts. Signed-off-by: Laurentiu Mihalcea <[email protected]>

This commit introduces the support for shared interrupts. This works by "hijacking" the ISR passed to z_isr_install() and installing our own "dummy" ISR, which, in turn, calls all of the ISRs registered by the clients. Since the support is based on overwriting z_isr_install(), this means we'll have a dependency on CONFIG_DYNAMIC_INTERRUPTS. Signed-off-by: Laurentiu Mihalcea <[email protected]>

With the support for shared interrupts, we need a way to unregister pairs of ISR/arg. This is because we don't want to keep piling up new clients as this will lead to reaching the limit of clients. This feature is only needed and supported in the case of shared interrupts. Signed-off-by: Laurentiu Mihalcea <[email protected]>

LaurentiuM1234 · 2023-08-09T12:12:12Z

CI is green, removing this from draft.

CC: @dbaluta @iuliana-prodan

andyross

Some notes. Seems pretty reasonable on the whole, though IIRC there was another PR supporting IRQ sharing kicking around a few months back that never merged. Would be good to dig it up and figure out which bits we want from both. IIRC it was attempting the somewhat more complicated case of handling shared handlers for static interrupts too. One thing I do like about this variant is that it doesn't involve an API change; you use the same irq_connect API to register shared interrupts as you do for normal ones.

Regarding the IRQ_CONNECT() interaction: I guess I'm personally fine with this API being limited to runtime, as dynamic IRQs are a commonly supported feature and relatively lightweight. It's not a hardship. But you do have to be really clear in the docs (which are a little thin) that IRQ_CONNECT() gives you exactly one interrupt, but that you can come along later and register more.

Also needs a test case for sure.

andyross · 2023-08-09T13:23:27Z

arch/common/CMakeLists.txt

+  zephyr_library_sources_ifdef(
+    CONFIG_SHARED_INTERRUPTS
+    shared_irq.c
+    )


There's some ambiguity, but this belongs in kernel/ not arch/common/ I'd say. The arch layer is responsible for handling interrupts and dispatching to a _sw_isr_table[] entry, but everything downstream is OS behavior and belongs in the kernel.

ACK. I'm assuming the same rationale is applicable to sw_isr_common.c, right?

andyross · 2023-08-09T13:26:32Z

arch/common/shared_irq.c

+	struct k_spinlock lock;
+};
+
+static struct shared_irq_data shared_irq_table[CONFIG_NUM_IRQS];


This is an awfully big table! 8 bytes per record, 16 (default) records per IRQ, and most platforms have a few dozen IRQ slots. At the very least I'd say 16 is way too generous for a MAX_NUM_CLIENTS default. The much more typical number of drivers forced to share an interrupt is "two". :)

Actually, you'd pay for it with a little code size, but one clever way to do this would be to put a dlist node into the structs and keep them in lists, "allocating" them pool-style (i.e. walk the array linearly to start, keep deleted records in a free list, and allocate from there when the original N have been exhausted). Then you'd only need one record (albeit one of 16 bytes instead of 8) for each shared handler of any IRQ in the system.

andyross · 2023-08-09T13:28:30Z

arch/common/shared_irq.c

+		_sw_isr_table[table_idx].arg = irq_data;
+		_sw_isr_table[table_idx].isr = shared_irq_isr;
+
+		irq_data->enabled = true;


(Never mind: this is me being confused as mentioned in the comment below, I just forgot to delete the comment)

This seems wrong? If you call this function on a disabled IRQ it'll silently drop the new function you registered and continue using whatever it was already there? Dynamic interrupts are dynamic, the whole point of the API is that you might want to change handlers at runtime.

andyross · 2023-08-09T13:31:52Z

arch/common/shared_irq.c

+
+	key = k_spin_lock(&irq_data->lock);
+
+	if (!irq_data->enabled) {


Just test if(_sw_isr_table[table_idx].isr == shared_irq_isr) instead and save the space in the irq_data struct. Actually IMHO having it present and calling it "enabled" is a net loss, because I read that as "interrupt line is enabled" and not "shared interrupts are enabled on this line" and was really confused about what was happening here.

andyross · 2023-08-09T13:33:49Z

arch/common/shared_irq.c

+	 */
+	for (i = client_idx; i <= (int)irq_data->client_num - 2; i++)
+		swap_client_data(&irq_data->clients[i],
+				 &irq_data->clients[i + 1]);


Might be worth expanding this handling a bit to detect the case where there's only one handler registered and "unshare" the IRQ.

andyross · 2023-08-09T13:36:42Z

include/zephyr/irq.h

+static inline int
+irq_disconnect_dynamic(unsigned int irq, unsigned int priority,
+		    void (*routine)(const void *parameter),
+		    const void *parameter, uint32_t flags)


Nitpick: would be cleaner to put the implementation for arch_irq_disconnect_dynamic() into this patch. Right now all the important stuff is added in a generic patch, but the patch that says it's adding irq_disconnect is actually only adding a wrapper.

Good point, will fix. Thanks!

LaurentiuM1234 · 2023-08-10T11:45:08Z

@andyross TYVM for all of your input !

Currently, I'm taking a look at #57146. I'm going to try to find a way to allow us to make use of the shared interrupts feature using IRQ_CONNECT() as well. I'm thinking a combination of this patch (which handles the shared interrupts "dynamically" ) and the one I've mentioned above (which handles the shared interrupts "statically"). When I have something somewhat decent I will submit a PR as an alternative to this one.

If that doesn't work, I'll come back to this PR and address all of your suggestions.

As I'm going to be away starting from next week, this might take a while :( (ETA 2 weeks or so).

EDIT: submitted alternative to this at #61422

LaurentiuM1234 · 2023-09-14T06:45:06Z

Feature implemented in #61422.

LaurentiuM1234 force-pushed the shared_irqs branch 2 times, most recently from fe40c6c to 921d4be Compare August 9, 2023 11:28

LaurentiuM1234 added 2 commits August 9, 2023 14:28

LaurentiuM1234 marked this pull request as ready for review August 9, 2023 12:12

LaurentiuM1234 requested review from dcpleung, nashif, andyross, carlescufi, galak, MaureenHelm and ioannisg as code owners August 9, 2023 12:12

zephyrbot added area: Base OS Base OS Library (lib/os) area: Architectures labels Aug 9, 2023

andyross reviewed Aug 9, 2023

View reviewed changes

gmarull requested a review from carlocaione August 11, 2023 09:18

LaurentiuM1234 mentioned this pull request Aug 11, 2023

Add architectural support for shared interrupts (take 2) #61422

Merged

soburi mentioned this pull request Aug 21, 2023

rpi_pico: Added Generic PIO support and a PIO based UART driver #56678

Closed

LaurentiuM1234 closed this Sep 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add architectural support for shared interrupts #61331

Add architectural support for shared interrupts #61331

LaurentiuM1234 commented Aug 9, 2023 •

edited

Loading

LaurentiuM1234 commented Aug 9, 2023

andyross left a comment

andyross Aug 9, 2023

LaurentiuM1234 Aug 10, 2023

andyross Aug 9, 2023

andyross Aug 9, 2023

andyross Aug 9, 2023 •

edited

Loading

andyross Aug 9, 2023

andyross Aug 9, 2023

andyross Aug 9, 2023

LaurentiuM1234 Aug 10, 2023

LaurentiuM1234 commented Aug 10, 2023 •

edited

Loading

LaurentiuM1234 commented Sep 14, 2023


		key = k_spin_lock(&irq_data->lock);

		if (!irq_data->enabled) {

Add architectural support for shared interrupts #61331

Add architectural support for shared interrupts #61331

Conversation

LaurentiuM1234 commented Aug 9, 2023 • edited Loading

LaurentiuM1234 commented Aug 9, 2023

andyross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyross Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurentiuM1234 commented Aug 10, 2023 • edited Loading

LaurentiuM1234 commented Sep 14, 2023

LaurentiuM1234 commented Aug 9, 2023 •

edited

Loading

andyross Aug 9, 2023 •

edited

Loading

LaurentiuM1234 commented Aug 10, 2023 •

edited

Loading