Rework the logic for finding the pic object.

[andrew]

Summary

In preparation for ACPI on arm64 we need to rework the logic for finding the pic object. This is needed as an ID of 0 may be a valid xref.

For this there is now two new values:

• XREF_INVALID: An invalid xref, used to signal only the dev should be matched.
• XREF_NONE: No xref, used when the interrupt controller has no xref, but should still be registered. Using this to look up the pic will only return a device that also has and xref of XREF_NONE, not just pics where just the device matches.

[skra]

(1) Be more verbose in summary and explain why 0 is not good for unknown xref value. And what 0 xref value will mean if -1 is unknown xref value.
(2) If you need to do cosmetic change which adds flag argument to pic_create(), do it in separate change.

[andrew]

In D8616#178832, @skra wrote:

(1) Be more verbose in summary and explain why 0 is not good for unknown xref value. And what 0 xref value will mean if -1 is unknown xref value.

0 is already a valid MSI xref. Having it as the unknown value was already problematic. With ACPI we will get a second pic with an xref of 0. This patch is needed to both tell them apart, and to be able to search for them.

(2) If you need to do cosmetic change which adds flag argument to pic_create(), do it in separate change.

Adding a flag argument to pic_create is a needed change as it then passes the flag to pic_lookup_locked.

[skra]

In D8616#178836, @andrew wrote:

In D8616#178832, @skra wrote:

(1) Be more verbose in summary and explain why 0 is not good for unknown xref value. And what 0 xref value will mean if -1 is unknown xref value.

0 is already a valid MSI xref. Having it as the unknown value was already problematic. With ACPI we will get a second pic with an xref of 0. This patch is needed to both tell them apart, and to be able to search for them.

(2) If you need to do cosmetic change which adds flag argument to pic_create(), do it in separate change.

Adding a flag argument to pic_create is a needed change as it then passes the flag to pic_lookup_locked.

Well, I missed that you want to lookup a PIC by its type. Nevertheless, can you please split up this change? I mean, in first step, to add flag argument to pic_create() to ensure that the flag is set under PIC lock, and to add flag argument to both pic_lookup() and pic_lookup_locked(). And to explain and document, if a PIC can have both flags FLAG_PIC and FLAG_MSI set, or not. And if there could be two or more PICs registered for the same device - one with FLAG_PIC, another with FLAG_MSI, and if xref values can be same or different. This became somehow not-so-clear after MSI implementation was added, and as INTRNG documentation is missing, I would like to have this documented at least in commit message.

Then, in second step, to introduce -1 as unknown xref value. I have some comments about it, but I do not want to mix it up. So I would like to wait till this change is splitted up. Thank you.

[mmel]

I don't think that you can change 'INVALID' value for xref only locally, without corresponding change in FDT. In many cases, the INTRNG functions are called directly with result from OF_xref_from_node(), they must be able to detect 'INVALID' value from FDT.

You can pass acpi handle throw function, say 'acpi_xref_from_handle()' which compresses/switches zero based acpi handle to any another format.
This way looks significantly easier for me.

Also, I'm curious why you want/need FLAG_PIC at all, everything is now driven by interrupt data type.
The FLAG_MSI is only used as safekeeper (and allow us to use common function for mapping of MSI interrupts).

sys/kern/subr_intr.c

701–704

I thing that KASSERT is not right at this place. pic_lookup can be probably called from intr_alloc_msi() with dev == NULL and xref == invalid, if MSI controller is not registered. Then intr_alloc_msi() fails and card can fallback to old LSI interrupt.

717

Match logic is incorrectly changed here. You cannot expect that PIC itself have valid xref. For example, you may want to create PIC for PCI gpio card.

1307

This function can be called with 'INVALID' xref and it's expected that it return error.

1385

Ditto

[andrew]

Rebase

[andrew]

sys/kern/subr_intr.c

701–704

We shouldn't be calling intr_alloc_msi with an invalid xref. The callers should be checking the msi parent is valid.

717

So we would be searching using a dev and xref with a valid xref that doesn't refer to the dev? That seems broken to begin with, and should be something we don't support.

