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

[Pantelis Antoniou <pantelis.antoniou@konsulko.com>]

Hi Guenter,

On Jun 23, 2014, at 10:48 PM, Guenter Roeck wrote:

> On 06/23/2014 12:13 PM, Pantelis Antoniou wrote:
>> Hi Ioan,
>> 
>> I'm going to let Grant answer that but the code in question doesn't look right.
>> 
>> On Jun 23, 2014, at 9:33 PM, Ioan Nicu wrote:
>> 
>>> Hi Pantelis,
>>> 
>>> On Mon, Jun 23, 2014 at 07:57:24PM +0300, ext Pantelis Antoniou wrote:
>>>> Hi Alexander,
>>>> 
>>>> On Jun 23, 2014, at 7:26 PM, Alexander Sverdlin wrote:
>>>> 
>>>>> Hello Pantelis!
>>>>> 
>>>>> On 22/06/14 11:40, ext Pantelis Antoniou wrote:
>>>>>> Introduce helper functions for working with the live DT tree,
>>>>>> all of them related to dynamically adding/removing nodes and
>>>>>> properties.
>>>>>> 
>>>>>> __of_copy_property() copies a property dynamically
>>>>>> __of_create_empty_node() creates an empty node
>>>>>> 
>>>>>> Bug fix about prop->len == 0 by Ionut Nicu <ioan.nicu.ext@nsn.com>
>>>>> 
>>>>> Are you sure about this? (see below...)
>>>>> 
>>> 
>>> Alexander is right, my fix was lost even though it's mentioned in this patch.
>>> 
>> 
>> I'm sorry, I didn't understand that the intention of the fix was to address
>> the issue below.
>> 
>>>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>>>>> ---
>>>> 
>>>> [snip]
>>>>>> +
>>>>>> +	if (prop->length > 0) {
>>>>>       ^^^^^^^^^^^^^^^^^^^^^
>>>>> Seems, that length==0 case will still produce value==NULL results,
>>>>> which will brake some checks in the kernel... Or am I missing something in
>>>>> the new version?
>>>>> 
>>>> 
>>>> prop->value will be set to NULL, and length will be set to zero (kzalloc).
>>>> This is a normal zero length property.
>>>> 
>>>> I don't know of any place in the kernel accessing the value if prop->length==0
>>>> 
>>> 
>>> We have a simple use case. We have an overlay which adds an interrupt controller.
>>> If you look in drivers/of/irq.c, in of_irq_parse_raw():
>>> 
>>> [...]
>>> 	/* Now start the actual "proper" walk of the interrupt tree */
>>> 	while (ipar != NULL) {
>>> 		/* Now check if cursor is an interrupt-controller and if it is
>>> 		 * then we are done
>>> 		 */
>>> 		if (of_get_property(ipar, "interrupt-controller", NULL) !=
>>> 				NULL) {
>>> 			pr_debug(" -> got it !\n");
>>> 			return 0;
>>> 		}
>>> [...]
>>> 
>>> A node is identified as an interrupt controller if it has a zero-length property
>>> called "interrupt-controller" but with a non-NULL value.
>>> 
>>> My proposed fix for this was to remove the if () condition. propn->value will be
>>> allocated with kmalloc(0) which returns ZERO_SIZE_PTR which is != NULL.
>>> 
>> 
>> 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.


> Either case, if the new code depends on proper use of of_get_property(), we may have
> a problem unless all those bad use cases are fixed.
> 

I have prepared a patch that fixes the caller problem, and issues a warning, so that we
know what we have to fix.

> Guenter

Regards

-- Pantelis