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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
