Date: Sat, 30 Jul 2016 00:16:28 +0200 From: Svatopluk Kraus <skra@freebsd.org> To: Nathan Whitehorn <nwhitehorn@freebsd.org> Cc: Michal Meloun <mmel@freebsd.org>, "freebsd-arm@freebsd.org" <freebsd-arm@freebsd.org>, "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Subject: Re: INTRNG (Was: svn commit: r301453....) Message-ID: <CAFHCsPXON9xQdLou=0ZXEieMVenL-Og2cpWYucSgD3fTp63%2BTw@mail.gmail.com> In-Reply-To: <eb603349-eb88-866d-7a26-9e026518fd39@freebsd.org> References: <201606051620.u55GKD5S066398@repo.freebsd.org> <57907B0F.9070204@FreeBSD.org> <9d2a224c-b787-2875-5984-a7a2354e8695@freebsd.org> <57934ABD.6010807@FreeBSD.org> <4e7a3e8f-cc21-f5f2-e3e0-4dbd554a4cd0@freebsd.org> <5794720F.4050303@FreeBSD.org> <8bfd8668-bc49-e109-e610-b5cd470be3ec@freebsd.org> <57950005.6070403@FreeBSD.org> <f82018ee-51e7-60fa-2682-f0ef307a52b5@freebsd.org> <57961549.4020105@FreeBSD.org> <e2cace17-0924-2084-5fcf-626f87e41cc3@freebsd.org> <CANCZdfr%2BZ4XxXRY0yMiWXwp=8iKq54y3uJ9-OfAOdfxAs1qdtw@mail.gmail.com> <f94bfd25-fabf-efc3-55c9-cfdfd9e4d6e6@freebsd.org> <CANCZdfpz=z3gc3pyb_Qssa3vGJSnPv_r6J-SWDPPpE9zPYB9=w@mail.gmail.com> <ab44ddb1-515b-94ac-6b12-673b7c53d658@freebsd.org> <57976867.6080705@FreeBSD.org> <f2edac8f-2859-cd98-754e-881e2b2d1e63@freebsd.org> <5798E104.5020104@FreeBSD.org> <a5d43044-1733-6cc7-2e99-e85b60b0fcf3@freebsd.org> <579A25BB.8070206@FreeBSD.org> <30790e40-58b4-3371-c0f0-b7545571f389@freebsd.org> <579AFFC5.1040005@FreeBSD.org> <eb603349-eb88-866d-7a26-9e026518fd39@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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. > - 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. > - 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". > - 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 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. > > 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. 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 > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFHCsPXON9xQdLou=0ZXEieMVenL-Og2cpWYucSgD3fTp63%2BTw>