[PATCH] of/irq: init struct resource to 0 in of_irq_to_resource()

[PATCH] of/irq: init struct resource to 0 in of_irq_to_resource()

[Sebastian Andrzej Siewior]

It almost does not matter because most users use only the ->start member
of the struct. However if this struct is passed to a platform device
which is then added via platform_device_add() then the ->parent member is
also used.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index a3c1c5a..e0a72fb 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -345,6 +345,7 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
 	if (r && irq) {
 		const char *name = NULL;
 
+		memset(r, 0, sizeof(*r));
 		/*
 		 * Get optional "interrupts-names" property to add a name
 		 * to the resource.
-- 
1.8.3.2

Re: [PATCH] of/irq: init struct resource to 0 in of_irq_to_resource()

[Rob Herring]

On 07/18/2013 05:24 AM, Sebastian Andrzej Siewior wrote:
> It almost does not matter because most users use only the ->start member
> of the struct. However if this struct is passed to a platform device
> which is then added via platform_device_add() then the ->parent member is
> also used.

Most users don't use the resource struct at all. The ones that do, all
seem to do a memset beforehand. So I think current behavior is correct.

There are some occurrences that pass a resource in, but then don't
actually use the resource. Those we should fix.

Rob

> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
>  drivers/of/irq.c | 1 +
>  1 file changed, 1 insearch/powerpc/sysdev/mv64x60_dev.crtion(+)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index a3c1c5a..e0a72fb 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -345,6 +345,7 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
>  	if (r && irq) {
>  		const char *name = NULL;
>  
> +		memset(r, 0, sizeof(*r));
>  		/*
>  		 * Get optional "interrupts-names" property to add a name
>  		 * to the resource.
> 

Re: [PATCH] of/irq: init struct resource to 0 in of_irq_to_resource()

[Sebastian Andrzej Siewior]

On 07/19/2013 04:49 AM, Rob Herring wrote:
> On 07/18/2013 05:24 AM, Sebastian Andrzej Siewior wrote:
>> It almost does not matter because most users use only the ->start member
>> of the struct. However if this struct is passed to a platform device
>> which is then added via platform_device_add() then the ->parent member is
>> also used.
> 
> Most users don't use the resource struct at all. The ones that do, all
> seem to do a memset beforehand. So I think current behavior is correct.

Seriously? That is your argument here? Most users use memset()
beforehand and that makes it correct? Now look at this: you pass a
pointer to fill out a struct and you don't do it properly.

There is of_address_to_resource() which does a memset() why is okay
here use memset()?

> 
> There are some occurrences that pass a resource in, but then don't
> actually use the resource. Those we should fix.



> 
> Rob
> 
>>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>> ---
>>  drivers/of/irq.c | 1 +
>>  1 file changed, 1 insearch/powerpc/sysdev/mv64x60_dev.crtion(+)
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index a3c1c5a..e0a72fb 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -345,6 +345,7 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
>>  	if (r && irq) {
>>  		const char *name = NULL;
>>  
>> +		memset(r, 0, sizeof(*r));
>>  		/*
>>  		 * Get optional "interrupts-names" property to add a name
>>  		 * to the resource.
>>
> 

Sebastian

Re: [PATCH] of/irq: init struct resource to 0 in of_irq_to_resource()

[Rob Herring]

On Fri, Jul 19, 2013 at 1:57 AM, Sebastian Andrzej Siewior
<bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> On 07/19/2013 04:49 AM, Rob Herring wrote:
>> On 07/18/2013 05:24 AM, Sebastian Andrzej Siewior wrote:
>>> It almost does not matter because most users use only the ->start member
>>> of the struct. However if this struct is passed to a platform device
>>> which is then added via platform_device_add() then the ->parent member is
>>> also used.
>>
>> Most users don't use the resource struct at all. The ones that do, all
>> seem to do a memset beforehand. So I think current behavior is correct.
>
> Seriously? That is your argument here? Most users use memset()
> beforehand and that makes it correct? Now look at this: you pass a
> pointer to fill out a struct and you don't do it properly.

>From a standpoint of initializing variables close to the point it is
allocated, yes.

> There is of_address_to_resource() which does a memset() why is okay
> here use memset()?

Fair enough. Consistency is important too. Rather than a memset, I'd
rather see the 3 remaining fields initialized and avoid setting most
of them twice.

Rob

>
>>
>> There are some occurrences that pass a resource in, but then don't
>> actually use the resource. Those we should fix.
>
>
>
>>
>> Rob
>>
>>>
>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>>> ---
>>>  drivers/of/irq.c | 1 +
>>>  1 file changed, 1 insearch/powerpc/sysdev/mv64x60_dev.crtion(+)
>>>
>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>>> index a3c1c5a..e0a72fb 100644
>>> --- a/drivers/of/irq.c
>>> +++ b/drivers/of/irq.c
>>> @@ -345,6 +345,7 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
>>>      if (r && irq) {
>>>              const char *name = NULL;
>>>
>>> +            memset(r, 0, sizeof(*r));
>>>              /*
>>>               * Get optional "interrupts-names" property to add a name
>>>               * to the resource.
>>>
>>
>
> Sebastian

Re: [PATCH] of/irq: init struct resource to 0 in of_irq_to_resource()

[Grant Likely]

On Thu, 18 Jul 2013 21:49:27 -0500, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 07/18/2013 05:24 AM, Sebastian Andrzej Siewior wrote:
> > It almost does not matter because most users use only the ->start member
> > of the struct. However if this struct is passed to a platform device
> > which is then added via platform_device_add() then the ->parent member is
> > also used.
> 
> Most users don't use the resource struct at all. The ones that do, all
> seem to do a memset beforehand. So I think current behavior is correct.
> 
> There are some occurrences that pass a resource in, but then don't
> actually use the resource. Those we should fix.

It's a reasonable safeguard though. I'm going to apply it.

g.