Page MenuHomeFreeBSD

Reduce the dependency on NIRQ
AbandonedPublic

Authored by andrew on May 17 2016, 4:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 11 2024, 8:35 PM
Unknown Object (File)
Dec 20 2023, 1:16 AM
Unknown Object (File)
Oct 18 2023, 4:44 PM
Unknown Object (File)
Oct 12 2023, 11:27 AM
Unknown Object (File)
Aug 14 2023, 10:29 AM
Unknown Object (File)
Jul 28 2023, 10:12 PM
Unknown Object (File)
Jun 30 2023, 6:56 AM
Unknown Object (File)
Jun 24 2023, 11:46 PM

Details

Reviewers
manu
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 - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3791
Build 3834: arc lint + arc unit

Event Timeline

andrew retitled this revision from to Reduce the dependency on NIRQ.
andrew updated this object.
andrew edited the test plan for this revision. (Show Details)
andrew added reviewers: ARM, arm64.
andrew added a subscriber: emaste.

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

Fix the pic_sources index in isrc_alloc_irq

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
269

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
219

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 inline comments.
sys/arm64/arm64/gic_v3_fdt.c
142

We will have separate intr_pic_register() for ITS correct?

andrew marked 2 inline comments as done.

Update based on feedback

sys/arm/allwinner/a10/a10_intc.c
269

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
219

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
142

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?

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.