From owner-freebsd-arm@freebsd.org Sat Jul 30 01:35:40 2016 Return-Path: Delivered-To: freebsd-arm@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id DDAFABA7F92; Sat, 30 Jul 2016 01:35:40 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from c.mail.sonic.net (c.mail.sonic.net [64.142.111.80]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id BBAF412E5; Sat, 30 Jul 2016 01:35:40 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from zeppelin.tachypleus.net (75-101-50-44.static.sonic.net [75.101.50.44]) (authenticated bits=0) by c.mail.sonic.net (8.15.1/8.15.1) with ESMTPSA id u6U1ZUpP029127 (version=TLSv1.2 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); Fri, 29 Jul 2016 18:35:31 -0700 Subject: Re: INTRNG (Was: svn commit: r301453....) To: Svatopluk Kraus References: <201606051620.u55GKD5S066398@repo.freebsd.org> <4e7a3e8f-cc21-f5f2-e3e0-4dbd554a4cd0@freebsd.org> <5794720F.4050303@FreeBSD.org> <8bfd8668-bc49-e109-e610-b5cd470be3ec@freebsd.org> <57950005.6070403@FreeBSD.org> <57961549.4020105@FreeBSD.org> <57976867.6080705@FreeBSD.org> <5798E104.5020104@FreeBSD.org> <579A25BB.8070206@FreeBSD.org> <30790e40-58b4-3371-c0f0-b7545571f389@freebsd.org> <579AFFC5.1040005@FreeBSD.org> Cc: Michal Meloun , "freebsd-arm@freebsd.org" , "freebsd-arch@freebsd.org" From: Nathan Whitehorn Message-ID: <8efe4828-ab2a-02a6-902b-8614b1f4b24e@freebsd.org> Date: Fri, 29 Jul 2016 18:35:30 -0700 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Sonic-CAuth: UmFuZG9tSVaAqmOkWSG1yDcFr4npweYNEYuVMAL+kwkFbqCX8t7eFnj92NW0YBGQFI6dUiiaLP7nVCu76DwRvecJujjdvHtxAJTIl3ekOaE= X-Sonic-ID: C;6gmc5vVV5hGfPJtMTlz00w== M;1Kz65vVV5hGfPJtMTlz00w== X-Spam-Flag: No X-Sonic-Spam-Details: 0.0/5.0 by cerberusd X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "Porting FreeBSD to ARM processors." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 30 Jul 2016 01:35:41 -0000 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 > 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 >> >>