[andrew]

Add an XREF_INVALID macro

[mmel]

sys/kern/subr_intr.c

702

That's not fully true.

• INTRNG *must* works also in case where you known only device, but not xref (ie for MIPS with hints based config)
• pic_lookup is not called only from intr_alloc_msi
• for msi, you must be able allow fallback from msi to legacy ones - i.e. when you have proper DTS but you haven't implemented MSI interrupt controller.

717

No, but this change returns valid result if someone calls pic_lookup() with valid dev a invalid xref, while pic itself have valid xref.
The whole idea is:

• single device can register multiple pics (with different xrefs)
• if lookup is called without dev, xref is primary identifier
• if lookup is called without xref, only pic without xref can match

The real example is interrupt controller in tegra (not a fully implemented yet) -
single device terminates MSI interrupt, handles legacy LSI and also serve local interrupts
from PCIe core (PCIe error).

Next one is above mentioned PCI gpio card with interrupt support.

[andrew]

sys/kern/subr_intr.c

702

How does this KASSERT break the case where you know a dev but not xref? If the dev is NULL and the xref is not invalid the second case will pass.

I don't see where we break the case where the bus only knows the dev. In these cases it generates an xref for its own use. I suspect we should create a second value for these to use to stop INTRNG from searching for an common xref, but with a NULL dev.

For xref the callers should filter out calls to INTRNG when they don't have an MSI parent, it should be considered to be a bug if they fail to do this so a KASSERT is appropriate.

717

The MSI interrupt would be passed to the MSI struct intr_pic *, while the legacy and error interrupts would be passed to the PIC struct intr_pic *.

For the PCI GPIO case we can use any xref not used by an existing bus. It may pay to create a macro with a safe value for devices to use when only the dev is applicable.

[andrew]

Ping? I'd like to commit this soon.

[skra]

(1) The staff around PIC and xref was implemented with concrete idea. You are trying to change it. So, it's not cleanup, it's an implementation of new idea. And I don't see why this change would be better.
(2) You are forcing devices which do not know (or have) their XREFs to generate some instead of using one and only XREF_UNKNOWN for these cases, which is zero.
(3) XREF_INVALID used the way you are suggesting is a nonsense. I would see XREF_INVALID only as an error code.
(4) This part of INTRNG was implemented strictly according to Michal, so I hope that you do not commit this before Michal agrees with it.

[andrew]

In D8616#210013, @skra wrote:

(1) The staff around PIC and xref was implemented with concrete idea. You are trying to change it. So, it's not cleanup, it's an implementation of new idea. And I don't see why this change would be better.

It might be ok for FDT, however we need to also consider ACPI where zero is a valid xref.

(2) You are forcing devices which do not know (or have) their XREFs to generate some instead of using one and only XREF_UNKNOWN for these cases, which is zero.

But zero may be a valid xref. I would prefer we create a new value for XREF_UNKNOWN separate from XREF_INVALID, e.g. -2. This will be used by devices that don't naturally have an xref, e.g. a PCI GPIO controller, but would allow the error checking in the patch to be kept. I didn't do this in this patch as I'm not aware of any drivers in the tree that would use this so am unable to test it.

(3) XREF_INVALID used the way you are suggesting is a nonsense. I would see XREF_INVALID only as an error code.

We already use errno values in this code so the error code would be EINVAL.

(4) This part of INTRNG was implemented strictly according to Michal, so I hope that you do not commit this before Michal agrees with it.

As far as I know I've answered all questions, and haven't seen any further feedback in a month.

[mmel]

In D8616#210126, @andrew wrote:

In D8616#210013, @skra wrote:

(1) The staff around PIC and xref was implemented with concrete idea. You are trying to change it. So, it's not cleanup, it's an implementation of new idea. And I don't see why this change would be better.

It might be ok for FDT, however we need to also consider ACPI where zero is a valid xref.

(2) You are forcing devices which do not know (or have) their XREFs to generate some instead of using one and only XREF_UNKNOWN for these cases, which is zero.

