Pass interrupt controller drivers their intr_pic pointer

[andrew]

Summary

Return the intr_pic pointer from intr_pic_register and have the irq
driver pass this back into other intr_pic_* functions when we would
have passed in a device_t, xref pair to use to lookup the pic.

[andrew]

Only add intr_pic to the softc when building for intrng

[skra]

sys/sys/intr.h

118

These functions are an interface for PICs (interrupt controller drivers), so the change is possible for them. From one point of view, it could be better to use struct pic pointer as an argument once a driver is registered by intr_pic_register(). The cost is only that each PIC driver must store its struct pic pointer. On the other hand, xref may be 0 in many drivers, so storing struct pic pointer may be pointless. These functions should not be called on time critical path and each of them only once for a driver instance. Further, most of the PICs will never call intr_pic_deregister() and intr_pic_claim_root() is called only on root PIC.

When I have thought more about that now, the xref argument is redundant in these functions. The device pointer is the thing which is important. So I suggest to change the interface this way:

int intr_pic_register(device_t);
int intr_pic_deregister(device_t);
int intr_pic_claim_root(device_t, intr_irq_filter_t *, void *, u_int);

And add one more function which adds an xref for a device.

int intr_pic_add_xref(device_t, intptr_t);

It also perfectly matches the plan for changing the way how struct pic list is implemented. I wrote about that in D5985.

123

This function is an interface for BUS drivers. So the change is not possible here as only a PIC driver could have a struct pic pointer.

I'm terribly sorry that the function is now called in bad context so it looks that the change would be okay. It's temporary solution before FDT BUS drivers will be patched for INTRNG. Further, I hope that Michal will create correct use pattern soon now, when struct intr_map_data was redefined.

[andrew]

sys/sys/intr.h

118

That will still need a lookup to find the intr_pic. By passing it in from the driver there is no need to call pic_lookup saving a lock and having to walk the linked list. I think adding 4 or 8 bytes to a softc isn't too much to reduce the need to walk the list.

I'm also planning on reworking the MSI change to also return a struct intr_pic * from intr_msi_register and passing it in when allocating/releasing/mapping MSI/MSI-X interrupts.

[andrew]

Don't change intr_map_irq

[skra]

sys/sys/intr.h

118

It's true only for intr_pic_claim_root(). But this function is called just once. Locking of the list lock cannot be avoided in case of intr_pic_deregister() as the struct pic must be removed from the list. And as long as a single linked list is used, the list must be traversed to find the place where a list entry is enqueued.

If you have not another reason why you need struct pic pointer in a PIC driver, I think that it's not necessary to save struct pic pointer in a PIC driver. Moreover when I suggest to remove xref argument from these functions. Maybe, if there will be hundreds of PICs in a system and the functions will be called frequently, a double linked list would take a place. But even then, either avoiding of one fast lock or adding 4 (or 8) bytes to each PIC will be a matter of various views.

How many PICs are you expecting in a system? If there will be 10 PICs there, the time needed for traversing of the list is so short compared to other things like a number of strcmp() called in FDT case for example.

BTW, I hope that you will define struct intr_msi for MSI interface to not confuse consumers. On the other hand, when you look at definition of struct pic, it's only a list entry keeping device_t and its xref. There is nothing more there now. So, struct pic could be renamed to something more general and used for any list which keeps information about devices and its xrefs. That is the plan noted in D5985.

However, there is also a plan to add a lock to each struct pic and used it instead of isrc_table_lock when PIC methods are called. This way controllers like i2c ones may sleep during pic_setup_intr() like functions and they will not block other controllers. Thus struct pic will be needed truly. And the general list entry must also keep a void * pointing to corresponding object.