INTRNG (Was: svn commit: r301453....)

[Nathan Whitehorn]

On 07/29/16 15:16, Svatopluk Kraus wrote:
>   Well, I'm online again, unfortanately, I'm quite busy, so just quick
> response for now to the formal ask for the reversion.
>
> On Fri, Jul 29, 2016 at 4:41 PM, Nathan Whitehorn
> <nwhitehorn at freebsd.org> wrote:
> [snip]
>
>> So here is where I am right now:
>> - The perceived advantage of the new API seems to be that the mapping
>> information (interrupt parent, etc.) ends up in a struct resource instead of
>> in a centralized mapping table
> It's more than that. The change has made INTRNG framework not
> dependent on OFW(FDT) stuff. So after next r301543, the framework is
> clean of all additions related to various buses. It was something I
> wanted to do before FreeBSD-11 release, once when adrian@ moved INTRNG
> to sys/kern. The framework is used by arm, arm64 and mips now, so from
> my point of view, it was quite important. The way how it was done is
> not perfect and may be changed in the future. However, it does not
> break anything, does not change old functionality, and only few bus
> drivers were modified slightly. It was also only way which I and
> Michal have agreed on considering current kernel code.

Except that the API cannot support all the existing things, so now we 
have two APIs to support for the lifetime of the 11.x branch. This 
needed extensive review from platform maintainers and from arch@, which 
it did not get.

>
>> - Additionally, it gives you a better shot at having the PIC online before
>> the PIC's interrupts are parsed (which is not required, but nice).
> For me, it's just correct design. Please, not all world is about OFW.

Of course not! But it does affect OFW platforms, which worked fine 
before. I also disagree strongly about the "correct design" in this 
case. This API is fragile since it requires coordination between 
resource list enumeration and allocation and it provides fewer features 
than the old one (which was not OFW-only either).

>> - Beyond these aesthetic points, there are no concrete examples of new
>> functionality added by this API, aside from some minor implementation bugs
>> of the old one on ARM that are easily fixed.
> Please, don't use subjective attributes like "aesthetic".

If the issues are not aesthetic, then what concrete, technical problems 
does this solve?

>
>
>> - There are, conversely, a number of concrete cases where this new API would
>> not be able to do the right thing. Some of these can be worked around or
>> fixed with significant restructuring in the MI parts of the kernel.
> I have not looked yet at these concrete cases, but which MI parts of
> kernel do you think of? It may be supposed that some bus drivers would
> be modified, but not MI parts of kernel!

If you don't want a global IRQ lookup table, the PCI interrupt routing 
APIs need to change, for one thing. You would also need a partial 
delayed attach mechanism to handle bus bridges that themselves have 
interrupts (hot-plug bridges, for example) and might have interrupt 
controllers as children athat does not currently exist in newbus.

>
>> - If we have both, the interrupt handling code in the MI parts of the kernel
>> will bifurcate. This patch alone has added a bunch of #ifdef to
>> kern/subr_bus.c, a new file to kern/, and lots of #ifdef to dev/ofw. This is
>> going to be really hard to maintain if we need both.
> IMO, this point is bogus totally. The #ifdefs in kern/subr_bus.c were
> added just to be polite to not-INTRNG kernels. They can be removed. No
> one new file was added. Lots of #ifdef in dev/ofw were added because
> ofw_bus_map_intr() return value is not checked in
> ofw_bus_intr_to_rl(), so resource list entry is always added. They are
> there also to clearly manifest INTRNG needs.

#ifdef or not, there are now two unrelated mechanisms and code paths for 
dealing with device tree interrupts. One of them cannot possibly be used 
on PowerPC (this one) and so there will have to be two parallel code 
paths in perpetuity.

>> Is this all correct?
>>
> I don't think so. Further, considering the reversion, I don't think
> that it can be done simply now. I suppose that more files were changed
> afterwards when no bus header is polluted by the framework now.
> However, as I have already wrote, this part of INTRNG may be changed
> to serve well for everyone. On the other hand, IMO, a centralized
> global interrupt mapping table is not good design, and if established
> after all, it should not be a part of the framework. That said, I
> suggest to continue with work on INTRNG.

It cannot be done simply because the code was checked in without review 
or warning during a code slush with a cryptic commit message and now we 
are stuck with it for 11.x. In HEAD, there are only a few commits based 
on this that seem to provide no functional changes, so I really don't 
think reverting would be that bad.

My concerns here are technical and I will stop complaining about this 
instantly if:
1. Anyone can point me to a single concrete example of a problem solved 
by this API that could not be solved with the existing code. You spent 
time writing this code, so there must be such a case!
2. Anyone can tell me how to implement the cases in my email to arch and 
on the wiki at  https://wiki.freebsd.org/Complicated_Interrupts using 
this API. These work perfectly fine with the normal newbus APIs but 
appear to break with this one.

This affects me directly since I maintain, or try to maintain, the 
content of dev/ofw and a platform (PowerPC) that relies on this code. 
I'm happy to switch the interrupt architecture, but it needs to be at 
least as functional as the old code to do that. Ideally, it would also 
be *more* functional so that it wasn't just churn -- but I would be 
willing to do the work regardless.

As it is, however, having this new API that seemingly can't be supported 
on one of our platforms imposes a significant burden on doing those 
things. That absolutely needs to be resolved before we can continue 
here. The normal procedure would be to revert, have a discussion, and 
then recommit. If you refuse to do that, or to have any meaningful 
discussion about the design of this patch, I am not sure what the path 
forward is.
-Nathan

>
> Svata
>
>
>> If so, can we please back this out until this discussion is complete? I'm
>> asking this formally at this point, under the Committer's Guide section
>> about reversion requests.
>> -Nathan
>>
>>