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.
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.
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?
This is pass-through PIC, so I suppose that no irq range is needed. And allocating some could be quite wrong.
struct pic* argument instead of device_t?
isrc->isrc_dev is initialized in this function on line 445.
add struct pic* argument?
Maybe not needed at all?
Don't use i but isrc->isrc_irq or pic->pic_irq_base + i.
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.
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.
Yes, this is part of the reason for this patch, to allow for the extra irqs we could get from ITS.
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 *.
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.