From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Daney Subject: Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts. Date: Wed, 28 Mar 2012 18:41:03 -0700 Message-ID: <4F73BDAF.7020206@cavium.com> References: <1332790281-9648-1-git-send-email-ddaney.cavm@gmail.com> <1332790281-9648-3-git-send-email-ddaney.cavm@gmail.com> <4F711E69.1080302@gmail.com> <4F7205F3.3000108@cavium.com> <20120328222246.8AFA83E0CFE@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20120328222246.8AFA83E0CFE@localhost> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org To: Grant Likely Cc: "linux-mips-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org" , "devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org" , Rob Herring List-Id: devicetree@vger.kernel.org On 03/28/2012 03:22 PM, Grant Likely wrote: > On Tue, 27 Mar 2012 11:24:51 -0700, David Daney wrote: >> On 03/26/2012 06:56 PM, Rob Herring wrote: >>> On 03/26/2012 02:31 PM, David Daney wrote: >>>> From: David Daney >> [...] >>>> +static int octeon_irq_ciu_map(struct irq_domain *d, >>>> + unsigned int virq, irq_hw_number_t hw) >>>> +{ >>>> + unsigned int line = hw>> 6; >>>> + unsigned int bit = hw& 63; >>>> + >>>> + if (virq>= 256) >>>> + return -EINVAL; >>> >>> Drop this. You should not care what the virq numbers are. >> >> >> I care that they don't overflow the width of octeon_irq_ciu_to_irq (a u8). >> >> So really I want to say: >> >> if (virq>= (1<< sizeof (octeon_irq_ciu_to_irq[0][0]))) { >> WARN(...); >> return -EINVAL; >> } >> >> >> I need a map external to any one irq_domain. The irq handling code >> handles sources that come from two separate irq_domains, as well as irqs >> that are not part of any domain. > > You can get past this limitation by using the struct irq_data .hwirq and > .domain members for the irq ==> hwirq translation, and for hwirq ==> > irq the code should already have the context to know which user it is. > > For the irqs that are not covered by an irq_domain, the driver is free > to set the .hwirq value directly. Ultimately however, it will > probably be best to add an irq domain for those users also. > > ... > > Howver, I don't understand where the risk is in overflowing > octeon_irq_ciu_to_irq[][]. From what I can see, the virq value isn't > used at all to calculate the array dereference. line and bit are > calculated from the hwirq value. What am I missing? > We do the opposite. We extract the hwirq value from the interrupt controller and then look up virq in the table. If the range of virq overflows the width of u8, we would end up calling do_IRQ() with a bad value. Also this dispatch code is not aware of the various irq_domains and non irq_domain irqs, it is a single function that handles them all calling do_IRQ() with whatever it looks up in the table. We could use a wider type for this lookup array, but that would increase the cache footprint of the irq dispatcher... David Daney David Daney > g. >