But zero may be a valid xref. I would prefer we create a new value for XREF_UNKNOWN separate from XREF_INVALID, e.g. -2. This will be used by devices that don't naturally have an xref, e.g. a PCI GPIO controller, but would allow the error checking in the patch to be kept. I didn't do this in this patch as I'm not aware of any drivers in the tree that would use this so am unable to test it.

In short:

• in current code takes 0 (or NULL) xref as unknown xref not as error indicator.
• we cannot expect that all "information sources" *FDT/ACPI/HINTS" can share same value for unknown xref natively
• natural value for unknown xref is 0 for FDT , NULL for HINTS
• if you want change value of unknown xref correctly, then you must also remap FDT no such property to INTRNG unknown xref for all FDT users
• the "#define XREF_INVALID (-1)" is nonsense, it's hidden "#define XREF_INVALID ACPI_INVALID_HANDLE" as these values are coupled

(3) XREF_INVALID used the way you are suggesting is a nonsense. I would see XREF_INVALID only as an error code.

We already use errno values in this code so the error code would be EINVAL.

Where we pass any error code to xref parameter? Ore where we return error code from function returning xref?

(4) This part of INTRNG was implemented strictly according to Michal, so I hope that you do not commit this before Michal agrees with it.

As far as I know I've answered all questions, and haven't seen any further feedback in a month.

Sorry for delay, I'm overloaded these days (or weeks)
And no, I don't think that you answered my questions:

• newly added kasserts limits already working cases. But why?
• you want to change already working pic_lookup() behavior. But why?
• you want to separate MSI PIC and standard PIC, even if the current code can handle this without problem. But why? Moreover the separation is not always possible - both interrupts (legacy and MSI) can be terminated in same PIC.

If all these changes are motivated only by the fact that 0 is valid ACPI handle, then please use recommendation (ignored until now) from my first response.

[mmel]

Alternatively, we can use global variable for unknown xref value , initialized at early system startup.

[skra]

In D8616#210278, @meloun-miracle-cz wrote:

In D8616#210126, @andrew wrote:

In D8616#210013, @skra wrote:

(3) XREF_INVALID used the way you are suggesting is a nonsense. I would see XREF_INVALID only as an error code.

We already use errno values in this code so the error code would be EINVAL.

Where we pass any error code to xref parameter? Ore where we return error code from function returning xref?

Just to explain my note. The XREF_INVALID as an error code was meant only as hypothetical case. Indeed, there is not use for it now. I just wanted to point out my feeling about it. In other words, who is an arbiter to say that some xref is invalid? There may be no PIC with some xref in a system, but this xref still may be valid. For what do we need XREF_INVALID then?

[andrew]

In D8616#210278, @meloun-miracle-cz wrote:

In D8616#210126, @andrew wrote:

In D8616#210013, @skra wrote:

(1) The staff around PIC and xref was implemented with concrete idea. You are trying to change it. So, it's not cleanup, it's an implementation of new idea. And I don't see why this change would be better.

It might be ok for FDT, however we need to also consider ACPI where zero is a valid xref.

(2) You are forcing devices which do not know (or have) their XREFs to generate some instead of using one and only XREF_UNKNOWN for these cases, which is zero.

But zero may be a valid xref. I would prefer we create a new value for XREF_UNKNOWN separate from XREF_INVALID, e.g. -2. This will be used by devices that don't naturally have an xref, e.g. a PCI GPIO controller, but would allow the error checking in the patch to be kept. I didn't do this in this patch as I'm not aware of any drivers in the tree that would use this so am unable to test it.

In short:

• in current code takes 0 (or NULL) xref as unknown xref not as error indicator.
• we cannot expect that all "information sources" *FDT/ACPI/HINTS" can share same value for unknown xref natively

I don't see why, both ACPI and FDT will use positive values close or equal to zero.

• natural value for unknown xref is 0 for FDT , NULL for HINTS

-1 is a natural value for unknown, it's too large for an FDT xref to work, and memory around the top of the virtual address space is unusable by HINTS. It also has the property where

