New ARM interrupt framework

[• ian]

Summary

The new more general framework deals mainly with:

• interrupt consumer which does not lay in its interrupt controller bus sub-tree
• cascaded interrupt controllers
• IPI setup

The design is quite general and can be utilized by big variety of controllers.

[andrew]

sys/arm/arm/intrng.c

18 ↗

(On Diff #4182)

Is this still the case, or has this code been rewritten so this clause could be removed?

sys/arm/arm/mp_machdep.c

213 ↗

(On Diff #4182)

Where has this moved to in the not-intrng case?

sys/dev/fdt/simplebus.c

334 ↗

(On Diff #4182)

I think this is wrong. There is nothing simplebus specific about this, we should add support for hooking into resource_list_print_type for a given resource type. Not all interrupts will come from simplebus, e.g. I'm planning on using this for arm64 where we will have multiple places to get the irq from.

[skra]

sys/arm/arm/intrng.c

18 ↗

(On Diff #4182)

Yes. I will replace it with standard FreeBSD copyright.

sys/arm/arm/mp_machdep.c

213 ↗

(On Diff #4182)

My bad. I will return it for not-intrng case.

sys/dev/fdt/simplebus.c

334 ↗

(On Diff #4182)

I do not like it too. However, it was taken from intrng project as is. I will remove this simplebus change and associated ones. It will have no impact for usage now. In time when intrng become full standard, I'm sure that some correct solution will be ready.

[skra]

Update to address suggestions.

[• ian]

Commandeer, because apparently that's the only way a collaborator is allowed to update the diff.

[• ian]

Update the diff so it applies to -current as of r286786, 2015/08/16.

[• ian]

sys/arm/arm/intrng.c

823 ↗

(On Diff #7984)

need a NULL check here due to M_NOWAIT, caller already expects potential NULL return from this routine.

sys/arm/include/intr.h

52 ↗

(On Diff #7984)

I think we need a comment to say what INTR_SOLO means, or what it can and can't be used for. (It seems to be a flag that only a PIC can use?)

128 ↗

(On Diff #7984)

another flag that needs a comment, I can't figure out why you would use this one from looking at the code.

[• ian]

sys/arm/arm/intrng.c

589 ↗

(On Diff #7984)

It seems strange to me that the PIC_REGISTER() method is responsible for filling out all sorts of information in the isrc struct, but it returns is_percpu via a pointer arg and then we set the ISRCF_PERCPU flag in the struct. Why not have PIC_REGISTER() set this flag along with all the other stuff it sets and eliminate the pointer arg?

sys/arm/arm/pic_if.m

95 ↗

(On Diff #7984)

The difference between en/disable_intr() and en/disable_source() is a bit hard to figure out. I think I've convinced myself that enable/disable_intr correspond to setup_/teardown_intr(), so maybe we should use those terms to make it clear.

[skra]

More info and ideas related to intrng can be found here -> start
The last email in this mailing thread is here -> last

sys/arm/arm/intrng.c

589 ↗

(On Diff #7984)

It's a thing of locking strategy. PIC_(UN)REGISTER() is called without isrc_table_lock locked to give pic more space to do what it needs. However, isrc_flags needs to be changed atomically. At the beginning, I wanted to keep isrc struct opaque for controllers. It turned out to be hard (and pointless) to accomplish. (Nevertheless, note that pic is setting only things strictly related to it.) However, I'd still like to keep control over isrc_table_lock in intrng only, so it could seem to be strange. On the other hand, this register/unregister thing is slightly outdated together with arm_fdt_map_irq() and arm_namespace_map_irq(). Note also that PIC_UNREGISTER() is used nowhere now.

Our new view to this (un)mapping/(un)registering problem is that it should be done during bus_alloc_resource() and bus_release_resource() calls with help of data saved in resource struct. Something like data type, pointer, and size + flag that resource is mapped. This way isrc struct will be more stable and free of various additional data asociated with a way how an interrupt is described. And struct resource is good candidate for that as it's being already allocated. An isrc struct will be allocated only once if an interrupt will be described by two or more ways. This is not true now and it's not correct! Only interrupt controller itself can decide that two types of interrupt description are same. And associated interrupt controller must be initialized at the time when these bus methods are called.

Further, together with thoughts about removable controllers, maybe some per pic lock is on air.

823 ↗

(On Diff #7984)

Well, this function was changed a few times and the result is inconsistent. M_NOWAIT is used as it's called under mtx lock. However, considering loadable controllers, it's probably legitimate that this function should return NULL. Thus, pic should be always malloc'ed before acquiring the lock and free'ed if such pic already existed. Hmm, wait, it's used only in arm_pic_register() which should be called only once for a controller and a controller is the caller. So, it should be an error to try creating same pic twice.

sys/arm/arm/pic_if.m

95 ↗

(On Diff #7984)

It's i386 terminology. These en/disable_intr() are called to tell a controller to prepare everything needed for an interrupt which is going to be used/unused. Some controller can be complex and more job could be needed to do that. Furher, interrupt configuration (level, edge, ..) and binding should be done in enable_intr(). On the other hand, these en/disable_source() are more about light work to mask and unmask an interrupt source.

Hmm, we have specialized PRE_ITHREAD(), POST_ITHREAD(), and POST_FILTER() methods which i386 interrupt framework does not have. Thus, enable_intr() with enable_source() and disable_source() with disable_intr() go in a pair except arm_irq_dispatch(). While i386 uses en/disable_source() separately in its framework.

On the other hand, some new bus methods for mask/unmask an interrupt are scheduled which should use en/disable_source() separately too.

BTW, it's terrible terminology for me. However, all new terminology I think of is even more terrible. ;)

sys/arm/include/intr.h

52 ↗

(On Diff #7984)

I agree. The INTR_SOLO logic is generalized, so it can be used for any interrupt which is served by one and only filter and no handler. It's almost a need for cascaded controllers to avoid all MI interrupt framework overhead and to be as fast as possible. And for same reason it could be used for any interrupt which requests same. It brings one more advantage in case of cascaded controllers. The SOLO filter has one more argument - trapframe - so it can be passed to all controllers in line easily.

128 ↗

(On Diff #7984)

Well, it's because it's not utilized yet. The idea is that some platforms can have different methods how to send ipi - fast interrupt for example. Nevertheless, dispatching will be always same - some ipi handler will be called. Thus this flag says - register my handler so I can use arm_ipi_dispatch(), but do not alloc any ipi on root interrupt controller as I will send ipi different way.

[• ian]

sys/arm/arm/intrng.c

1374 ↗

(On Diff #7984)

Turns out this causes an error...

Release APs
timeout stopping cpus
panic: mtx_lock() by idle thread 0xc6c5aa20 on sleep mutex arm isrc table @ /local/build/staging/freebsd/wand/src/sys/arm/arm/intrng.c:1374

[skra]

sys/arm/arm/intrng.c

1374 ↗

(On Diff #7984)

Hmm, tested on two-core pandaboard, so never in race with other secondary cores. And arm_isrc_table is sleep mutex, so the panic. I have no quick solution here as isrc table must be traversed in PIC and there could be other races without lock. And change the mutex to spin one is too big hammer for this problem. Maybe somehow serialize the call among secondary cores?

[skra]

sys/arm/arm/intrng.c

1374 ↗

(On Diff #7984)

Well, local PIC isrc table is traversed, but arm_isrc_table mutex must be used as settings on isrc is evaluated.

[skra]

sys/arm/arm/intrng.c

1374 ↗

(On Diff #7984)

After small talk with Michal, the problem is more complex here and init_secondary() in mp_machdep.c should be reworked. PIC init secondary should only init hardware. Interrupt enabling should be done lately after corresponding hardware is inited on secondary cores too. Thus the lock won't be needed here after that.

BTW, some part of init_secondary() is already serialized while core 0 is waiting. However, it's out of question now.

[bsz_semihalf.com]

head/sys/arm/arm/gic.c

787

It doesn't compile without SMP enabled, because arm_gic_bind is in #if defined(SMP) block. I'm not sure what is the right way to fix it, perhaps adding ifdef here?

[skra]

head/sys/arm/arm/gic.c

787

It could depend on on two things: (1) to which core are interrupts routed after gic reset and (2) on which core is kernel running in not SMP case. However, to not depend on such things, correct fix is to take out arm_gic_bind() from #ifdef SMP section as arm_irq_next_cpu() deals already with it. (It returns current cpu in not SMP case.)

[• ian]

head/sys/arm/arm/gic.c

787

I moved it out of the #ifdef SMP block and commited it.

[ehem_freebsd_m5p.com]

head/sys/arm/arm/intrng.c

399

And now more than 5 years down the road this line got noticed. This enhances performance by causing an early ENOSPC return, but requires irq_next_free to be reset when an interrupt is freed. Unfortunately that is presently missing so if this triggers, all future allocations will fail even if one is freed. I suspect this isn't very common as this managed to remain unnoticed for 5 years, but it might account for an occasional mysterious panic.

D29310 is for fixing this.