[U-Boot] RFC: getramsize() prototype and volatile qualifier

[Albert ARIBAUD]

Le 21/04/2011 19:02, Mike Frysinger a écrit :
> On Thu, Apr 21, 2011 at 3:09 AM, Albert ARIBAUD wrote:
>> Call it a detail, but I see that get_ram_size() calls sometime qualify
>> their argument as volatile and sometimes not, and this makes checkpatch
>> complain that volatiles are Bad(tm), which I would like to get fixed.
>>
>> The prototype for get_ram_size() in is
>>
>>         long    get_ram_size  (volatile long *, long);
>>
>> While I understand that the way get_ram_size() works, it needs to
>> perform volatile *accesses* to addresses computed from its arguments, I
>> don't see why it requires one of the arguments themselves to be volatile.
>>
>> Am I missing something here, particularly about some toolchain requiring
>> the argument to be volatile? I see no reason it should, but better safe
>> than sorry.
>
> the argument isnt volatile, the memory it points to is.  since
> get_ram_size() itself doesnt dereference the argument (only goes
> through a local "addr"), the volatile marking in the prototype could
> be dropped, but personally i dont see the point.  the prototype doesnt
> require callers to add volatile markings to their own code, and i
> could see someone using "base" in the future after the volatile being
> dropped and people not noticing right away.  the current code is safe
> and causes no problems by itself, so i see no value in changing it.
>
> this sounds like useless "gotta be checkpatch clean" work by people
> blindly following its output.
> -mike

Just because checkpatch complains about something does not mean that 
particular complaint is without merit. :)

Yes, the code works as it is; but it'll work just as well if volatile is 
removed from the prototype and calls, and that'll make the code simpler 
and more homogeneous overall, plus it'll give checkpatch one less cause 
for complaint. We gain something there, if not a functional plus, an 
overal quality plus.

As for people fiddling with get_ram_size() *body* and removing the 
volatile 'addr' and use the non-volatile 'base' directly, why would 
they? They will plainly see that 'addr' is volatile and 'base' is not 
(all the more because removing the volatile' attribute from 'base' will 
require casting it to 'base+int' where it belongs), and the very role of 
the function makes it clear to them that the volatile attribute is 
required for 'addr', so they'll keep 'addr'.

Plus, get_ram_size() is pretty stable -- actually, it has changed only 
once in 2006 since its creation in 2004.

Amicalement,
-- 
Albert.