• if you want change value of unknown xref correctly, then you must also remap FDT no such property to INTRNG unknown xref for all FDT users

As best as I can tell the OFW code uses -1 for unknown values. This patch will help find bugs where these values are unchecked.

• the "#define XREF_INVALID (-1)" is nonsense, it's hidden "#define XREF_INVALID ACPI_INVALID_HANDLE" as these values are coupled

The exact value doesn't matter, e.g. -2 would also work, however by using -1 we will catch bugs in drivers that fail to check the return value of ofw_bus_get_node before passing them to OF_xref_from_node.

(3) XREF_INVALID used the way you are suggesting is a nonsense. I would see XREF_INVALID only as an error code.

We already use errno values in this code so the error code would be EINVAL.

Where we pass any error code to xref parameter? Ore where we return error code from function returning xref?

They shouldn't be passing in error values as if it was a valid xref.

(4) This part of INTRNG was implemented strictly according to Michal, so I hope that you do not commit this before Michal agrees with it.

As far as I know I've answered all questions, and haven't seen any further feedback in a month.

Sorry for delay, I'm overloaded these days (or weeks)
And no, I don't think that you answered my questions:

• newly added kasserts limits already working cases. But why?

Because drivers passing the invalid value in are broken, they should only get this from failing to check return values.

• you want to change already working pic_lookup() behavior. But why?

Because it won't work with ACPI. As such I tried to make the minimal changes needed to support this.

• you want to separate MSI PIC and standard PIC, even if the current code can handle this without problem. But why? Moreover the separation is not always possible - both interrupts (legacy and MSI) can be terminated in same PIC.

Do you have an example of this? The only interrupt controller I know that calls both intr_pic_register and intr_msi_register is the GICv3 ITS driver and that's only due to the way interrupts are passed between the GICv3 and ITS drivers.

All the interrupt controllers I've seen handle either legacy or MSI interrupts.

If all these changes are motivated only by the fact that 0 is valid ACPI handle, then please use recommendation (ignored until now) from my first response.

I would also like to have people to clean up their drivers, e.g. failing to check return values when they should. It will also help to make INTRNG less FDT centric.

In D8616#210293, @meloun-miracle-cz wrote:

Alternatively, we can use global variable for unknown xref value , initialized at early system startup.

If, as you claim above, this is a per-bus value I don't see how this would help as there may be different busses in a single system, e.g. ACPI and PCIe.

[mmel]

In D8616#212404, @andrew wrote:

In D8616#210278, @meloun-miracle-cz wrote:

In D8616#210126, @andrew wrote:

In D8616#210013, @skra wrote:

(1) The staff around PIC and xref was implemented with concrete idea. You are trying to change it. So, it's not cleanup, it's an implementation of new idea. And I don't see why this change would be better.

It might be ok for FDT, however we need to also consider ACPI where zero is a valid xref.

(2) You are forcing devices which do not know (or have) their XREFs to generate some instead of using one and only XREF_UNKNOWN for these cases, which is zero.

But zero may be a valid xref. I would prefer we create a new value for XREF_UNKNOWN separate from XREF_INVALID, e.g. -2. This will be used by devices that don't naturally have an xref, e.g. a PCI GPIO controller, but would allow the error checking in the patch to be kept. I didn't do this in this patch as I'm not aware of any drivers in the tree that would use this so am unable to test it.

In short:

• in current code takes 0 (or NULL) xref as unknown xref not as error indicator.
• we cannot expect that all "information sources" *FDT/ACPI/HINTS" can share same value for unknown xref natively

I don't see why, both ACPI and FDT will use positive values close or equal to zero.

• natural value for unknown xref is 0 for FDT , NULL for HINTS

-1 is a natural value for unknown, it's too large for an FDT xref to work, and memory around the top of the virtual address space is unusable by HINTS. It also has the property where

• if you want change value of unknown xref correctly, then you must also remap FDT no such property to INTRNG unknown xref for all FDT users

As best as I can tell the OFW code uses -1 for unknown values. This patch will help find bugs where these values are unchecked.

