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

Tomasz Figa
2013-08-22T22:30:34Z

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