Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1344433 > unrolled thread

Re: [PATCH] mfd: Fix MACRO for commonly declared MFD cell attributes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mfd: Fix MACRO for commonly declared MFD cell attributes Rhyland Klein <rklein@nvidia.com> - 2016-02-26 17:40 +0100
    Re: [PATCH] mfd: Fix MACRO for commonly declared MFD cell attributes Laxman Dewangan <ldewangan@nvidia.com> - 2016-02-29 14:00 +0100
      Re: [PATCH] mfd: Fix MACRO for commonly declared MFD cell attributes Lee Jones <lee.jones@linaro.org> - 2016-03-02 14:10 +0100

#1344433 — Re: [PATCH] mfd: Fix MACRO for commonly declared MFD cell attributes

On 2/19/2016 11:28 AM, Rhyland Klein wrote:
> On 2/19/2016 3:50 AM, Lee Jones wrote:
>> On Thu, 18 Feb 2016, Rhyland Klein wrote:
>>
>>> MFD_ARRAY_SIZE() would not accurately return 0 if the passed
>>> parameter was NULL. Fix this so that num_resources will
>>> accurately be 0 in the case that _res is NULL.
>>>
>>> cc: Lee Jones <lee.jones@linaro.org>
>>> cc: Laxman Dewangan <ldewangan@nvidia.com>
>>> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
>>> ---
>>>  include/linux/mfd/core.h | 15 +++++++++------
>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
>>> index 1a5a87f3cd38..62136ccff1df 100644
>>> --- a/include/linux/mfd/core.h
>>> +++ b/include/linux/mfd/core.h
>>> @@ -18,11 +18,11 @@
>>>  
>>>  #define MFD_ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>>>  
>>> -#define MFD_CELL_ALL(_name, _res, _pdata, _id, _compat, _match)		\
>>> +#define MFD_CELL_ALL(_name, _nres, _res, _pdata, _id, _compat, _match)	\
>>>  	{								\
>>>  		.name = (_name),					\
>>>  		.resources = (_res),					\
>>> -		.num_resources = MFD_ARRAY_SIZE((_res)),		\
>>> +		.num_resources = (_nres),				\
>>>  		.platform_data = (_pdata),				\
>>>  		.pdata_size = MFD_ARRAY_SIZE((_pdata)), 		\
>>>  		.of_compatible = (_compat),				\
>>> @@ -31,16 +31,19 @@
>>>  	}
>>>  
>>>  #define OF_MFD_CELL(_name, _res, _pdata, _id, _compat)			\
>>> -		MFD_CELL_ALL(_name, _res, _pdata, _id, _compat, NULL)	\
>>> +		MFD_CELL_ALL(_name, MFD_ARRAY_SIZE((_res)), _res,	\
>>> +			_pdata, _id, _compat, NULL)			\
>>
>> I'm confused.  Why would it be any different just by changing the call
>> site of MFD_ARRAY_SIZE?
> 
> It isn't different, but for MFD_CELL_NAME, it explicitly passes 0
> instead of using MFD_ARRAY_SIZE, as its the only place that doesn't
> expect to have resources.
> 
>>
>> And what about .platform_data?
> 
> This crashed for me (without the change) at :
> 
> mfd_add_device():
>         for (r = 0; r < cell->num_resources; r++) {
>                 res[r].name = cell->resources[r].name;
>                 res[r].flags = cell->resources[r].flags;
> 
> where dereferencing cell->resources[0] when there are no resources. I
> guess the platform_data could do the same, but I didn't run into it.
> 
>>
>> How about this change instead?
>>
>> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
>> index 1a5a87f..8440f42 100644
>> --- a/include/linux/mfd/core.h
>> +++ b/include/linux/mfd/core.h
>> @@ -16,7 +16,7 @@
>>  
>>  #include <linux/platform_device.h>
>>  
>> -#define MFD_ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>> +#define MFD_ARRAY_SIZE(arr) (arr ? (sizeof(arr) / sizeof((arr)[0])) : 0)
>>  
>>  #define MFD_CELL_ALL(_name, _res, _pdata, _id, _compat, _match)                \
>>         {                                                               \
>>
> That was my first thought too. However, I see this when I try to compile
> that:
> 
> In file included from drivers/mfd/max77620.c:18:0:
> include/linux/mfd/core.h:19:34: warning: the address of ‘gpio_resources’
> will always evaluate as ‘true’ [-Waddress]
>  #define MFD_ARRAY_SIZE(arr) (arr ? (sizeof(arr) / sizeof((arr)[0])) : 0)
> 
> 7 different times. This patch was the only way I seemed to be able to
> WAR around compile time warnings.
> 
> -rhyland
> 

Did you not see warnings like this when you compiled the kernel? Did you
find a different approach than what I proposed above to deal with it?
I'd like to get this in soon so that when the max77620 drivers are all
in and using it, it should be functional.

-rhyland

-- 
nvpublic

[toc] | [next] | [standalone]

#1345769

On Friday 26 February 2016 10:05 PM, Rhyland Klein wrote:
> On 2/19/2016 11:28 AM, Rhyland Klein wrote:
>> On 2/19/2016 3:50 AM, Lee Jones wrote:
>>> On Thu, 18 Feb 2016, Rhyland Klein wrote:
>>>
>>>>   #define MFD_ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>>>>   
>>>> -#define MFD_CELL_ALL(_name, _res, _pdata, _id, _compat, _match)		\
>>>> +#define MFD_CELL_ALL(_name, _nres, _res, _pdata, _id, _compat, _match)	\
>>>>   	{								\
>>>>   		.name = (_name),					\
>>>>   		.resources = (_res),					\
>>>> -		.num_resources = MFD_ARRAY_SIZE((_res)),		\
>>>> +		.num_resources = (_nres),				\
>>>>   		.platform_data = (_pdata),				\
>>>>   		.pdata_size = MFD_ARRAY_SIZE((_pdata)), 		\
The pdata_size initialization is also not correct. This is not the array 
count but the size
of the platform data which is passed. Should be sizeof((_pdata))



>>   
>>   #define MFD_CELL_ALL(_name, _res, _pdata, _id, _compat, _match)                \
>>          {                                                               \
>>
>> That was my first thought too. However, I see this when I try to compile
>> that:
>>
>> In file included from drivers/mfd/max77620.c:18:0:
>> include/linux/mfd/core.h:19:34: warning: the address of ‘gpio_resources’
>> will always evaluate as ‘true’ [-Waddress]
>>   #define MFD_ARRAY_SIZE(arr) (arr ? (sizeof(arr) / sizeof((arr)[0])) : 0)
>>
>> 7 different times. This patch was the only way I seemed to be able to
>> WAR around compile time warnings.
>>
>> -rhyland
>>
> Did you not see warnings like this when you compiled the kernel? Did you
> find a different approach than what I proposed above to deal with it?
> I'd like to get this in soon so that when the max77620 drivers are all
> in and using it, it should be functional.
>
I think the following change also crash in runtime:

/***
commit e60a946f05db2cac857025da6ffb72df48d3be54
Author: Lee Jones <lee.jones@linaro.org>

     mfd: ab8500: Provide a small example using new MFD cell MACROs

***/

Should we have something MFD_CELL_RES, MFD_CELL_RES_PDATA, 
MFD_CELL_PDATA, for more common user and not to pass the NULL here.

[toc] | [prev] | [next] | [standalone]

#1348026

On Mon, 29 Feb 2016, Laxman Dewangan wrote:

> 
> On Friday 26 February 2016 10:05 PM, Rhyland Klein wrote:
> >On 2/19/2016 11:28 AM, Rhyland Klein wrote:
> >>On 2/19/2016 3:50 AM, Lee Jones wrote:
> >>>On Thu, 18 Feb 2016, Rhyland Klein wrote:
> >>>
> >>>>  #define MFD_ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> >>>>-#define MFD_CELL_ALL(_name, _res, _pdata, _id, _compat, _match)		\
> >>>>+#define MFD_CELL_ALL(_name, _nres, _res, _pdata, _id, _compat, _match)	\
> >>>>  	{								\
> >>>>  		.name = (_name),					\
> >>>>  		.resources = (_res),					\
> >>>>-		.num_resources = MFD_ARRAY_SIZE((_res)),		\
> >>>>+		.num_resources = (_nres),				\
> >>>>  		.platform_data = (_pdata),				\
> >>>>  		.pdata_size = MFD_ARRAY_SIZE((_pdata)), 		\
> The pdata_size initialization is also not correct. This is not the
> array count but the size
> of the platform data which is passed. Should be sizeof((_pdata))
> 
> 
> 
> >>  #define MFD_CELL_ALL(_name, _res, _pdata, _id, _compat, _match)                \
> >>         {                                                               \
> >>
> >>That was my first thought too. However, I see this when I try to compile
> >>that:
> >>
> >>In file included from drivers/mfd/max77620.c:18:0:
> >>include/linux/mfd/core.h:19:34: warning: the address of ‘gpio_resources’
> >>will always evaluate as ‘true’ [-Waddress]
> >>  #define MFD_ARRAY_SIZE(arr) (arr ? (sizeof(arr) / sizeof((arr)[0])) : 0)
> >>
> >>7 different times. This patch was the only way I seemed to be able to
> >>WAR around compile time warnings.
> >>
> >>-rhyland
> >>
> >Did you not see warnings like this when you compiled the kernel? Did you
> >find a different approach than what I proposed above to deal with it?
> >I'd like to get this in soon so that when the max77620 drivers are all
> >in and using it, it should be functional.
> >
> I think the following change also crash in runtime:
> 
> /***
> commit e60a946f05db2cac857025da6ffb72df48d3be54
> Author: Lee Jones <lee.jones@linaro.org>
> 
>     mfd: ab8500: Provide a small example using new MFD cell MACROs
> 
> ***/
> 
> Should we have something MFD_CELL_RES, MFD_CELL_RES_PDATA,
> MFD_CELL_PDATA, for more common user and not to pass the NULL here.

I'll have a re-think about this.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web