Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.

[David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>]

On 03/28/2012 03:22 PM, Grant Likely wrote:
> On Tue, 27 Mar 2012 11:24:51 -0700, David Daney<david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>  wrote:
>> On 03/26/2012 06:56 PM, Rob Herring wrote:
>>> On 03/26/2012 02:31 PM, David Daney wrote:
>>>> From: David Daney<david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>> [...]
>>>> +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.
>