Re: [PATCH 5/6] OF: Utility helper functions for dynamic nodes

[Alexander Sverdlin <alexander.sverdlin@nsn.com>]

Hi Rob,

On 24/06/14 16:49, ext Rob Herring wrote:
>>>>>> If that's the case, the code in irq.c is wrong.
>>>>>>
>>>>>> interrupt-controller is a bool property; the correct call to use is of_property_read_bool()
>>>>>> which returns true or false when the value is defined.
>>>>>>
>>>>>> The use of of_get_property is a bug here. It is perfectly valid for a property to have a
>>>>>> NULL value when length = 0.
>>>>>>
>>>>>
>>>>> That usage is spread throughout the code though. There are three or four similar checks
>>>>> for interrupt-controller in the code, and many others using of_get_property() to check
>>>>> for booleans.
>>>>>
>>>>> Some examples:
>>>>>       s5m8767,pmic-buck2-ramp-enable
>>>>>       s5m8767,pmic-buck3-ramp-enable
>>>>>       s5m8767,pmic-buck4-ramp-enable
>>>>>       d7s-flipped?
>>>>>       atmel,use-dma-rx
>>>>>       linux,rs485-enabled-at-boot-time
>>>>>       marvell,enable-port1 (and many others)
>>>>>       linux,bootx-noscreen
>>>>>       linux,opened
>>>>>
>>>>> and many many others.
>>>>>
>>>>> Maybe people meant to use of_find_property() ?
>>>>>
>>>>
>>>> I bet... I see a lot of users doing if (of_get_property()).
>>>>
>>>> Which is no good.
>>>
>>> I don't see what the issue is. The irq.c code is correct. The code is
>>> checking for existence of a property or not and of_get_property
>>> adequately does that. of_find_property is equivalent in this case. Now
>>
>> The code is correct and the functions are equal as long as we declare that
>> void property has value != NULL. Which must not be actually and the patch
>> from Pantelis actually creates this situation first time.
>>
>> So let's first agree, how should the empty property look like.
>> length should be 0, it's clear. But what about the value?
> 
> Actually, it is really only about what of_get_property's behavior is,
> and I'm saying it should remain as is. You could also make
> of_get_property return an empty string if prop->value is NULL. That
> could still possibly break some callers of of_find_property which

This will not brake anything and especially of_find_property() users,
but it's even worse hack, than to ensure value!=NULL :)

> would need fixing. But there's ~300 callers vs. ~1300 for
> of_get_property. In scanning thru of_find_property callers, it look
> like in general they are okay if prop->value changed to NULL.

Most of the 1300 uses of of_get_property() are valid, because they want the
value directly. Only some of them, who actually only checks for the existence of
the property are wrong and must use of_find_property(). 

> Unless there is some difficulty to support, the lower risk approach is
> just ensure prop->value is not NULL.

-- 
Best regards,
Alexander Sverdlin.