• the "#define XREF_INVALID (-1)" is nonsense, it's hidden "#define XREF_INVALID ACPI_INVALID_HANDLE" as these values are coupled

The exact value doesn't matter, e.g. -2 would also work, however by using -1 we will catch bugs in drivers that fail to check the return value of ofw_bus_get_node before passing them to OF_xref_from_node.

(3) XREF_INVALID used the way you are suggesting is a nonsense. I would see XREF_INVALID only as an error code.

We already use errno values in this code so the error code would be EINVAL.

Where we pass any error code to xref parameter? Ore where we return error code from function returning xref?

They shouldn't be passing in error values as if it was a valid xref.

(4) This part of INTRNG was implemented strictly according to Michal, so I hope that you do not commit this before Michal agrees with it.

As far as I know I've answered all questions, and haven't seen any further feedback in a month.

Sorry for delay, I'm overloaded these days (or weeks)
And no, I don't think that you answered my questions:

• newly added kasserts limits already working cases. But why?

Because drivers passing the invalid value in are broken, they should only get this from failing to check return values.

• you want to change already working pic_lookup() behavior. But why?

Because it won't work with ACPI. As such I tried to make the minimal changes needed to support this.

I think that we got into some form of total confusion.
The current INTRNG code takes 0 in xref as special "don't use me for matching" value and this value is part of "normal lookup protocol". It cannot be taken as a error value, in any case.

It seems that I misread your sentence (sorry for that):

I would prefer we create a new value for XREF_UNKNOWN separate from XREF_INVALID, e.g. -2. This will be used by devices that don't naturally have an xref, e.g. a PCI GPIO controller, but would
allow the error checking in the patch to be kept.

So yes, I agree that having two definitions is the best way to solve this problem. With this, all KASSERTs outside of pic_lookup code give sense for me.
But the second part, changes in pic_lookup() are incorrect -. 0 must be replaced by XREF_UNKNOWN without changing lookup logic.

I didn't do this in this patch as I'm not aware of any drivers in the tree that would use this so am unable to test it.

Imo, all calls to intr_map_irq(), intr_pic_register() and intr_msi_register() are subject to check. I'm pretty sure that intr_map_irq() is called with 0 xref from ofw_gpio code.
Also, I think that OF_xref_from_node(ofw_bus_get_node(sc->sc_dev)); can return 0 in some cases, but I'm not sure here.

• you want to separate MSI PIC and standard PIC, even if the current code can handle this without problem. But why? Moreover the separation is not always possible - both interrupts (legacy and MSI) can be terminated in same PIC.

Do you have an example of this? The only interrupt controller I know that calls both intr_pic_register and intr_msi_register is the GICv3 ITS driver and that's only due to the way interrupts are passed between the GICv3 and ITS drivers.

All the interrupt controllers I've seen handle either legacy or MSI interrupts.

Majority of ARMv6 SoC, with PCIe terminates all interrupts (internal, legacy, MSI) in PCIe IP block.
tegra124 (and also tegra210) have MSI related registers inside of PCIe block, mixed with bits for other functions. But, at least, it have separate interrupt lines for internal/legacy and MSI.
imx6 - register are mixed in PCIe register space. legacy (4) interrupts are routed to GIC where line D is shared with MSI
qcom - register are mixed in PCIe register space. Older SoCs have only one interrupt line, newer have legacy and MSI lines separated

[andrew]

Add XREF_NONE

[mmel]

sys/kern/subr_intr.c

107

imho, It would be better to group this with XREF_NONE

702

KASSERT(xref != XREF_INVALID, ("%s: Invalid xref", func)); ???

705

Again, I prefer original version (returning NULL for dev == NULL && xref != XREF_NONE ).
It saves few lines of repeated code for each caller.

716

XREF_NONE

717

I significantly disagree with this change. Why you want to limit INTRNG functionality (see above)?
It's clear that this is not related with ACPI issue (handle 0) and it breaks my futher improving of tegra PCI driver,

892

Typo? XREF_NONE looks more appropriate.

[manu]

Is it still needed ?