Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Tomasz Figa <tomasz.figa@gmail.com>]

On Thursday 22 of August 2013 15:08:12 Stephen Warren wrote:
> On 08/22/2013 03:01 AM, Lars Poeschel wrote:
> > On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote:
> >> On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> >>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> >>>> 
> >>>> +			 */
> >>>> +			if (irq_domain && irq_domain->ops->xlate)
> >>>> +				irq_domain->ops->xlate(irq_domain, 
gcn,
> >>>> +						       intspec + 
i, intsize,
> >>>> +						       &hwirq, 
&type);
> >>>> +			else
> >>>> +				hwirq = intspec[0];
> >>> 
> >>> Is it a correct fallback when irq_domain is NULL?
> >> 
> >> Indeed this fallback is dangerous. The /only/ way to parse an IRQ
> >> specifier is with binding-specific knowledge, which is obtained by
> >> calling irq_domain->ops->xlate(). If the IRQ domain can't be found,
> >> this operation simply has to be deferred; we can't just guess and
> >> hope.> 
> > At least the of irq mapping code make this assumption also:
> > kernel/irq/irqdomain.c:483
> > It should be valid for us here too.
> > The additional assumption that I made is that if irq_domain == NULL
> > (not only xlate), that we can use intspec[0] either.
> 
> OK, I guess it's likely this won't cause any additional issue then. I
> suspect most IRQ domains use within the context of device tree already
> provide an explicit xlate op anyway; for example irq_domain_simple_ops
> points at the default irq_domain_xlate_onetwocell.

We got away from the problem I pointed in my reply. If irq_domain == NULL, 
there is no way to translate specifier to hwirq (and in what domain such 
hwirq would be in anyway?).

> >>>> +
> >>>> +			hwirq = be32_to_cpu(hwirq);
> >>> 
> >>> Is this conversion correct? I don't think hwirq could be big endian
> >>> here (unless running on a big endian CPU).
> >> 
> >> I think that should be inside the else branch above.
> > 
> > No it has to be in both branches as it is. Device tree data is big
> > endian. The conversion is converting big endian data (from device
> > tree in both cases) to cpu endianess and not coverting TO big endian.
> > My test machine is a arm in little endian mode and it provided wrong
> > values if I did not do the conversion.
> > What I am a bit unsure about is if the xlate function is expecting the
> > intspec pointer to point to big endian device tree data or data
> > already
> > converted to cpu endianess. For the standard xlate functions
> > irq_domain_xlate_[one|two|onetwo]cell it does not matter.
> 
> The xlate function assumes that data is already converted to CPU-endian.
> See:
> 
> irq_of_parse_and_map() ->
>     of_irq_map_one() ->
>         of_irq_map_raw() ->
>             out_irq->specifier[i] = of_read_number(intspec +i, 1);
>     irq_create_of_mapping()
> 
> (of_read_number does the be32_to_cpu() internally)

So basically to be correct, the code here would need to read the specifier 
into internal buffer using a helper like of_read_number() or maybe even 
of_property_read_u32_array() and then pass this buffer to xlate().

Best regards,
Tomasz