Re: [Xen-devel] [PATCH v4 1/6] xen: dt: add dt_for_each_irq_map helper

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 03/07/15 15:16, Ian Campbell wrote:
> On Fri, 2015-06-26 at 19:47 +0200, Julien Grall wrote:
> 
>>> +    /* First get the #interrupt-cells property of the current cursor
>>> +     * that tells us how to interpret the passed-in intspec. If there
>>> +     * is none, we are nice and just walk up the tree
>>> +     */
>>> +    do {
>>> +        tmp = dt_get_property(ipar, "#interrupt-cells", NULL);
>>> +        if ( tmp != NULL )
>>> +        {
>>> +            intsize = be32_to_cpu(*tmp);
>>> +            break;
>>> +        }
>>> +        tnode = ipar;
>>> +        ipar = dt_irq_find_parent(ipar);
>>> +    } while ( ipar );
>>
>> This loop doesn't seem useful. AFAIU the spec, the PCI node (i.e your 
>> variable dev) will always have property #interrupt-cells. We will break 
>> directly.
> 
> The reason for this is explained in the comment i.e. "we are nice" to
> broken trees.
> 
> This code is from Linux, via dt_irq_map_raw, I don't fancy messing with
> it too much.

Ok. Let's stick with that.


>>> +        dt_raw_irq.controller = ipar;
>>> +        dt_raw_irq.size = pintsize;
>>
>> Don't you need to check that pintsize is < DT_MAX_IRQ_SPEC?
>> The previous "if ( ... > DT_MAX_IRQ_SPEC )" will likely be done on a 
>> different parent.
> 
> Yes, I think that would be a good idea.
> 
>>
>> For instance with the following incomplete DT (based on the 
>> apm-storm.dsi in Linux):
>>
>> pcie0 {
>>     #interrupt-cells = <1>;
>>     ...
>>     #interrupt-map = < 0x0 0x0 0x0 0x1 &gic ... >
>> }
>>
>> The first ipar will point to pcie0 because it has a property 
>> "#interrupt-cells", while the second time ipar will point to the gic node.
>>
>>> +        for ( i = 0; i < pintsize; i++ )
>>> +            dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1);
>>> +
>>> +        ret = dt_irq_translate(&dt_raw_irq, &dt_irq);
>>> +        if ( ret < 0 )
>>
>> The other caller of dt_irq_translate returns an error when ret is not 0. 
>> I would do the same here.
> 
> dt_device_get_irq just returns the value of dt_irq_translate directly.

But the caller of dt_device_get_irq is treating everything other than 0
as an error.

> 
> Are you suggesting this code should treat positive results as an error
> as well as negative ones? I don't agree, this function has the normal 0
> on success -ve on error semantics AFAICT.

Well, it's not documented so it can be interpreted differently. I
personally interpreted as anything other than 0 is an error. This is how
Linux behave on most of the of_* function and I think it's "safer".

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel