Date: Thu, 21 Jul 2016 09:34:39 +0200 From: Michal Meloun <mmel@FreeBSD.org> To: Nathan Whitehorn <nwhitehorn@freebsd.org>, Svatopluk Kraus <skra@FreeBSD.org>, src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org Subject: Re: svn commit: r301453 - in head/sys: arm/arm arm64/arm64 dev/fdt dev/gpio dev/iicbus dev/ofw dev/pci dev/vnic kern mips/mips sys Message-ID: <57907B0F.9070204@FreeBSD.org> In-Reply-To: <05a80ac6-4285-ec9d-36e9-2f92c609f746@freebsd.org> References: <201606051620.u55GKD5S066398@repo.freebsd.org> <b9606755-69cb-2cb0-04d7-6be32e4cb89e@freebsd.org> <578E0B5D.3070105@FreeBSD.org> <e026f6fc-76ed-5dbe-00fc-365b6d7bcf94@freebsd.org> <578F6075.7010500@FreeBSD.org> <05a80ac6-4285-ec9d-36e9-2f92c609f746@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Dne 20.07.2016 v 17:45 Nathan Whitehorn napsal(a): > > > On 07/20/16 04:28, Michal Meloun wrote: >> Dne 19.07.2016 v 17:06 Nathan Whitehorn napsal(a): >>> >>> >>> On 07/19/16 04:13, Michal Meloun wrote: >>>> Dne 19.07.2016 v 2:11 Nathan Whitehorn napsal(a): >>>> Hi Nathan, >>>> I’m afraid that skra is on vacation, for next 2 weeks (at minimum), so >>>> please don’t expect quick response. >>>> >>>>> Could you please describe what this change is in more detail? >>>> Short description is appended. >>>> >>>>> It breaks a lot of encapsulations we have worked very hard to >>>>> maintain, >>>>> moves ARM code into MI parts of the kernel, and the OFW parts violate >>>>> IEEE 1275 (the Open Firmware standard). In particular, there is no >>>>> guarantee that the interrupts for a newbus (or OF) device are >>>>> encoded in >>>>> a property called "interrupts" (or, indeed, in any property at >>>>> all) on >>>>> that node and there are many, many device trees where that is not the >>>>> case (e.g. ones with interrupt maps, as well as Apple hardware). By >>>>> putting that knowledge into the OF root bus device, which we have >>>>> tried >>>>> to keep it out of, this enforces a standard that doesn't actually >>>>> exist. >>>> Imho, this patch doesn’t change anything in this area. Only >>>> handling of >>>> “interrupts” property is changed, all other cases are unchanged (I >>>> hope). Also, INTRNG code is currently shared by ARM, ARM64 and MIPS. >>> >>> But "interrupts" isn't a generic part of OF. This makes it one, >>> incorrectly. >> How? Can you be little more exact ? > > Because it puts knowledge into ofwbus that expects that children at > arbitrary levels of nesting have interrupts defined by an "interrupts" > property. You could patch this through on sub-devices, of course, but > that's already done correctly by the existing ofw_bus_map_intr() code > in a much more robust way that doesn't involve trying to guess how > sub-buses and devices have chosen to allocate resources. Why reinvent > the wheel all the way through the bus hierarchy? Nope, the code only expect that „interrupts" property is default way hot to get interrupt description. Any device or bus in the hierarchy can fill appropriate resource list, or terminate call at any level. > >>> >>>> >>>>> I'm hesitant to ask for reversion on something that landed 6 weeks >>>>> ago >>>>> without me noticing, but this needs a lot more architectural work >>>>> before >>>>> any parts of the kernel should use it. >>>>> -Nathan >>>> I think that it’s too late. This patch series consist of r301451 >>>> (https://reviews.freebsd.org/D6632), >>>> r301453, r301539 and 301543. And new GPIO interrupts are currently >>>> used >>>> (by in tree drivers or in development trees). >>> >>> Well, then we need in-place rearchitecture. >>> >>>> >>>> >>>> The root of problem is that standard way of delivering interrupt >>>> resource to consumer driver doesn’t works in OFW world. >>>> >>>> So we have some fact: >>>> - the format of interrupt property is dependent of interrupt >>>> controller and only interrupt controller can parse it. >>>> - the interrupt property can have more data than just interrupt >>>> number. >>>> - single interrupt controller must be able to handle multiple >>>> format of interrupt description. >>>> >>>> In pre-patchset era, simplebus enumerates children and attempts to set >>>> memory and interrupts to resource list for them. But the interrupt >>>> controllers are not yet populated so nobody can parse interrupt >>>> property. Moreover, in all cases (parsed or not), we cannot store >>>> complete interrupt description into resource list. >>> >>> We have done this for many years on PowerPC and sparc64 with delayed >>> configuration of interrupts and a look-up table. This handles >>> complicated bus configurations better than this code and requires no >>> changes outside of a few MD files. That is why the (now partially >>> duplicated) OFW_BUS_MAP_INTR() function exists. That one also has >>> the benefit of still working when used in conjunction with, e.g., >>> devices with an interrupt-map-mask property. >>> >>>> >>>> The patch simply postpones reading of interrupt property to >>>> bus_alloc_resource() (called by consumer driver) time. >>>> >>>> Due to this, we can: >>>> - parse interrupt property. The interrupt driver must exist >>>> at this time. >>> >>> This only works with some types of interrupt properties, not all, >>> and breaks if the interrupt driver hasn't attached yet (which it >>> can't be guaranteed to -- some PPC systems have interrupt drivers >>> that live on the PCI bus, for example). >> How you can allocate (and reserve it in rman) interrupt if is not >> mapped (so you have not real IRQ number for it). Just for notice - >> multiple virtual IRQs can be mapped into single real IRQ. > > The core idea is to think of the full interrupt specifier -- the > interrupt parent and the full byte string in the device tree -- as the > IRQ rather than the interrupt pin on some chip (which is usually, but > not always, the first word in that byte string). The "virtual" IRQ > number is just a compression of that longer piece of data, which > usually can't fit in an rman resource. I understand. While this approach can works (and actually works) for single sourced OFW world, it immediately fails if you must be able to parse data from different sources (which uses different encoding) (OFW, UEFI / ACPI, GPIO...) within one system. By this, one byte string in OFW encoding can describe one IRQ and exactly same string in UEFI encoding can describe different IRQ. Or, in reverse, OFW and UEFI can describe same (and compatible) IRQ by two different strings. This is exact reason, why we discards virtual IRQ idea and I think that this fact is root issue of this clash. Probably it doesn't make sense to talk about others, unless we can find consensus on this. > There is no need to actually activate those interrupts before > interrupts are enabled, so you can just cache them in a table until > the end of device probing, which lets you break circular dependency > loops between bus and interrupt topology. > > So long as you keep track of your mapping and the same (parent, > interrupt specifier) parent always gives the same virtual IRQ, there > is no way in this system to map multiple active IRQs onto a single > interrupt pin on the PIC unless your device tree is broken and > specifies two devices with incompatible modes (active high and edge > downgoing or something) on the same pin. In this case, nothing you can > do will save you -- unless your PIC supports interrupts for different > kinds of events, in which case this system will work perfectly by > treating them as different interrupts to the kernel for which the fact > they are on the same pin is immaterial. > > I should note that ARM and MIPS have an almost complete implementation > of this already: maybe some more intr_machdep.c logic is needed for > some cases, but all the rest of the plumbing is there. > >> >>> >>>> - bus_alloc_resource() returns resource, so we can attach parsed >>>> interrupt data to it. By this, the resource itself can be used >>>> for delivering configuration data to subsequent call to >>>> bus_setup_intr() (or to all related bus_<foo>() calls). >>>> >>>> >>>> The patched code still accepts delivering of interrupts in resource >>>> list. >>>> >>>> Michal >>>> >>> >>> Given that other code depends on this, fixing it will likely require >>> some complex work. I wish I had known about it when it went in. >>> >>> There are three main problems: >>> 1. It doesn't work for interrupts defined by other mechanisms (e.g. >>> interrupt-map properties) >> I aggree, but missing ' interrupt-map' functioanlity is not caused by >> this patch. > > It is in that the standard system already implements it completely. Really? https://svnweb.freebsd.org/base/head/sys/dev/ofw/ofw_bus_subr.c?revision=301453&view=markup#l521 and https://svnweb.freebsd.org/base/head/sys/arm/arm/nexus.c?revision=301453&view=markup#l411 > >> >>> 2. It partially duplicates the functionality of OFW_BUS_MAP_INTR(), >>> but is both problematically more general and less flexible (it has >>> requirements on timing of PIC attachment vs. driver resource >>> allocation) >> OFW_BUS_MAP_INTR() can parse only OFW based data and expect that >> parsed data are magicaly stored within the call. >> The new method, bus_map_intr(), can parse data from multiple >> sources (OFW, UEFI / ACPI, synthetic[gpio device + pin number]). It >> also returns parsed data back to caller. > > That is not true. It works as long as you can specify the interrupt > state as a 32-bit key of some kind for the PIC and a string of > arbitrary data, which works with all of those. You could even make the > interrupt data be a pointer to exactly the structs you have chosen to > define here. Nope, in heterogeneous world, same string can describe two different IRQs and/or two different strings can describe single IRQ in compatible manner. > >> And no, it doesn't add any additional timing requirements . > > As far as I can tell, it requires the interrupt controller to be > attached before you can allocate interrupts. Is that not true? Yes, sure. And the patch doesn't change this. Before: https://svnweb.freebsd.org/base/head/sys/kern/subr_intr.c?view=markup&pathrev=301263#l1103 After: https://svnweb.freebsd.org/base/head/sys/kern/subr_intr.c?view=markup&pathrev=301543#l928 > >> >>> 3. It is not fully transparent to end code. Since it happens at >>> bus_alloc_resource() time, it is complicated to get the appropriate >>> values for IRQs constructed by composite techniques (interrupt-map >>> vs. interrupts vs. hand allocation vs. PCI routing, for example). >> I don't see any limitation - can you be more exact? Why is not >> transparent? Why is more complicated ? > > Suppose that a PCI device adds more IRQs to its resource list or > modifies the ordering. How is whatever bus layer supposed to do > something sensible at allocation time? It requires that RID numbers > mean something to the parent bus after assignment, which is not > guaranteed by anything and is, in more than handful of cases I think > of, not true in practice. Sure. And since the new code allows delivering resources in RL, so I don't see any limitation here. > >>> It is much easier to do this correctly at bus attach time when the >>> resource lists are made (how PPC does it). >>> >> I don't agree. I don't agree. Making this at bus attach time leads >> into complicated 'virtual' IRQ infrastructure, with many unresolved >> corner cases. > > Which unresolved corner cases? This has been working correctly on a > number of platforms in both FreeBSD and Linux for many years. Nope, it doesn't work for ARM yet (for GPIO interrupts for example) and Linux uses EPROBE_DEFER mechanism for a long time. See: http://lxr.free-electrons.com/source/drivers/base/platform.c#L87 > >> >>> (1) is easy to fix without API changes, but (2) and (3) are >>> fundamental architectural problems that will bite us immediately >>> down the road and cause a permanent schism between OF support on >>> different platforms. >>> >>> Let me describe how this is handled on PowerPC (Linux on PPC solves >>> the problem the same way). When constructing a resource list, bus >>> drivers that construct them from OF properties call >>> ofw_bus_map_intr() with the interrupt parent phandle and the array >>> of cells corresponding to the interrupt. This thunks immediately to >>> nexus, which connects to code in intr_machdep.c. Code there assigns >>> a unique made-up virtual IRQ and returns it, caching the interrupt >>> parent ID and opaque interrupt data (if the same string of data >>> reappears later, you get back the same virtual IRQ of course). >>> >>> When PIC drivers attach and register themselves with the interrupt >>> handling layer, all the interrupts for that PIC are passed to it >>> along with the virtual IRQ. The PIC driver is supposed to know what >>> its interrupt data mean, which can be safely guaranteed, and it >>> presents the assigned virtual IRQ number to the kernel when >>> dispatching interrupts. (IRQs configured after PIC attachment are >>> passed through immediately). >>> >>> This accomplishes the following things: >>> 1. Parsing interrupt data is moved to the PIC driver, which is the >>> only place it can be done safely. >> I don't see anything different comparing with INTRNG. > > What I am advocating *is* INTRNG, at least as originally conceived and > implemented. > >>> 2. There is no ordering requirement on PIC attachment vs. the >>> attachment of anything else. >> I think thats is not a true - PIC must exist before >> bus_alloc_resource() / bus_setup_intr() is called. > > It does not with the IRQ mapping infrastructure. Interrupts are set up > at PIC attachment, whenever that occurs. > Assuming that bus_alloc_resource and bus_setup_intr() are close thorougher and in linear piece of code, can i assume that you can call bus_setup_intr() without PIC attached ? >> >>> 3. Changes are extremely minimal relative to the "standard" >>> interrupt flow: you only have to patch code that is already directly >>> dealing with OF interrupts. >> I don't see anything different comparing with INTRNG. > > Again, this was the original INTRNG architecture and is already > implemented. As such, there are *no* changes required on ARM to get > it. bus_map_intr() adds a bunch of new code, in parallel with the old > code that also solves the problem, to no purpose. So, on PPC, how i can get interrupt for GPIO pin described by this property: https://svnweb.freebsd.org/base/head/sys/gnu/dts/arm/tegra124-jetson-tk1.dts?revision=295436&view=markup#l1691 > >>> 4. It happens at bus enumeration time, when results can be >>> guaranteed self-consistent. >> Where do you see any potential source of inconsistency in INTRNG? > > See the example above about modified interrupt lists. There is also no > obvious way for a child device to construct an interrupt not assigned > to it by the parent device from device tree properties without knowing > in some detail what kind of interrupt needs to be built. > >> >>> 5. It combines naturally with ofw_bus_lookup_imap() and friends in >>> the interrupt-map case (e.g. for PCI). >> Again, I don't see anything different. Proper parsing of interrupt >> property is not a problem of INTRNG (but must be fixed, of course). > > But it is *already* fixed by the standard code that already exists. > You are introducing a less-functional parallel code path here. NO, its not fixed, at least not for ARM. > >>> >>> I'm not sure what the right path forward is, but this code needs to >>> be fixed. The PowerPC code is fully MI, and was the template for the >>> original INTRNG, so it shouldn't be too bad to replace. >>> -Nathan >>> >> >> So, new INTRNG: >> - Introduces new more general bus method that can parse interrupt >> configuration >> data from any source. Is this step backward? > > Yes, since it is more general in some sense, while simultaneously > handling fewer cases than code that already exists and is implemented. > >> >> - Old INTRNG and PPC code stores unparsed and/or parsed interrupt data in >> INTRNG and each consumer must query for them. This data sharing >> also causes >> significant locking issues. New INTRNG stores interrupt >> configuration data into >> given resource, so each relevant bus method can access it immediately. >> Is this step backward? > > Which locking issues? And yes, it is. > >> >> - New INTRNG is not OFW centric, it can works with virtually >> unlimited number >> of configuration data sources. Is this step backward? > > Also yes, because it makes the interrupt handles less opaque, which > makes the infrastructure less flexible. > >> - New INTRNG correctly uses standard system infrastructure. Real IRQ >> number >> is reserved in rman within bus_alloc_resource() call, interrupt HW is >> configured (only!) within bus_setup_intr() call. Is this step >> backward? > > The "real" IRQ number is not well defined always, so requiring that is > a step backwards, yes. > >> - New INTRNG completely eliminates huge and not always working virtual >> IRQ concept. > > When does it "not always work"? It seems to, in fact, always work on > multiple platforms and have for a long time in the face of all kinds > of totally bizarre topologies and system architectures. > >> >> >> Don’t take me bad, I’m open to any change. But no, at this time, I’m >> not ready to completely revert someone else's work – although I am a >> co-author. > > I would urge, in the strongest possible terms, that this be backed out > from stable/11 at least. We can add the new API back for 11.1 if we > want it, but we totally lose the ability to change it later in the > stable/11 cycle if it stays in now. > -Nathan > The API is part of still unstable, experimental INTRNG, so its not fixed we we can change it at any time, I think. But yes, we forget to wrap new bus_map_intr() method (and associated code) by #ifdef INTRNG. Is this sufficient for you? Michal
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?57907B0F.9070204>