Page MenuHomeFreeBSD

Reduce the dependency on NIRQ
Needs ReviewPublic

Authored by andrew on May 17 2016, 4:40 PM.

Details

Reviewers
None
Group Reviewers
ARM
arm64
Summary

Remove a fixed interrupt list size, and replace it with a per-PIC dynamic
sized array. The size is determined when the pic attaches any may probe
the hardware for the information.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3807
Build 3850: arc lint + arc unit

Event Timeline

andrew retitled this revision from to Reduce the dependency on NIRQ.May 17 2016, 4:40 PM
andrew updated this object.
andrew edited the test plan for this revision. (Show Details)
andrew added reviewers: ARM, arm64.
andrew added a subscriber: emaste.
andrew updated this revision to Diff 16477.

I still need to fix tegra_lic.c, but am unsure how many interrupts this controller supports.

andrew updated this revision to Diff 16478.May 17 2016, 4:54 PM

Fix the pic_sources index in isrc_alloc_irq

skra added a subscriber: skra.May 17 2016, 9:56 PM

In general, I'm not able decide now if I like or dislike this change. For the mention reasons of this change, dynamic reallocation of global irq table would be enough. Note that INTRCNT_COUNT depends on NIRQ too. So, definitely, dynamic allocation of irq counters (intrcnt and intrnames arrays) should be solved too and IMO before. For example, the counter could be a part of struct intr_isrc and the arrays could be filled on request.

sys/arm/allwinner/a10/a10_intc.c
272–273

Should not intr_pic_register() be called before intr_isrc_register() in your patch? If yes, it would be okay now to return struct pic* from intr_pic_register() and use it as argument for intr_isrc_register() (and other functions) instead of device_t. And I would make it the first argument. The device_t is stored in struct pic* then. This note is valid for all PICs.

BTW, I was thinking about adding max_count argument to intr_pic_register() to make intr_isrc_register() error safe. I did not do that to not limit INTRNG usability. But probably, you would like to register ISRC dynamically in MSI/MSI-X case?

sys/arm/ti/omap4/omap4_wugen.c
218

This is pass-through PIC, so I suppose that no irq range is needed. And allocating some could be quite wrong.

sys/kern/subr_intr.c
433

struct pic* argument instead of device_t?

440

isrc->isrc_dev is initialized in this function on line 445.

445

pic->pic_dev?

474

add struct pic* argument?

1026

Maybe not needed at all?

1538

Don't use i but isrc->isrc_irq or pic->pic_irq_base + i.

zbb added a subscriber: zbb.May 18 2016, 10:46 AM
zbb added inline comments.
sys/arm64/arm64/gic_v3_fdt.c
139–140

We will have separate intr_pic_register() for ITS correct?

andrew marked 2 inline comments as done.May 18 2016, 1:13 PM
andrew updated this revision to Diff 16525.

Update based on feedback

andrew added inline comments.May 18 2016, 1:31 PM
sys/arm/allwinner/a10/a10_intc.c
272–273

I would have picked up on this, however I didn't notice I booted the wrong kernel.

I would like to allow us to handle thousands of MSI/MSI-X interrupts as I could imagine servers where this will be the case, e.g. on ThunderX most of the internal devices are on the end of an internal PCI bus. The ThunderX can also be used in a dual package configuration. Add to this the external PCIe bus and the interrupt count can quickly add up.

sys/arm/ti/omap4/omap4_wugen.c
218

Yes, I think so. I also think this driver only needs to implement the pic_map_intr function as the other pic_ functions will all use the parent interrupt controller in subr_intr.c.

sys/arm64/arm64/gic_v3_fdt.c
139–140

Yes, this is part of the reason for this patch, to allow for the extra irqs we could get from ITS.

sys/kern/subr_intr.c
433

Can I hold off changing this ABI for now & update D6304 on top of this? I have other needs for a driver to know its struct intr_pic *.

445

Should be the same?

mmel added a subscriber: mmel.May 19 2016, 6:03 AM

My main problem with this patch is that it has changed allocation of interrupt sources numbers.
Previously, the system had arbitrary limited number of IRQS, but we could make an infinite number of pic_register() / pic_deregister().
Now, interrupt source numbers are allocated only in forward, by blocks, without overflow check.
For me, the first case is better. In worst case, we can easily convert now static irq_sources table to dynamically allocated/reallocated one.

Second problem is that changed calling order of intr_pic_register() and intr_isrc_register() creates race – the PIC become visible for consumers
after intr_pic_register() call, but its only half initialized at this time. This is also one of reasons why pointer to PIC is not a part of intr_irqsrc structure.
This patch also added next new limitation to INTRNG, using ‘pic_lookup(dev, 0);’ assumes that one device can implement only one PIC,
the original code doesn’t have this assumption.

Also locking looks slightly broken. I think that pic table lock must be help between pic_lookup() and isrc action, but this can be easily fixed later.

Personally, I vote NO for this patch – the problem, unlimited (dynamic) number of interrupt sources, can be easily
solved by dynamic allocation of irq_sources table without adding additional limitations to current INTRNG framework.